This CL fixes issue 10860 - RTL: Hebrew file names should have forced LTR

directionality in download shelf.

File names in download shelf are forced to be LTR in DownloadItemView and
through ElideFileName().

BUG=http://crbug.com/10860
TEST=1. Open chrome with Hebrew UI.
2. Right click a link and chose Save As... (4th item from the top for
non-Hebrew
speakers)
3. In the save as dialog name the file קובץ.html
4. In the download shelf the filename should display as קובץ.html (not
html.קובץ)
Review URL: http://codereview.chromium.org/131001

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

8 files changed, 91 insertions, 24 deletions

diff --git a/app/gfx/text_elider.cc b/app/gfx/text_elider.cc
index 7f56fac..a1db1c6 100644
--- a/app/gfx/text_elider.cc
+++ b/app/gfx/text_elider.cc
@@ -4,6 +4,7 @@
 
 #include "app/gfx/font.h"
 #include "app/gfx/text_elider.h"
+#include "app/l10n_util.h"
 #include "base/file_path.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
@@ -257,8 +258,10 @@ std::wstring ElideFilename(const FilePath& filename,
                            const gfx::Font& font,
                            int available_pixel_width) {
   int full_width = font.GetStringWidth(filename.ToWStringHack());
-  if (full_width <= available_pixel_width)
-    return filename.ToWStringHack();
+  if (full_width <= available_pixel_width) {
+    std::wstring elided_name = filename.ToWStringHack();
+    return l10n_util::GetDisplayStringInLTRDirectionality(&elided_name);
+  }
 
 #if defined(OS_WIN)
   std::wstring extension = filename.Extension();
@@ -268,18 +271,25 @@ std::wstring ElideFilename(const FilePath& filename,
   std::wstring rootname =
       filename.BaseName().RemoveExtension().ToWStringHack();
 
-  if (rootname.empty() || extension.empty())
-    return ElideText(filename.ToWStringHack(), font, available_pixel_width);
+  if (rootname.empty() || extension.empty()) {
+    std::wstring elided_name = ElideText(filename.ToWStringHack(), font,
+                                         available_pixel_width);
+    return l10n_util::GetDisplayStringInLTRDirectionality(&elided_name);
+  }
 
   int ext_width = font.GetStringWidth(extension);
   int root_width = font.GetStringWidth(rootname);
 
   // We may have trimmed the path.
-  if (root_width + ext_width <= available_pixel_width)
-    return rootname + extension;
+  if (root_width + ext_width <= available_pixel_width) {
+    std::wstring elided_name = rootname + extension;
+    return l10n_util::GetDisplayStringInLTRDirectionality(&elided_name);
+  }
 
   int available_root_width = available_pixel_width - ext_width;
-  return ElideText(rootname, font, available_root_width) + extension;
+  std::wstring elided_name = ElideText(rootname, font, available_root_width);
+  elided_name += extension;
+  return l10n_util::GetDisplayStringInLTRDirectionality(&elided_name);
 }
 
 // This function adds an ellipsis at the end of the text if the text
diff --git a/app/gfx/text_elider.h b/app/gfx/text_elider.h
index 4c9c1fa..aa33c29 100644
--- a/app/gfx/text_elider.h
+++ b/app/gfx/text_elider.h
@@ -42,7 +42,10 @@ std::wstring ElideText(const std::wstring& text,
 
 // Elide a filename to fit a given pixel width, with an emphasis on not hiding
 // the extension unless we have to. If filename contains a path, the path will
-// be removed if filename doesn't fit into available_pixel_width.
+// be removed if filename doesn't fit into available_pixel_width. The elided
+// filename is forced to have LTR directionality, which means that in RTL UI
+// the elided filename is wrapped with LRE (Left-To-Right Embedding) mark and
+// PDF (Pop Directional Formatting) mark.
 std::wstring ElideFilename(const FilePath& filename,
                            const gfx::Font& font,
                            int available_pixel_width);
diff --git a/app/gfx/text_elider_unittest.cc b/app/gfx/text_elider_unittest.cc
index abb7c23..fb6e7523 100644
--- a/app/gfx/text_elider_unittest.cc
+++ b/app/gfx/text_elider_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "app/gfx/font.h"
 #include "app/gfx/text_elider.h"
+#include "app/l10n_util.h"
 #include "base/file_path.h"
 #include "base/string_util.h"
 #include "googleurl/src/gurl.h"
@@ -176,7 +177,10 @@ TEST(TextEliderTest, TestFilenameEliding) {
   static const gfx::Font font;
   for (size_t i = 0; i < arraysize(testcases); ++i) {
     FilePath filepath(testcases[i].input);
-    EXPECT_EQ(testcases[i].output, ElideFilename(filepath,
+    std::wstring expected = testcases[i].output;
+    if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT)
+      l10n_util::WrapStringWithLTRFormatting(&expected);
+    EXPECT_EQ(expected, ElideFilename(filepath,
         font,
         font.GetStringWidth(testcases[i].output)));
   }
diff --git a/app/l10n_util.cc b/app/l10n_util.cc
index 2dd3cf2..f23de27 100644
--- a/app/l10n_util.cc
+++ b/app/l10n_util.cc
@@ -830,6 +830,12 @@ void WrapPathWithLTRFormatting(const FilePath& path,
   rtl_safe_path->push_back(kPopDirectionalFormatting);
 }
 
+std::wstring GetDisplayStringInLTRDirectionality(std::wstring* text) {
+  if (GetTextDirection() == RIGHT_TO_LEFT)
+    WrapStringWithLTRFormatting(text);
+  return *text;
+}
+
 int DefaultCanvasTextAlignment() {
   if (GetTextDirection() == LEFT_TO_RIGHT) {
     return gfx::Canvas::TEXT_ALIGN_LEFT;
diff --git a/app/l10n_util.h b/app/l10n_util.h
index 85fa36b..5e50d6e 100644
--- a/app/l10n_util.h
+++ b/app/l10n_util.h
@@ -248,12 +248,18 @@ 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.
+// Wraps file path to get it to display correctly in 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);
 
+// Given the string in |text|, this function returns the adjusted string having
+// LTR directionality for display purpose. Which means that in RTL locale the
+// string is wrapped with LRE (Left-To-Right Embedding) and PDF (Pop
+// Directional Formatting) marks and returned. In LTR locale, the string itself
+// is returned.
+std::wstring GetDisplayStringInLTRDirectionality(std::wstring* text);
+
 // Returns the default text alignment to be used when drawing text on a
 // gfx::Canvas based on the directionality of the system locale language. This
 // function is used by gfx::Canvas::DrawStringInt when the text alignment is
diff --git a/app/l10n_util_unittest.cc b/app/l10n_util_unittest.cc
index 18a568c..c9409fe 100644
--- a/app/l10n_util_unittest.cc
+++ b/app/l10n_util_unittest.cc
@@ -412,6 +412,33 @@ TEST_F(L10nUtilTest, WrapPathWithLTRFormatting) {
   }
 }
 
+typedef struct  {
+    std::wstring raw_filename;
+    std::wstring display_string;
+} StringAndLTRString;
+
+TEST_F(L10nUtilTest, GetDisplayStringInLTRDirectionality) {
+  const StringAndLTRString test_data[] = {
+    { L"test", L"\x202atest\x202c" },
+    { L"test.html", L"\x202atest.html\x202c" },
+    { L"\x05d0\x05d1\x05d2", L"\x202a\x05d0\x05d1\x05d2\x202c" },
+    { L"\x05d0\x05d1\x05d2.txt", L"\x202a\x05d0\x05d1\x05d2.txt\x202c" },
+    { L"\x05d0"L"abc", L"\x202a\x05d0"L"abc\x202c" },
+    { L"\x05d0"L"abc.txt", L"\x202a\x05d0"L"abc.txt\x202c" },
+    { L"abc\x05d0\x05d1", L"\x202a"L"abc\x05d0\x05d1\x202c" },
+    { L"abc\x05d0\x05d1.jpg", L"\x202a"L"abc\x05d0\x05d1.jpg\x202c" },
+  };
+  for (unsigned int i = 0; i < arraysize(test_data); ++i) {
+    std::wstring input = test_data[i].raw_filename;
+    std::wstring expected =
+        l10n_util::GetDisplayStringInLTRDirectionality(&input);
+    if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT)
+      EXPECT_EQ(test_data[i].display_string, expected);
+    else
+      EXPECT_EQ(input, expected);
+  }
+}
+
 TEST_F(L10nUtilTest, GetTextDirection) {
   EXPECT_EQ(l10n_util::RIGHT_TO_LEFT, GetTextDirection("ar"));
   EXPECT_EQ(l10n_util::RIGHT_TO_LEFT, GetTextDirection("ar_EG"));
diff --git a/base/file_path.h b/base/file_path.h
index b608651..e1dbd22 100644
--- a/base/file_path.h
+++ b/base/file_path.h
@@ -61,7 +61,13 @@
 // |   FilePath log_file_path(kLogFileName);
 // |   [...]
 // | }
-
+//
+// WARNING: FilePaths should ALWAYS be displayed with LTR directionality, even
+// when the UI language is RTL. This means you always need to pass filepaths
+// through l10n_util::WrapPathWithLTRFormatting() before displaying it in the
+// RTL UI.
+//
+// This is a very common source of bugs, please try to keep this in mind.
 #ifndef BASE_FILE_PATH_H_
 #define BASE_FILE_PATH_H_
 
diff --git a/chrome/browser/views/download_item_view.cc b/chrome/browser/views/download_item_view.cc
index c704b67..9f4d310 100644
--- a/chrome/browser/views/download_item_view.cc
+++ b/chrome/browser/views/download_item_view.cc
@@ -303,9 +303,11 @@ DownloadItemView::DownloadItemView(DownloadItem* download,
       ElideString(extension, kFileNameMaxLength / 2, &extension);
 
     ElideString(rootname, kFileNameMaxLength - extension.length(), &rootname);
+    std::wstring filename = rootname + L"." + extension;
+    if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT)
+      l10n_util::WrapStringWithLTRFormatting(&filename);
     dangerous_download_label_ = new views::Label(
-        l10n_util::GetStringF(IDS_PROMPT_DANGEROUS_DOWNLOAD,
-                              rootname + L"." + extension));
+        l10n_util::GetStringF(IDS_PROMPT_DANGEROUS_DOWNLOAD, filename));
     dangerous_download_label_->SetMultiLine(true);
     dangerous_download_label_->SetHorizontalAlignment(
         views::Label::ALIGN_LEFT);
@@ -594,15 +596,18 @@ void DownloadItemView::Paint(gfx::Canvas* canvas) {
       filename = gfx::ElideFilename(download_->GetFileName(),
                                     font_, kTextWidth);
     } else {
-      std::wstring tmp_name =
-          l10n_util::GetStringF(IDS_DOWNLOAD_STATUS_OPENING,
-                                download_->GetFileName().ToWStringHack());
-#if defined(OS_WIN)
-      FilePath filepath(tmp_name);
-#else
-      FilePath filepath(base::SysWideToNativeMB(tmp_name));
-#endif
-      filename = gfx::ElideFilename(filepath, font_, kTextWidth);
+      // First, Calculate the download status opening string width.
+      std::wstring empty_string;
+      std::wstring status_string =
+          l10n_util::GetStringF(IDS_DOWNLOAD_STATUS_OPENING, empty_string);
+      int status_string_width = font_.GetStringWidth(status_string);
+      // Then, elide the file name.
+      std::wstring filename_string =
+          gfx::ElideFilename(download_->GetFileName(), font_,
+                             kTextWidth - status_string_width);
+      // Last, concat the whole string.
+      filename = l10n_util::GetStringF(IDS_DOWNLOAD_STATUS_OPENING,
+                                       filename_string);
     }
 
     int mirrored_x = MirroredXWithWidthInsideView(