More or less implement RenderTextHarfBuzz

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
index 645573865c3d075a7389a54d3c0dc5277658fdb7..e835c4f2403e2074d58168de501df0b45652f38f 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -17,6 +17,8 @@
 #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"
@@ -218,17 +220,14 @@ 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());
   }
 }
 
@@ -319,6 +318,7 @@ 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) {
@@ -339,25 +339,29 @@ void SkiaTextRenderer::DiagonalStrike::Draw() {
       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;
   }
 }
@@ -394,11 +398,23 @@ 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)