This CL fix the following 2 bugs:

1.7324 -- RTL: Path contains Hebrew is wrong on "Download location" text box
(http://crbug.com/7324)

2. 7326 -- RTL: Folder icon missing on "Download location" text box
(http://crbug.com/7326)

The fix are:
1. force path to have LTR directionality.
2. draw icon in mirrored position in RTL locales.

Steps for test:
1. Create a folder with a Hebrew file name on Hebrew XP
2. Change the download location to the folder created in step 1
3. Observe the path displayed on "Google Chrome Options" --> "Minor Tweaks" --> "Download location" text box

Without the fix: 
The path contains Hebrew folder name is wrong. It displayed as "cCBA\:" while the path is "c:\CBA" where "CBA" is a Hebrew folder name.
And there is no folder icon in the "download location" text box.

With the fix:
The path displayed correctly as "c:\CBA" or "c:\CBA\FED" where "FED" is subfolder of "CBA".
And the folder icon showed at the very right.


Review URL: http://codereview.chromium.org/20038

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@10121 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 185 insertions, 23 deletions

diff --git a/chrome/browser/bookmarks/bookmark_table_model.cc b/chrome/browser/bookmarks/bookmark_table_model.cc
index 4f85120..d2f4e6d 100644
--- a/chrome/browser/bookmarks/bookmark_table_model.cc
+++ b/chrome/browser/bookmarks/bookmark_table_model.cc
@@ -363,7 +363,7 @@ void BookmarkTableModel::BuildPath(BookmarkNode* node, std::wstring* path) {
   // (in which, "CBA" and "FED" stand for folder names in Hebrew, and "FED" is
   // a subfolder of "CBA") will be displayed as "FED/CBA".
   if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) {
-    path->append(l10n_util::kLeftToRightMark);
+    path->push_back(static_cast<wchar_t>(l10n_util::kLeftToRightMark));
   }
   path->append(node->GetTitle());
 }
diff --git a/chrome/browser/views/options/advanced_contents_view.cc b/chrome/browser/views/options/advanced_contents_view.cc
index 898db6f..080f578 100644
--- a/chrome/browser/views/options/advanced_contents_view.cc
+++ b/chrome/browser/views/options/advanced_contents_view.cc
@@ -691,8 +691,11 @@ void WebContentSection::InitControlLayout() {
   } else {
     // Add an RTL mark so that
     // ":" in "Google Gears:" in Hebrew Chrome is displayed left-most.
-    gears_label_ = new views::Label(l10n_util::GetString(
-        IDS_OPTIONS_GEARSSETTINGS_GROUP_NAME) + l10n_util::kRightToLeftMark);
+    std::wstring gearssetting_group_name =
+        l10n_util::GetString(IDS_OPTIONS_GEARSSETTINGS_GROUP_NAME);
+    gearssetting_group_name.push_back(
+        static_cast<wchar_t>(l10n_util::kRightToLeftMark));
+    gears_label_ = new views::Label(gearssetting_group_name);
   }
   gears_settings_button_ = new views::NativeButton(
       l10n_util::GetString(IDS_OPTIONS_GEARSSETTINGS_CONFIGUREGEARS_BUTTON));
diff --git a/chrome/browser/views/options/content_page_view.cc b/chrome/browser/views/options/content_page_view.cc
index 5ca51cb..b3a728c 100644
--- a/chrome/browser/views/options/content_page_view.cc
+++ b/chrome/browser/views/options/content_page_view.cc
@@ -52,7 +52,7 @@ class FileDisplayArea : public views::View {
   FileDisplayArea();
   virtual ~FileDisplayArea();
 
-  void SetFile(const std::wstring& file_path);
+  void SetFile(const FilePath& file_path);
 
   // views::View overrides:
   virtual void Paint(ChromeCanvas* canvas);
@@ -94,8 +94,15 @@ FileDisplayArea::FileDisplayArea()
 FileDisplayArea::~FileDisplayArea() {
 }
 
-void FileDisplayArea::SetFile(const std::wstring& file_path) {
-  text_field_->SetText(file_path);
+void FileDisplayArea::SetFile(const FilePath& file_path) {
+  // Force file path to have LTR directionality.
+  if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) {
+    string16 localized_file_path;
+    l10n_util::WrapPathWithLTRFormatting(file_path, &localized_file_path);
+    text_field_->SetText(UTF16ToWide(localized_file_path));
+  } else {
+    text_field_->SetText(file_path.ToWStringHack());
+  }
 }
 
 void FileDisplayArea::Paint(ChromeCanvas* canvas) {
@@ -105,7 +112,9 @@ void FileDisplayArea::Paint(ChromeCanvas* canvas) {
       dc, EP_EDITTEXT, ETS_READONLY, 0, &rect,
       skia::SkColorToCOLORREF(text_field_background_color_), true, true);
   canvas->endPlatformPaint();
-  canvas->DrawBitmapInt(default_folder_icon_, icon_bounds_.x(),
+  // Mirror left point for icon_bounds_ to draw icon in RTL locales correctly.
+  canvas->DrawBitmapInt(default_folder_icon_,
+                        MirroredLeftPointForRect(icon_bounds_),
                         icon_bounds_.y());
 }
 
@@ -144,11 +153,18 @@ void FileDisplayArea::Init() {
   text_field_->SetBackgroundColor(text_field_background_color_);
 }
 
+// static
 void FileDisplayArea::InitClass() {
   static bool initialized = false;
   if (!initialized) {
+    // We'd prefer to use UILayoutIsRightToLeft() to perform the RTL
+    // environment check, but it's nonstatic, so, instead, we check whether the
+    // locale is RTL.
+    bool ui_is_rtl = l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT;
     ResourceBundle& rb = ResourceBundle::GetSharedInstance();
-    default_folder_icon_ = *rb.GetBitmapNamed(IDR_FOLDER_CLOSED);
+    default_folder_icon_ = *rb.GetBitmapNamed(ui_is_rtl ?
+                                              IDR_FOLDER_CLOSED_RTL :
+                                              IDR_FOLDER_CLOSED);
     initialized = true;
   }
 }
@@ -501,6 +517,6 @@ void ContentPageView::InitFontsLangGroup() {
 
 void ContentPageView::UpdateDownloadDirectoryDisplay() {
   download_default_download_location_display_->SetFile(
-      default_download_location_.GetValue());
+      FilePath::FromWStringHack(default_download_location_.GetValue()));
 }
 
diff --git a/chrome/common/l10n_util.cc b/chrome/common/l10n_util.cc
index c4b276f..6365291 100644
--- a/chrome/common/l10n_util.cc
+++ b/chrome/common/l10n_util.cc
@@ -7,8 +7,14 @@
 #include "chrome/common/l10n_util.h"
 
 #include "base/command_line.h"
+#include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
+#include "base/scoped_ptr.h"
+#include "base/string16.h"
+#include "base/string_piece.h"
+#include "base/string_util.h"
+#include "base/sys_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/gfx/chrome_canvas.h"
@@ -303,10 +309,10 @@ std::wstring GetLocalName(const std::wstring& locale_code_wstr,
   DCHECK(U_SUCCESS(error));
   name_local.resize(actual_size);
   // Add an RTL mark so parentheses are properly placed.
-  if (is_for_ui && GetTextDirection() == RIGHT_TO_LEFT)
-    return name_local + kRightToLeftMark;
-  else
-    return name_local;
+  if (is_for_ui && GetTextDirection() == RIGHT_TO_LEFT) {
+    name_local.push_back(static_cast<wchar_t>(kRightToLeftMark));
+  }
+  return name_local;
 }
 
 std::wstring GetString(int message_id) {
@@ -514,12 +520,17 @@ bool AdjustStringForLocaleDirection(const std::wstring& text,
 }
 
 bool StringContainsStrongRTLChars(const std::wstring& text) {
-  const wchar_t* string = text.c_str();
-  int length = static_cast<int>(text.length());
-  int position = 0;
+#if defined(WCHAR_T_IS_UTF32)
+  string16 text_utf16 = WideToUTF16(text);
+  const UChar* string = text_utf16.c_str();
+#else
+  const UChar* string = text.c_str();
+#endif
+  size_t length = text.length();
+  size_t position = 0;
   while (position < length) {
     UChar32 character;
-    int next_position = position;
+    size_t next_position = position;
     U16_NEXT(string, next_position, length, character);
 
     // Now that we have the character, we use ICU in order to query for the
@@ -536,18 +547,60 @@ bool StringContainsStrongRTLChars(const std::wstring& text) {
 
 void WrapStringWithLTRFormatting(std::wstring* text) {
   // Inserting an LRE (Left-To-Right Embedding) mark as the first character.
-  text->insert(0, L"\x202A");
+  text->insert(0, 1, static_cast<wchar_t>(kLeftToRightEmbeddingMark));
 
   // Inserting a PDF (Pop Directional Formatting) mark as the last character.
-  text->append(L"\x202C");
+  text->push_back(static_cast<wchar_t>(kPopDirectionalFormatting));
 }
 
 void WrapStringWithRTLFormatting(std::wstring* text) {
   // Inserting an RLE (Right-To-Left Embedding) mark as the first character.
-  text->insert(0, L"\x202B");
+  text->insert(0, 1, static_cast<wchar_t>(kRightToLeftEmbeddingMark));
 
   // Inserting a PDF (Pop Directional Formatting) mark as the last character.
-  text->append(L"\x202C");
+  text->push_back(static_cast<wchar_t>(kPopDirectionalFormatting));
+}
+
+void WrapPathWithLTRFormatting(const FilePath& path,
+                               string16* rtl_safe_path) {
+  // Split the path.
+  std::vector<FilePath::StringType> path_components;
+  file_util::PathComponents(path, &path_components);
+  // Compose the whole path from components with the following 2 additions:
+  // 1. Wrap the overall path with LRE-PDF pair which essentialy marks the
+  // string as a Left-To-Right string. Otherwise, the punctuation (if there is
+  // any) at the end of the path will not be displayed at the correct position.
+  // Inserting an LRE (Left-To-Right Embedding) mark as the first character.
+  rtl_safe_path->push_back(kLeftToRightEmbeddingMark);
+  char16 path_separator = static_cast<char16>(FilePath::kSeparators[0]);
+  for (size_t index = 0; index < path_components.size(); ++index) {
+#if defined(OS_MACOSX)
+    rtl_safe_path->append(UTF8ToUTF16(path_components[index]));
+#elif defined(OS_WIN)
+    rtl_safe_path->append(path_components[index]);
+#else  // defined(OS_LINUX)
+    std::wstring one_component =
+        base::SysNativeMBToWide(path_components[index]);
+    rtl_safe_path->append(WideToUTF16(one_component));
+#endif
+    bool first_component_is_separator =
+        ((index == 0) &&
+         (path_components[0].length() == 1) &&
+         (FilePath::IsSeparator(path_components[0][0])));
+    bool last_component = (index == path_components.size() - 1);
+    // Add separator for components except for the first component if itself is
+    // a separator, and except for the last component.
+    if (!last_component && !first_component_is_separator) {
+      rtl_safe_path->push_back(path_separator);
+      // 2. Add left-to-right mark after path separator to force each subfolder
+      // in the path to have LTR directionality. Otherwise, folder path
+      // "CBA/FED" (in which, "CBA" and "FED" stand for folder names in Hebrew,
+      // and "FED" is a subfolder of "CBA") will be displayed as "FED/CBA".
+      rtl_safe_path->push_back(kLeftToRightMark);
+    }
+  }
+  // Inserting a PDF (Pop Directional Formatting) mark as the last character.
+  rtl_safe_path->push_back(kPopDirectionalFormatting);
 }
 
 int DefaultCanvasTextAlignment() {
diff --git a/chrome/common/l10n_util.h b/chrome/common/l10n_util.h
index 3f971ac..0c67712 100644
--- a/chrome/common/l10n_util.h
+++ b/chrome/common/l10n_util.h
@@ -21,17 +21,22 @@
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/scoped_ptr.h"
+#include "base/string16.h"
 #include "base/string_util.h"
 #include "third_party/icu38/public/common/unicode/ubidi.h"
 #include "unicode/coll.h"
 #include "unicode/locid.h"
 
+class FilePath;
 class PrefService;
 
 namespace l10n_util {
 
-const wchar_t kRightToLeftMark[] = L"\x200f";
-const wchar_t kLeftToRightMark[] = L"\x200e";
+const char16 kRightToLeftMark = 0x200f;
+const char16 kLeftToRightMark = 0x200e;
+const char16 kLeftToRightEmbeddingMark = 0x202A;
+const char16 kRightToLeftEmbeddingMark = 0x202B;
+const char16 kPopDirectionalFormatting = 0x202C;
 
 // This method is responsible for determining the locale as defined below. In
 // nearly all cases you shouldn't call this, rather use GetApplicationLocale
@@ -151,6 +156,12 @@ void WrapStringWithLTRFormatting(std::wstring* text);
 // strings are rendered properly in an LTR context.
 void WrapStringWithRTLFormatting(std::wstring* text);
 
+// Wraps individual file path components to get them to display correctly in an
+// RTL UI. All filepaths should be passed through this function before display
+// in UI for RTL locales.
+void WrapPathWithLTRFormatting(const FilePath& path,
+                               string16* rtl_safe_path);
+
 // Returns the locale-dependent extended window styles.
 // This function is used for adding locale-dependent extended window styles
 // (e.g. WS_EX_LAYOUTRTL, WS_EX_RTLREADING, etc.) when creating a window.
diff --git a/chrome/common/l10n_util_unittest.cc b/chrome/common/l10n_util_unittest.cc
index 18cb8f0..3d93ef7 100644
--- a/chrome/common/l10n_util_unittest.cc
+++ b/chrome/common/l10n_util_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "build/build_config.h"
 
+#include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
@@ -159,5 +160,83 @@ TEST_F(L10nUtilTest, GetAppLocale) {
   Locale::setDefault(locale, error_code);
 }
 
+typedef struct {
+  std::wstring path;
+  std::wstring wrapped_path;
+} PathAndWrappedPath;
+
+TEST_F(L10nUtilTest, WrapPathWithLTRFormatting) {
+  std::wstring kSeparator;
+  kSeparator.push_back(static_cast<wchar_t>(FilePath::kSeparators[0]));
+  const PathAndWrappedPath test_data[] = {
+    // Test common path, such as "c:\foo\bar".
+    { L"c:" + kSeparator + L"foo" + kSeparator + L"bar",
+      L"\x202a"L"c:" + kSeparator + L"\x200e"L"foo" + kSeparator +
+      L"\x200e"L"bar\x202c"
+    },
+    // Test path with file name, such as "c:\foo\bar\test.jpg".
+    { L"c:" + kSeparator + L"foo" + kSeparator + L"bar" + kSeparator +
+      L"test.jpg",
+      L"\x202a"L"c:" + kSeparator + L"\x200e"L"foo" + kSeparator +
+      L"\x200e"L"bar" + kSeparator + L"\x200e"L"test.jpg\x202c"
+    },
+    // Test path ending with punctuation, such as "c:\(foo)\bar.".
+    { L"c:" + kSeparator + L"(foo)" + kSeparator + L"bar.",
+      L"\x202a"L"c:" + kSeparator + L"\x200e"L"(foo)" + kSeparator +
+      L"\x200e"L"bar.\x202c"
+    },
+    // Test path ending with separator, such as "c:\foo\bar\".
+    { L"c:" + kSeparator + L"foo" + kSeparator + L"bar" + kSeparator,
+      L"\x202a"L"c:" + kSeparator + L"\x200e"L"foo" + kSeparator +
+      L"\x200e"L"bar" + kSeparator + L"\x200e\x202c"
+    },
+    // Test path with RTL character.
+    { L"c:" + kSeparator + L"\x05d0",
+      L"\x202a"L"c:" + kSeparator + L"\x200e\x05d0\x202c",
+    },
+    // Test path with 2 level RTL directory names.
+    { L"c:" + kSeparator + L"\x05d0" + kSeparator + L"\x0622",
+      L"\x202a"L"c:" + kSeparator + L"\x200e\x05d0" + kSeparator +
+      L"\x200e\x0622\x202c",
+    },
+    // Test path with mixed RTL/LTR directory names and ending with punctuation.
+    { L"c:" + kSeparator + L"\x05d0" + kSeparator + L"\x0622" + kSeparator +
+      L"(foo)" + kSeparator + L"b.a.r.",
+      L"\x202a"L"c:" + kSeparator + L"\x200e\x05d0" + kSeparator +
+      L"\x200e\x0622" + kSeparator + L"\x200e"L"(foo)" + kSeparator +
+      L"\x200e"L"b.a.r.\x202c",
+    },
+    // Test path without driver name, such as "/foo/bar/test/jpg".
+    { kSeparator + L"foo" + kSeparator + L"bar" + kSeparator + L"test.jpg",
+      L"\x202a" + kSeparator + L"foo" + kSeparator + L"\x200e" + L"bar" +
+      kSeparator + L"\x200e" + L"test.jpg" + L"\x202c"
+    },
+    // Test path start with current directory, such as "./foo".
+    { L"." + kSeparator + L"foo",
+      L"\x202a"L"." + kSeparator + L"\x200e" + L"foo" + L"\x202c"
+    },
+    // Test path start with parent directory, such as "../foo/bar.jpg".
+    { L".." + kSeparator + L"foo" + kSeparator + L"bar.jpg",
+      L"\x202a"L".." + kSeparator + L"\x200e" + L"foo" + kSeparator +
+      L"\x200e" + L"bar.jpg" + L"\x202c"
+    },
+    // Test absolute path, such as "//foo/bar.jpg".
+    { kSeparator + kSeparator + L"foo" + kSeparator + L"bar.jpg",
+      L"\x202a" + kSeparator + kSeparator + L"\x200e"L"foo" + kSeparator +
+      L"\x200e"L"bar.jpg" + L"\x202c"
+    },
+    // Test empty path.
+    { L"",
+      L"\x202a\x202c"
+    }
+  };
+  for (unsigned int i = 0; i < arraysize(test_data); ++i) {
+    string16 localized_file_path_string;
+    FilePath path = FilePath::FromWStringHack(test_data[i].path);
+    l10n_util::WrapPathWithLTRFormatting(path, &localized_file_path_string);
+    std::wstring wrapped_path = UTF16ToWide(localized_file_path_string);
+    EXPECT_EQ(wrapped_path, test_data[i].wrapped_path);
+  }
+}
 }