Fix clipping RenderTextHarfBuzz text on Windows 8.

ui/gfx/render_text.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/render_text.h"
 
 #include <algorithm>
 #include <climits>
 
 #include "base/command_line.h"
@@ skipping 10 matching lines @@
 #include "ui/gfx/geometry/safe_integer_conversions.h"
 #include "ui/gfx/insets.h"
 #include "ui/gfx/render_text_harfbuzz.h"
 #include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/gfx/switches.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/gfx/utf16_indexing.h"
 
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#endif
+
 namespace gfx {
 
 namespace {
 
 // All chars are replaced by this char when the password style is set.
 // TODO(benrg): GTK uses the first of U+25CF, U+2022, U+2731, U+273A, '*'
 // that's available in the font (find_invisible_char() in gtkentry.c).
 const base::char16 kPasswordReplacementChar = '*';
 
 // Default color used for the text and cursor.
@@ skipping 121 matching lines @@
     FontRenderParams::Hinting params_hinting) {
   switch (params_hinting) {
     case FontRenderParams::HINTING_NONE:   return SkPaint::kNo_Hinting;
     case FontRenderParams::HINTING_SLIGHT: return SkPaint::kSlight_Hinting;
     case FontRenderParams::HINTING_MEDIUM: return SkPaint::kNormal_Hinting;
     case FontRenderParams::HINTING_FULL:   return SkPaint::kFull_Hinting;
   }
   return SkPaint::kNo_Hinting;
 }
 
+// Returns true if the RenderTextHarfBuzz implementation should be used.
+bool UseHarfBuzz() {
+  static const bool kUseHarfBuzz =
+      !CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableHarfBuzzRenderText);
+  return kUseHarfBuzz;
+}
+
+// Returns the padding left of centered text used to match legacy behavior.
+// TODO(msw|ckocagil): This may not be needed at all for RenderTextHarfBuzz.
+int GetLeftOfCenterPadding() {
+#if defined(OS_WIN)
+   if (UseHarfBuzz() && base::win::GetVersion() > base::win::VERSION_WIN7)
+     return 0;
+#endif
+   return 1;
+}
+
 }  // namespace
 
 namespace internal {
 
 // Value of |underline_thickness_| that indicates that underline metrics have
 // not been set explicitly.
 const SkScalar kUnderlineMetricsNotSet = -1.0f;
 
 SkiaTextRenderer::SkiaTextRenderer(Canvas* canvas)
     : canvas_(canvas),
@@ skipping 209 matching lines @@
   paint->setAutohinted(params.autohinter);
   paint->setHinting(FontRenderParamsHintingToSkPaintHinting(params.hinting));
 }
 
 }  // namespace internal
 
 RenderText::~RenderText() {
 }
 
 RenderText* RenderText::CreateInstance() {
-  return CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kDisableHarfBuzzRenderText) ? CreateNativeInstance() :
-      new RenderTextHarfBuzz;
+  return UseHarfBuzz() ? new RenderTextHarfBuzz() : CreateNativeInstance();
 }
 
 void RenderText::SetText(const base::string16& text) {
   DCHECK(!composition_range_.IsValid());
   if (text_ == text)
     return;
   text_ = text;
 
   // Adjust ranged styles and colors to accommodate a new text length.
   // Clear style ranges as they might break new text graphemes and apply
@@ skipping 651 matching lines @@
   Vector2d offset;
   HorizontalAlignment horizontal_alignment = GetCurrentHorizontalAlignment();
   if (horizontal_alignment != ALIGN_LEFT) {
 #if defined(OS_WIN)
     const int width = lines_[line_number].size.width() +
         (cursor_enabled_ ? 1 : 0);
 #else
     const int width = GetContentWidth();
 #endif
     offset.set_x(display_rect().width() - width);
-    // Put any extra margin pixel on the left to match legacy behavior.
+    // Put any extra margin pixel on the left to match some legacy behaviors.
+    static const int kLeftOfCenterPadding = GetLeftOfCenterPadding();
     if (horizontal_alignment == ALIGN_CENTER)
-      offset.set_x((offset.x() + 1) / 2);
+      offset.set_x((offset.x() + kLeftOfCenterPadding) / 2);

[Peter Kasting, 2014/11/06 23:14:47]

I don't understand why the previous code was causing problems assuming |width|
is at least as large at display_rect().width() and display_rect().width()
actually contains the full extent of the text.

The only difference between the old code and the new is that, if |width| has an
odd number of extra pixels beyond what's necessary for diaply_rect(), the old
code puts the extra pixel on the left, and the new on the right.  But in neither
case should we actually be truncating something visible.  That should only
happen if one of the two conditions I mention above is violated.  And if those
are violated, it seems like the right fix is elsewhere rather than here.

Maybe I just don't understand what the code is doing.  Where is the logic flaw
above?

[msw, 2014/11/06 23:48:44]

You're right, width+1 in RenderTextHarfBuzz::GetStringSizeF works.
That's a better temporary hack to use until we can dig deeper.

   }
 
   // Vertically center the text.
   if (multiline_) {
     const int text_height = lines_.back().preceding_heights +
         lines_.back().size.height();
     offset.set_y((display_rect_.height() - text_height) / 2);
   } else {
     offset.set_y(GetBaseline() - GetLayoutTextBaseline());
   }
@@ skipping 310 matching lines @@
   SetDisplayOffset(display_offset_.x() + delta_x);
 }
 
 void RenderText::DrawSelection(Canvas* canvas) {
   const std::vector<Rect> sel = GetSubstringBounds(selection());
   for (std::vector<Rect>::const_iterator i = sel.begin(); i < sel.end(); ++i)
     canvas->FillRect(*i, selection_background_focused_color_);
 }
 
 }  // namespace gfx