Add RenderText DirectionalityMode enum and support; etc.

ui/gfx/canvas_skia.cc

Index: ui/gfx/canvas_skia.cc
diff --git a/ui/gfx/canvas_skia.cc b/ui/gfx/canvas_skia.cc
index 29f6f773063167ce12b6431ea46899a9ba45345b..25327c95424b4fc3fb1e512ae154beb0d5205268 100644
--- a/ui/gfx/canvas_skia.cc
+++ b/ui/gfx/canvas_skia.cc
@@ -19,37 +19,22 @@
 
 namespace {
 
-// 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.
-// TODO(msw): Nix this, now that RenderTextWin supports directionality directly.
-bool AdjustStringDirection(int flags, string16* text) {
-  // TODO(msw): FORCE_LTR_DIRECTIONALITY does not work for RTL text now.
-
-  // If the string is empty or LTR was forced, simply return false since the
-  // default RenderText directionality is already LTR.
-  if (text->empty() || (flags & gfx::Canvas::FORCE_LTR_DIRECTIONALITY))
-    return false;
-
-  // If RTL is forced, apply it to the string.
-  if (flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY) {
-    base::i18n::WrapStringWithRTLFormatting(text);
-    return true;
-  }
+// If necessary, force RTL/LTR base directionality.
+// Returns true if the text will be rendered with an RTL base direction.
+bool AdjustStringDirection(int flags,
+                           const string16& text,
+                           gfx::RenderText* render_text) {
+  if (flags & gfx::Canvas::FORCE_LTR_DIRECTIONALITY)
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_LTR);
+  else if (flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY)
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL);
 
   // 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::StringContainsStrongRTLChars(*text)) {
-    base::i18n::WrapStringWithRTLFormatting(text);
-    return true;
-  }
+  if (base::i18n::IsRTL() && base::i18n::StringContainsStrongRTLChars(text))
+    render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL);

[xji, 2012/07/30 18:32:16]

this is DIRECTIONALITY_FROM_UI.

we probably should not do anything for this case and use render_text's default
value (either DIRECTIONALITY_FROM_UI, or DIRECTIONALTY_FROM_TEXT).

[msw, 2012/07/31 03:03:06]

Changed to DIRECTIONALITY_FROM_UI for now (to avoid any behavioral change),
since this is actually checking if the string contains any strong RTL
characters, not if the string *starts* with strong RTL characters.

We can follow up later if we want a behavior change to use the default,
DIRECTIONALITY_FROM_TEXT (which would make these strings that start with LTR but
contain RTL actually show as LTR).

[xji, 2012/07/31 23:03:25]

are we already introducing behavior changes in windows?
previously, in LTR UI, strings were always in LTR direction.
Now, their direction is based on first character's.

then, why we want to keep the behavior in RTL UI unchanged?
to be consistent, should the string's direction also based on its first
character's?

And from Aharon's comment in
http://code.google.com/p/chromium/issues/detail?id=134746#c13, looks like this
behavior change is ok.

[msw, 2012/08/01 17:30:10]

Done. I made the recommended change (which amounts to removing this block and
defaulting to FROM_TEXT). One case might be negatively effected (but seems ok):
An RTL string in RTL UI with gfx::Canvas::FORCE_LTR_DIRECTIONALITY will now be
LTR (this code previously ignored the forced override and used RTL anyway).

 
-  // In the default case, the string should be rendered as LTR. RenderText's
-  // default directionality is LTR, so the text doesn't need to be wrapped.
-  // Note that individual runs within the string may still be rendered RTL
-  // (which will be the case for RTL text under non-RTL locales, since under RTL
-  // locales it will be handled by the if statement above).
-  return false;
+  return render_text->GetTextDirection() == base::i18n::RIGHT_TO_LEFT;
 }
 
 // Checks each pixel immediately adjacent to the given pixel in the bitmap. If
@@ -203,8 +188,10 @@ void Canvas::SizeStringInt(const string16& text,
   flags = AdjustPlatformSpecificFlags(text, flags);
 
   string16 adjusted_text = text;
+  scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
+
 #if defined(OS_WIN)
-  AdjustStringDirection(flags, &adjusted_text);
+  AdjustStringDirection(flags, adjusted_text, render_text.get());
 #endif
 
   if ((flags & MULTI_LINE) && *width != 0) {
@@ -218,7 +205,6 @@ void Canvas::SizeStringInt(const string16& text,
     std::vector<string16> strings;
     ui::ElideRectangleText(adjusted_text, font, rect.width(), rect.height(),
                            wrap_behavior, &strings);
-    scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
     UpdateRenderText(rect, string16(), font, flags, 0, render_text.get());
 
     int h = 0;
@@ -240,7 +226,6 @@ void Canvas::SizeStringInt(const string16& text,
       *width = adjusted_text.length() * font.GetAverageCharacterWidth();
       *height = font.GetHeight();
     } else {
-      scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
       gfx::Rect rect(*width, *height);
       StripAcceleratorChars(flags, &adjusted_text);
       UpdateRenderText(rect, adjusted_text, font, flags, 0, render_text.get());
@@ -280,13 +265,13 @@ void Canvas::DrawStringWithShadows(const string16& text,
   gfx::Rect rect(text_bounds);
   string16 adjusted_text = text;
 
-#if defined(OS_WIN)
-  AdjustStringDirection(flags, &adjusted_text);
-#endif
-
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
   render_text->SetTextShadows(shadows);
 
+#if defined(OS_WIN)
+  AdjustStringDirection(flags, adjusted_text, render_text.get());
+#endif
+
   if (flags & MULTI_LINE) {
     ui::WordWrapBehavior wrap_behavior = ui::IGNORE_LONG_WORDS;
     if (flags & CHARACTER_BREAK)
@@ -423,7 +408,8 @@ void Canvas::DrawFadeTruncatingString(
 
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
   string16 clipped_text = text;
-  const bool is_rtl = AdjustStringDirection(flags, &clipped_text);
+  const bool is_rtl =
+      AdjustStringDirection(flags, clipped_text, render_text.get());
 
   switch (truncate_mode) {
     case TruncateFadeTail: