Fix the path layout in both bookmark manager and download location. (issue 8997)

http://crbug.com/8997

Previously, the path itself and every single path component are marked with LTR marks.

The fix is only mark the path as a whole (but not every single path component)with LTR marks.

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

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

3 files changed, 52 insertions, 61 deletions

diff --git a/chrome/browser/bookmarks/bookmark_table_model.cc b/chrome/browser/bookmarks/bookmark_table_model.cc
index a95f898..2f294b1 100644
--- a/chrome/browser/bookmarks/bookmark_table_model.cc
+++ b/chrome/browser/bookmarks/bookmark_table_model.cc
@@ -316,10 +316,25 @@ std::wstring BookmarkTableModel::GetText(int row, int column_id) {
     case IDS_BOOKMARK_TABLE_PATH: {
       std::wstring path;
       BuildPath(node->GetParent(), &path);
-      // Force path to have LTR directionality. The whole path needs to be
-      // marked with LRE-PDF, so that the path containing both LTR and RTL
-      // subfolder names (such as "CBA/FED/(xji)/.x.j.", in which, "CBA" and
-      // "FED" are subfolder names in Hebrew) can be displayed as LTR.
+      // Force path to have LTR directionality. The whole path (but not every
+      // single path component) is marked with LRE-PDF. For example,
+      // ALEPH/BET/GIMEL (using uppercase English for Hebrew) is supposed to
+      // appear (visually) as LEMIG/TEB/HPELA; foo/C/B/A.doc refers to file
+      // C.doc in directory B in directory A in directory foo, not to file
+      // A.doc in directory B in directory C in directory foo. The reason to
+      // mark the whole path, but not every single path component, as LTR is
+      // because paths need to get written in text documents, and that is how
+      // they will appear there. Being a saint and doing the tedious formatting
+      // to every single path component to get it to come out in the logical
+      // order will accomplish nothing but confuse people, since they will now
+      // see both formats being used, and will never know what anything means.
+      // Furthermore, doing the "logical" formatting with characters like LRM,
+      // LRE, and PDF to every single path component means that when someone
+      // copy/pastes your path, it will still contain those characters, and
+      // trying to access the file will fail because of them. Windows Explorer,
+      // Firefox, IE, Nautilus, gedit choose to format only the whole path as
+      // LTR too. The point here is to display the path the same way as it's
+      // displayed by other software.
       if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT)
         l10n_util::WrapStringWithLTRFormatting(&path);
       return path;
@@ -373,11 +388,5 @@ void BookmarkTableModel::BuildPath(BookmarkNode* node, std::wstring* path) {
   }
   BuildPath(node->GetParent(), path);
   path->append(l10n_util::GetString(IDS_BOOKMARK_TABLE_PATH_SEPARATOR));
-  // Force 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".
-  if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) {
-    path->push_back(static_cast<wchar_t>(l10n_util::kLeftToRightMark));
-  }
   path->append(node->GetTitle());
 }
diff --git a/chrome/common/l10n_util.cc b/chrome/common/l10n_util.cc
index de01f6b..8615622 100644
--- a/chrome/common/l10n_util.cc
+++ b/chrome/common/l10n_util.cc
@@ -562,42 +562,18 @@ void WrapStringWithRTLFormatting(std::wstring* text) {
 
 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.
+  // Wrap the overall path with LRE-PDF pair which essentialy marks the
+  // string as a Left-To-Right string.
   // 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]));
+    rtl_safe_path->append(UTF8ToUTF16(path.value()));
 #elif defined(OS_WIN)
-    rtl_safe_path->append(path_components[index]);
+    rtl_safe_path->append(path.value());
 #else  // defined(OS_LINUX)
-    std::wstring one_component =
-        base::SysNativeMBToWide(path_components[index]);
-    rtl_safe_path->append(WideToUTF16(one_component));
+    std::wstring wide_path = base::SysNativeMBToWide(path.value());
+    rtl_safe_path->append(WideToUTF16(wide_path));
 #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);
 }
diff --git a/chrome/common/l10n_util_unittest.cc b/chrome/common/l10n_util_unittest.cc
index eb0b028..122dc3d 100644
--- a/chrome/common/l10n_util_unittest.cc
+++ b/chrome/common/l10n_util_unittest.cc
@@ -313,59 +313,65 @@ TEST_F(L10nUtilTest, WrapPathWithLTRFormatting) {
   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"
+      L"\x202a"L"c:" + kSeparator + L"foo" + kSeparator +
+      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"
+      L"\x202a"L"c:" + kSeparator + L"foo" + kSeparator +
+      L"bar" + kSeparator + 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"
+      L"\x202a"L"c:" + kSeparator + L"(foo)" + kSeparator +
+      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"
+      L"\x202a"L"c:" + kSeparator + L"foo" + kSeparator +
+      L"bar" + kSeparator + L"\x202c",
     },
     // Test path with RTL character.
     { L"c:" + kSeparator + L"\x05d0",
-      L"\x202a"L"c:" + kSeparator + L"\x200e\x05d0\x202c",
+      L"\x202a"L"c:" + kSeparator + L"\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",
+      L"\x202a"L"c:" + kSeparator + L"\x05d0" + kSeparator +
+      L"\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",
+      L"\x202a"L"c:" + kSeparator + L"\x05d0" + kSeparator +
+      L"\x0622" + kSeparator + L"(foo)" + kSeparator +
+      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"
+      L"\x202a" + kSeparator + L"foo" + kSeparator + L"bar" +
+      kSeparator + 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"
+      L"\x202a"L"." + kSeparator + 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"
+      L"\x202a"L".." + kSeparator + L"foo" + kSeparator +
+      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"
+      L"\x202a" + kSeparator + kSeparator + L"foo" + kSeparator +
+      L"bar.jpg" + L"\x202c"
+    },
+    // Test path with mixed RTL/LTR directory names.
+    { L"c:" + kSeparator + L"foo" + kSeparator + L"\x05d0" + kSeparator +
+      L"\x0622" + kSeparator + L"\x05d1.jpg",
+      L"\x202a"L"c:" + kSeparator + L"foo" + kSeparator + L"\x05d0" +
+      kSeparator + L"\x0622" + kSeparator + L"\x05d1.jpg" + L"\x202c",
     },
     // Test empty path.
     { L"",