Cleanup unnecessary code in canvas_skia

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/gfx/font_list.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/safe_integer_conversions.h"
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/shadow_value.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 
 namespace gfx {
 
 namespace {
 
-#if defined(OS_WIN)
-// 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, base::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 & Canvas::FORCE_LTR_DIRECTIONALITY))
-    return false;
-
-  // If RTL is forced, apply it to the string.
-  if (flags & Canvas::FORCE_RTL_DIRECTIONALITY) {
-    base::i18n::WrapStringWithRTLFormatting(text);
-    return true;
-  }
-
-  // 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;
-  }
-
-  // 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;
-}
-#endif  // defined(OS_WIN)
-
 // 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
 // |DrawStringRectWithHalo| will modify the bitmap as it goes, and cleared
 // pixels shouldn't count as changed.
 bool PixelShouldGetHalo(const SkBitmap& bitmap,
                         int x, int y,
                         SkColor halo_color) {
   if (x > 0 &&
@@ skipping 55 matching lines @@
                       RenderText* render_text) {
   render_text->SetFontList(font_list);
   render_text->SetText(text);
   render_text->SetCursorEnabled(false);
   render_text->SetDisplayRect(rect);
 
   // Set the text alignment explicitly based on the directionality of the UI,
   // if not specified.
   if (!(flags & (Canvas::TEXT_ALIGN_CENTER |
                  Canvas::TEXT_ALIGN_RIGHT |
-                 Canvas::TEXT_ALIGN_LEFT))) {
+                 Canvas::TEXT_ALIGN_LEFT |
+                 Canvas::TEXT_ALIGN_TO_HEAD))) {
     flags |= Canvas::DefaultCanvasTextAlignment();
   }
 
-  if (flags & Canvas::TEXT_ALIGN_RIGHT)
+  if (flags & Canvas::TEXT_ALIGN_TO_HEAD)
+    render_text->SetHorizontalAlignment(ALIGN_TO_HEAD);
+  else if (flags & Canvas::TEXT_ALIGN_RIGHT)
     render_text->SetHorizontalAlignment(ALIGN_RIGHT);
   else if (flags & Canvas::TEXT_ALIGN_CENTER)
     render_text->SetHorizontalAlignment(ALIGN_CENTER);
   else
     render_text->SetHorizontalAlignment(ALIGN_LEFT);
 
   render_text->set_subpixel_rendering_suppressed(
       (flags & Canvas::NO_SUBPIXEL_RENDERING) != 0);
 
   render_text->SetColor(color);
   const int font_style = font_list.GetFontStyle();
   render_text->SetStyle(BOLD, (font_style & Font::BOLD) != 0);
   render_text->SetStyle(ITALIC, (font_style & Font::ITALIC) != 0);
   render_text->SetStyle(UNDERLINE, (font_style & Font::UNDERLINE) != 0);
 }
 
 }  // namespace
 
 // static
 void Canvas::SizeStringFloat(const base::string16& text,
                              const FontList& font_list,
                              float* width, float* height,
                              int line_height,
                              int flags) {
   DCHECK_GE(*width, 0);
   DCHECK_GE(*height, 0);
 
-  base::string16 adjusted_text = text;
-#if defined(OS_WIN)
-  AdjustStringDirection(flags, &adjusted_text);
-#endif
-
   if ((flags & MULTI_LINE) && *width != 0) {
     WordWrapBehavior wrap_behavior = TRUNCATE_LONG_WORDS;
     if (flags & CHARACTER_BREAK)
       wrap_behavior = WRAP_LONG_WORDS;
     else if (!(flags & NO_ELLIPSIS))
       wrap_behavior = ELIDE_LONG_WORDS;
 
     std::vector<base::string16> strings;
-    ElideRectangleText(adjusted_text, font_list,
+    ElideRectangleText(text, font_list,

[msw, 2015/02/25 17:05:20]

nit: update line wrapping.

[oshima, 2015/03/04 08:01:50]

Done.

                        *width, INT_MAX,
                        wrap_behavior, &strings);
     Rect rect(ClampToInt(*width), INT_MAX);
     scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
     UpdateRenderText(rect, base::string16(), font_list, flags, 0,
                      render_text.get());
 
     float h = 0;
     float w = 0;
     for (size_t i = 0; i < strings.size(); ++i) {
       StripAcceleratorChars(flags, &strings[i]);
       render_text->SetText(strings[i]);
       const SizeF& string_size = render_text->GetStringSizeF();
       w = std::max(w, string_size.width());
       h += (i > 0 && line_height > 0) ?
                std::max(static_cast<float>(line_height), string_size.height())
                    : string_size.height();
     }
     *width = w;
     *height = h;
   } else {
-    // If the string is too long, the call by |RenderTextWin| to |ScriptShape()|
-    // will inexplicably fail with result E_INVALIDARG. Guard against this.
-    const size_t kMaxRenderTextLength = 5000;

[msw, 2015/03/04 17:09:36]

You should update the comment in text_elider.cc referencing this value.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/text_elider...

-    if (adjusted_text.length() >= kMaxRenderTextLength) {

[msw, 2015/02/25 17:05:20]

Did you test some very long strings here? (up to 10k chars)

[oshima, 2015/03/04 08:01:50]

RenderTextWin no longer exists, so E_INVALIDARG can't happen, can it? 

or do you want me to add test to canvas_unittest? (There is
RenderTexTest.StringSizeLongStrings for RT itself)

[msw, 2015/03/04 17:09:36]

This was unconditionally clamping at 5k chars, even for RTHB, but now this will
pass a string of unbounded length to RenderText. RTHB should truncate at
kMaxTextLength = 10000, and StringSizeLongStrings seems to test this
functionality sufficiently, so I think this will be okay. I just thought it
might be worth testing in practice to ensure we don't hit any crash/hang/jank,
and I wonder if the value of determining the precise size of a string longer
than 5k-10k chars here outwieghs any performance degradation that might occur.

[oshima, 2015/03/04 20:17:42]

In the omibox, which uses TextField that uses RT directly, I tested the long
text
(that I can see elide "..." in omnibox) and I didn't see big performance issue.

-      *width = static_cast<float>(
-          font_list.GetExpectedTextWidth(adjusted_text.length()));
-      *height = static_cast<float>(font_list.GetHeight());
-    } else {
-      scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
-      Rect rect(ClampToInt(*width), ClampToInt(*height));
-      StripAcceleratorChars(flags, &adjusted_text);
-      UpdateRenderText(rect, adjusted_text, font_list, flags, 0,
-                       render_text.get());
-      const SizeF& string_size = render_text->GetStringSizeF();
-      *width = string_size.width();
-      *height = string_size.height();
-    }
+    scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
+    Rect rect(ClampToInt(*width), ClampToInt(*height));
+    base::string16 adjusted_text = text;
+    StripAcceleratorChars(flags, &adjusted_text);
+    UpdateRenderText(rect, adjusted_text, font_list, flags, 0,
+                     render_text.get());
+    const SizeF& string_size = render_text->GetStringSizeF();
+    *width = string_size.width();
+    *height = string_size.height();
   }
 }
 
 void Canvas::DrawStringRectWithShadows(const base::string16& text,
                                        const FontList& font_list,
                                        SkColor color,
                                        const Rect& text_bounds,
                                        int line_height,
                                        int flags,
                                        const ShadowValues& shadows) {
   if (!IntersectsClipRect(text_bounds))
     return;
 
   Rect clip_rect(text_bounds);
   clip_rect.Inset(ShadowValue::GetMargin(shadows));
 
   canvas_->save();
   ClipRect(clip_rect);
 
   Rect rect(text_bounds);
-  base::string16 adjusted_text = text;
-
-#if defined(OS_WIN)
-  AdjustStringDirection(flags, &adjusted_text);
-#endif
 
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
   render_text->set_shadows(shadows);
 
   if (flags & MULTI_LINE) {
     WordWrapBehavior wrap_behavior = IGNORE_LONG_WORDS;
     if (flags & CHARACTER_BREAK)
       wrap_behavior = WRAP_LONG_WORDS;
     else if (!(flags & NO_ELLIPSIS))
       wrap_behavior = ELIDE_LONG_WORDS;
 
     std::vector<base::string16> strings;
-    ElideRectangleText(adjusted_text, font_list,
+    ElideRectangleText(text, font_list,
                        static_cast<float>(text_bounds.width()),
                        text_bounds.height(), wrap_behavior, &strings);
 
     for (size_t i = 0; i < strings.size(); i++) {
       Range range = StripAcceleratorChars(flags, &strings[i]);
       UpdateRenderText(rect, strings[i], font_list, flags, color,
                        render_text.get());
       int line_padding = 0;
       if (line_height > 0)
         line_padding = line_height - render_text->GetStringSize().height();
@@ skipping 11 matching lines @@
 
       rect.set_height(line_height - line_padding);
 
       if (range.IsValid())
         render_text->ApplyStyle(UNDERLINE, true, range);
       render_text->SetDisplayRect(rect);
       render_text->Draw(this);
       rect += Vector2d(0, line_height);
     }
   } else {
+    base::string16 adjusted_text = text;
     Range range = StripAcceleratorChars(flags, &adjusted_text);
     bool elide_text = ((flags & NO_ELLIPSIS) == 0);
 
 #if defined(OS_LINUX)
     // On Linux, eliding really means fading the end of the string. But only
     // for LTR text. RTL text is still elided (on the left) with "...".
     if (elide_text) {
       render_text->SetText(adjusted_text);
       if (render_text->GetDisplayTextDirection() == base::i18n::LEFT_TO_RIGHT) {
         render_text->SetElideBehavior(FADE_TAIL);
@@ skipping 69 matching lines @@
 void Canvas::DrawFadedString(const base::string16& text,
                              const FontList& font_list,
                              SkColor color,
                              const Rect& display_rect,
                              int flags) {
   // If the whole string fits in the destination then just draw it directly.
   if (GetStringWidth(text, font_list) <= display_rect.width()) {
     DrawStringRectWithFlags(text, font_list, color, display_rect, flags);
     return;
   }
-
-  // Align with forced content directionality, overriding alignment flags.
-  if (flags & FORCE_RTL_DIRECTIONALITY) {
-    flags &= ~(TEXT_ALIGN_CENTER | TEXT_ALIGN_LEFT);
-    flags |= TEXT_ALIGN_RIGHT;
-  } else if (flags & FORCE_LTR_DIRECTIONALITY) {
-    flags &= ~(TEXT_ALIGN_CENTER | TEXT_ALIGN_RIGHT);
-    flags |= TEXT_ALIGN_LEFT;
-  } else if (!(flags & TEXT_ALIGN_LEFT) && !(flags & TEXT_ALIGN_RIGHT)) {
-    // Also align with content directionality instead of fading both ends.
+  if (!(flags &
+        (TEXT_ALIGN_LEFT | TEXT_ALIGN_RIGHT | TEXT_ALIGN_TO_HEAD))) {
+    // align with content directionality instead of fading both ends.

[msw, 2015/02/25 17:05:20]

nit: Align

[oshima, 2015/03/04 08:01:50]

Done.

     flags &= ~TEXT_ALIGN_CENTER;

[msw, 2015/02/25 17:05:20]

nit: refactor the code to unconditionally prevent center fading:
  // Align with content directionality instead of fading both ends.
  flags &= ~TEXT_ALIGN_CENTER;
  if (!(flags & (TEXT_ALIGN_LEFT | TEXT_ALIGN_RIGHT)))
    flags |= TEXT_ALIGN_TO_HEAD;

[oshima, 2015/03/04 08:01:50]

Done.

-    const bool is_rtl = base::i18n::GetFirstStrongCharacterDirection(text) ==
-                        base::i18n::RIGHT_TO_LEFT;
-    flags |= is_rtl ? TEXT_ALIGN_RIGHT : TEXT_ALIGN_LEFT;
+    flags |= TEXT_ALIGN_TO_HEAD;
   }
   flags |= NO_ELLIPSIS;
 
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
   Rect rect = display_rect;
   UpdateRenderText(rect, text, font_list, flags, color, render_text.get());
   render_text->SetElideBehavior(FADE_TAIL);
 
   canvas_->save();
   ClipRect(display_rect);
   render_text->Draw(this);
   canvas_->restore();
 }
 
 }  // namespace gfx