Fix incorrect forcing of text directionality in canvas_skia.cc.

ui/gfx/canvas_skia.cc

Index: ui/gfx/canvas_skia.cc
===================================================================
--- ui/gfx/canvas_skia.cc	(revision 136986)
+++ ui/gfx/canvas_skia.cc	(working copy)
@@ -19,14 +19,38 @@
 
 namespace {
 
-// Based on |flags| and |text| content, returns whether text should be
-// rendered right-to-left.
-bool IsTextRTL(int flags, const string16& text) {
-  if (flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY)
+// If necessary, wraps |text| with RTL/LTR directionality characters based on
+// |flags| and |text| content.
+// Returns true if the text will be rendered right-to-left.
+bool AdjustStringDirection(int flags, string16* text) {
+  if (text->empty())
+    return false;
+
+  // First, if RTL was forced, simply apply it.
+  if (flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY) {
+    base::i18n::WrapStringWithRTLFormatting(text);
     return true;
-  if (flags & gfx::Canvas::FORCE_LTR_DIRECTIONALITY)
+  }
+
+  // If RTL wasn't forced and the string does not have strong RTL chars, it
+  // will be rendered LTR already, so no need to do anything special.
+  if (!base::i18n::StringContainsStrongRTLChars(*text))
     return false;
-  return base::i18n::IsRTL() && base::i18n::StringContainsStrongRTLChars(text);
+
+  // If LTR is forced for a string with strong RTL characters, apply it here.
+  if (flags & gfx::Canvas::FORCE_LTR_DIRECTIONALITY) {
+    base::i18n::WrapStringWithLTRFormatting(text);

[xji, 2012/05/15 19:42:07]

I think in our current Uniscribe implementation of RenderText, text is assumed
to be in left-to-right directionality, no matter the UI is a RTL UI or LTR UI.
In which case, we do not need to add bidi mark to force directionality here.

But we probably want to change the RenderText Win behavior to set base
directionality as RTL if UI is RTL, and base directionality as LTR if UI is LTL,
in which case, we will need to wrap the text only when IsRTL() is true.

Either case, I feel better to wrap the string when IsRTL() is true to reduce
empty squares situation.

Or ultimately, to reset RenderText's base directionalty based on the each text
itself.

BTW, while testing the JS alert display in RTL UI, I think there might be
another regression. The message got truncated. It might be a regression after
r124736. If you run the test file, are the messages got truncated with your CL
(which force correct directionality)?

+    return false;
+  }
+
+  // Finally, if a direction wasn't forced but the UI language is RTL and there
+  // were strong RTL characters, ensure RTL is applied.
+  if (base::i18n::IsRTL()) {
+    base::i18n::WrapStringWithRTLFormatting(text);
+    return true;
+  }
+
+  return false;

[xji, 2012/05/15 19:42:07]

I'd prefer the old 'if'/'else' structure.

 }
 
 // Checks each pixel immediately adjacent to the given pixel in the bitmap. If
@@ -250,8 +274,7 @@
   string16 adjusted_text = text;
 
 #if defined(OS_WIN)
-  if (IsTextRTL(flags, adjusted_text))
-    base::i18n::AdjustStringForLocaleDirection(&adjusted_text);
+  AdjustStringDirection(flags, &adjusted_text);
 #endif
 
   scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
@@ -392,9 +415,7 @@
 
   scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
   string16 clipped_text = text;
-  const bool is_rtl = IsTextRTL(flags, text);
-  if (is_rtl)
-    base::i18n::AdjustStringForLocaleDirection(&clipped_text);
+  const bool is_rtl = AdjustStringDirection(flags, &clipped_text);
 
   switch (truncate_mode) {
     case TruncateFadeTail: