Fix incorrect forcing of text directionality in canvas_skia.cc.

ui/gfx/canvas_skia.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/canvas.h"
 
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "ui/base/range/range.h"
 #include "ui/base/text/text_elider.h"
 #include "ui/gfx/font.h"
 #include "ui/gfx/font_list.h"
 #include "ui/gfx/insets.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/shadow_value.h"
 #include "ui/gfx/skia_util.h"
 
 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
 // any of them are not the halo color, returns true. This defines the halo of
 // pixels that will appear around the text. Note that we have to check each
 // pixel against both the halo color and transparent since |DrawStringWithHalo|
 // will modify the bitmap as it goes, and cleared pixels shouldn't count as
 // changed.
 bool PixelShouldGetHalo(const SkBitmap& bitmap,
                         int x, int y,
@@ skipping 203 matching lines @@
   gfx::Rect clip_rect(text_bounds);
   clip_rect.Inset(ShadowValue::GetMargin(shadows));
 
   canvas_->save(SkCanvas::kClip_SaveFlag);
   ClipRect(clip_rect);
 
   gfx::Rect rect(text_bounds);
   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());
   render_text->SetTextShadows(shadows);
 
   if (flags & MULTI_LINE) {
     ui::WordWrapBehavior wrap_behavior = ui::IGNORE_LONG_WORDS;
     if (flags & CHARACTER_BREAK)
       wrap_behavior = ui::WRAP_LONG_WORDS;
     else if (!(flags & NO_ELLIPSIS))
@@ skipping 120 matching lines @@
 
   // If the whole string fits in the destination then just draw it directly.
   if (GetStringWidth(text, font) <= display_rect.width()) {
     DrawStringInt(text, font, color, display_rect.x(), display_rect.y(),
                   display_rect.width(), display_rect.height(), flags);
     return;
   }
 
   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:
       render_text->set_fade_tail(true);
       if (is_rtl)
         flags |= TEXT_ALIGN_RIGHT;
       break;
     case TruncateFadeHead:
       render_text->set_fade_head(true);
       if (!is_rtl)
@@ skipping 30 matching lines @@
   render_text->SetDisplayRect(rect);
 
   canvas_->save(SkCanvas::kClip_SaveFlag);
   ClipRect(display_rect);
   render_text->Draw(this);
   canvas_->restore();
 }
 #endif
 
 }  // namespace gfx