More or less implement RenderTextHarfBuzz

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/i18n/break_iterator.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/icu/source/common/unicode/rbbi.h"
 #include "third_party/icu/source/common/unicode/utf16.h"
 #include "third_party/skia/include/core/SkTypeface.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
 #include "ui/gfx/canvas.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/text_constants.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/gfx/utf16_indexing.h"
 
 namespace gfx {
 
 namespace {
 
@@ skipping 181 matching lines @@
 }
 
 void SkiaTextRenderer::SetTextSize(SkScalar size) {
   paint_.setTextSize(size);
 }
 
 void SkiaTextRenderer::SetFontFamilyWithStyle(const std::string& family,
                                               int style) {
   DCHECK(!family.empty());
 
-  SkTypeface::Style skia_style = ConvertFontStyleToSkiaTypefaceStyle(style);
-  skia::RefPtr<SkTypeface> typeface =
-      skia::AdoptRef(SkTypeface::CreateFromName(family.c_str(), skia_style));
+  skia::RefPtr<SkTypeface> typeface = CreateSkiaTypeface(family.c_str(), style);
   if (typeface) {
     // |paint_| adds its own ref. So don't |release()| it from the ref ptr here.
     SetTypeface(typeface.get());
 
     // Enable fake bold text if bold style is needed but new typeface does not
     // have it.
-    paint_.setFakeBoldText((skia_style & SkTypeface::kBold) &&
-                           !typeface->isBold());
+    paint_.setFakeBoldText((style & gfx::Font::BOLD) && !typeface->isBold());
   }
 }
 
 void SkiaTextRenderer::SetForegroundColor(SkColor foreground) {
   paint_.setColor(foreground);
 }
 
 void SkiaTextRenderer::SetShader(SkShader* shader, const Rect& bounds) {
   bounds_ = RectToSkRect(bounds);
   paint_.setShader(shader);
@@ skipping 70 matching lines @@
   const SkScalar height = SkScalarMul(text_size, kLineThickness);
   const SkScalar offset = SkScalarMulAdd(text_size, kStrikeThroughOffset, y);
   const SkRect r = SkRect::MakeLTRB(x, offset, x + width, offset + height);
   canvas_skia_->drawRect(r, paint_);
 }
 
 SkiaTextRenderer::DiagonalStrike::DiagonalStrike(Canvas* canvas,
                                                  Point start,
                                                  const SkPaint& paint)
     : canvas_(canvas),
+      matrix_(canvas->sk_canvas()->getTotalMatrix()),

[msw, 2014/04/29 06:24:45]

nit: can this just be calculated in DiagonalStrike::Draw?

[ckocagil, 2014/05/01 22:02:01]

No, because for each text run we do a matrix translation in DrawVisualText but
we need to remember the initial matrix so that the diagonal strike starts from
the initial run, not the last run.

[msw, 2014/05/02 05:08:21]

This wasn't previously necessary, is it related to the tangential ScopedCanvas
changes below, and hopefully avoidable in this CL?

[ckocagil, 2014/05/06 03:38:40]

Actually these changes are intended. When we want to offset the glyphs in
RenderTextWin by the line position, baseline, previous runs on the line, etc. we
create a second positions array that is translated. Why not just translate the
canvas, draw the glyphs as if we were drawing at (0,0) and then restore the
canvas? This is faster, simpler, and exactly what we're doing in this patch.

[msw, 2014/05/09 22:55:18]

Ah, so we could apply canvas translations in RenderTextWin and RenderTextPango
DrawVisualText and then not pass a start point here, like in RenderTextHarfBuzz?
Does that also simplify the use of internal::SkiaTextRenderer to actually paint
the text runs? Please update Win/Pango similarly in a separate CL before or
after this one, to have more shared and simplified behavior.

[ckocagil, 2014/05/12 09:53:29]

It doesn't simplify SkiaTextRenderer. Sure, I will do this for RenderTextWin
too.

       start_(start),
       paint_(paint),
       total_length_(0) {
 }
 
 SkiaTextRenderer::DiagonalStrike::~DiagonalStrike() {
 }
 
 void SkiaTextRenderer::DiagonalStrike::AddPiece(int length, SkColor color) {
   pieces_.push_back(Piece(length, color));
   total_length_ += length;
 }
 
 void SkiaTextRenderer::DiagonalStrike::Draw() {
   const SkScalar text_size = paint_.getTextSize();
   const SkScalar offset = SkScalarMul(text_size, kDiagonalStrikeMarginOffset);
   const int thickness =
       SkScalarCeilToInt(SkScalarMul(text_size, kLineThickness) * 2);
   const int height = SkScalarCeilToInt(text_size - offset);
   const Point end = start_ + Vector2d(total_length_, -height);
+  const int clip_height = height + 2 * thickness;
 
   paint_.setAntiAlias(true);
   paint_.setStrokeWidth(thickness);
 
+  ScopedCanvas scoped_canvas(canvas_);

[msw, 2014/04/29 06:24:45]

Why this is needed over doing Canvas::Save/Restore?

[ckocagil, 2014/05/01 22:02:01]

This is just another way of doing Save/Restore, nothing special. I can change it
if you find it obscure.

[msw, 2014/05/02 05:08:21]

Reducing the amount of churn in this change is highly desirable.

[ckocagil, 2014/05/06 03:38:40]

Done, this is now a Save/Restore pair.

[msw, 2014/05/09 22:55:18]

It doesn't look like you updated the latest patch set as such, but I now like
the use of ScopedCanvas, so you can leave this change in or make it in a
separate CL, whichever you prefer.

+
+  SkCanvas* sk_canvas = canvas_->sk_canvas();
+  sk_canvas->setMatrix(matrix_);
+
   const bool clipped = pieces_.size() > 1;
   int x = start_.x();
   for (size_t i = 0; i < pieces_.size(); ++i) {
     paint_.setColor(pieces_[i].second);
 
     if (clipped) {
-      canvas_->Save();
-      canvas_->ClipRect(Rect(x, 0, pieces_[i].first, start_.y() + thickness));
+      sk_canvas->clipRect(RectToSkRect(
+          Rect(x, end.y() - thickness, pieces_[i].first, clip_height)),

[msw, 2014/05/09 22:55:18]

I think you'll need to still respect start_.y() for Win/Pango calls. Perhaps
this is more reason to land this part of the change (plus |matrix_|,
ScopedCanvas, and Win/Pango translation changes) before this HarfBuzz CL.

[ckocagil, 2014/05/12 09:53:29]

We're already respecting that value by using end.y() and clip_height.

+          SkRegion::kReplace_Op);
     }
 
     canvas_->DrawLine(start_, end, paint_);
 
-    if (clipped)
-      canvas_->Restore();
-
     x += pieces_[i].first;
   }
 }
 
 StyleIterator::StyleIterator(const BreakList<SkColor>& colors,
                              const std::vector<BreakList<bool> >& styles)
     : colors_(colors),
       styles_(styles) {
   color_ = colors_.breaks().begin();
   for (size_t i = 0; i < styles_.size(); ++i)
@@ skipping 16 matching lines @@
 }
 
 LineSegment::LineSegment() : run(0) {}
 
 LineSegment::~LineSegment() {}
 
 Line::Line() : preceding_heights(0), baseline(0) {}
 
 Line::~Line() {}
 
+skia::RefPtr<SkTypeface> CreateSkiaTypeface(const std::string& family,
+                                            int style) {
+  SkTypeface::Style skia_style = ConvertFontStyleToSkiaTypefaceStyle(style);
+  return skia::AdoptRef(SkTypeface::CreateFromName(family.c_str(), skia_style));
+}
+
 }  // namespace internal
 
 RenderText::~RenderText() {
 }
 
+RenderText* RenderText::CreateInstance() {
+  if (CommandLine::ForCurrentProcess()->HasSwitch("use-harfbuzz-rendertext"))

[msw, 2014/04/29 06:24:45]

Make "use-harfbuzz-rendertext" a proper ui/gfx/switches.h constant;
"enable-harfbuzz-rendertext" would be better. Add a
chrome/browser/about_flags.cc entry for the settings and add the corresponding
strings to ui/resources/ui_resources.grd.

[ckocagil, 2014/05/01 22:02:01]

Done. I added the strings to chrome/app/generated_resources.grd, since that
seems to be where most (all?) experiment strings are located.

[msw, 2014/05/02 05:08:21]

Good, the file I suggested for strings was wrong, you are right.

+    return new RenderTextHarfBuzz;
+  return 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.
   const size_t text_length = text_.length();
   colors_.SetMax(text_length);
   for (size_t style = 0; style < NUM_TEXT_STYLES; ++style)
@@ skipping 889 matching lines @@
   cursor_bounds_ += delta_offset;
 }
 
 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