Check string and UI direction in GetDisplayStringInLTRDirectionality, etc.

base/i18n/rtl.cc

Index: base/i18n/rtl.cc
diff --git a/base/i18n/rtl.cc b/base/i18n/rtl.cc
index 29a2a88d555e6d2164b6f7acf23eba6631fc5890..9e71928e4116d6b8aa9371e4990a2e9b0bcdeede 100644
--- a/base/i18n/rtl.cc
+++ b/base/i18n/rtl.cc
@@ -319,7 +319,9 @@ void WrapPathWithLTRFormatting(const FilePath& path,
 }
 
 string16 GetDisplayStringInLTRDirectionality(const string16& text) {
-  if (!IsRTL())
+  // Always wrap the string in RTL UI (it may be appended to RTL string).
+  // Also wrap strings with an RTL first strong character direction in LTR UI.
+  if (!IsRTL() && GetFirstStrongCharacterDirection(text) == LEFT_TO_RIGHT)

[xji, 2012/09/10 18:07:26]

why RTL locale is specially handled (always wrap in LRE/PDF)?

for rtl locale (in other cases), are we still displaying text in RTL
directionality or we are also using first-strong-character's directionality?

[msw, 2012/09/10 18:37:31]

I don't know about Mac/GTK, but Win and CrOs use first-strong-char's dir.
I left the check to not regress additional cases (perhaps in Mac/GTK); also:

In RTL UI, |text| is more likely to be concatenated w/RTL text: "OPENING file".
So we must also wrap "file" if it starts with weak directionality characters.
Otherwise, concatenating "[weak text][ltr text]" to "[RTL TEXT]" would yield:
Logical text: "[RTL TEXT][weak text][ltr text]"
Display text: "[ltr text][weak text][RTL TEXT]" < wrong, fixed by wrapping.

Perhaps we must wrap if the first char isn't strong LTR? Or always wrap?
(just in case LTR UI uses RTL strings somewhere like the example above?)

[xji, 2012/09/10 22:27:57]

In English windows (windows xp), if a user does not install the RTL language
package, LRE/PDF characters render as empty square. So, we do not want always
wrapping.

[msw, 2012/09/10 22:46:13]

Cool, this still avoids adding them in LTR for LTR text.
( I cc'ed myself on that issue http://crbug.com/99300 )
I guess those users see RTL text as empty squares anyway.

[jungshik at Google, 2012/09/12 20:24:35]

Can we match the comment and if-statement? At first, I was confused because the
comment and conditions in if are opposite of each other. 

if (IsRTL() || GetFirst...() == RIGHT_TO_LEFT) {
   string16 text_mutable...
   ....
   return text_mutable;
}

return text;

}

BTW, I wonder about TODO comment in RenderText (from
https://chromiumcodereview.appspot.com/10693061 )

// Use the first strong character direction as the base text direction.
 601   // TODO(msw): Use the application text direction instead of LTR by
default?
Actually, I believe Arial/Times New Roman in XP do have Hebrew (and possibly
Arabic) covered. 

[msw, 2012/09/12 20:46:52]

Done, flipped condition & blocks.
See my comment above about the TODO.

     return text;
   string16 text_mutable(text);
   WrapStringWithLTRFormatting(&text_mutable);