RenderText: Allow strike-through line thickness to be customized.

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 dfac16495193edac4772265f9f6903c821d1d734..465d7e9b7c60a00ac28866d474d15b6452386afd 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -55,10 +55,10 @@ const SkColor kDefaultColor = SK_ColorBLACK;
 const SkColor kDefaultSelectionBackgroundColor = SK_ColorGRAY;
 
 // Fraction of the text size to lower a strike through below the baseline.

[cjgrant, 2017/06/30 19:04:25]

Should this be "raise the strike through above the baseline" and just adjust the
math to not need a negative in the constant?

Also, if my changes are okay, I can also document that this value is the center
of the strike-through line rather than the upper edge.

[msw, 2017/06/30 20:51:23]

That seems fine to me, just make sure you don't accidentally shift the underline
position by its thickness in the calculation on line 257. aside: you can also
hyphenate "strike-through" here (and elsewhere) if that's your preference.

[cjgrant, 2017/07/05 16:16:48]

I did some underline and strike before/after tests to make sure I'm not changing
the default positions/sizes of the lines.

-const SkScalar kStrikeThroughOffset = (-SK_Scalar1 * 6 / 21);
+const SkScalar kStrikeThroughOffset = (-SK_Scalar1 * 29 / 126);

[cjgrant, 2017/06/30 21:06:24]

Forgot to say, this relatively nasty-looking constant is simply (6/21 - 1/9), to
ensure I'm not moving the existing line when changing to a center-based offset.

[msw, 2017/06/30 23:01:32]

Acknowledged.

[cjgrant, 2017/07/05 16:16:48]

And, it's now actually correct. :|

 // Fraction of the text size to lower an underline below the baseline.
 const SkScalar kUnderlineOffset = (SK_Scalar1 / 9);
-// Fraction of the text size to use for a strike through or under-line.
+// Default fraction of the text size to use for a strike through or under-line.

[msw, 2017/06/30 20:51:23]

optional nit: "underline" here to match most uses elsewhere.

[cjgrant, 2017/07/05 16:16:48]

Done.

 const SkScalar kLineThickness = (SK_Scalar1 / 18);

[msw, 2017/06/30 20:51:23]

optional nit: kLineThicknessFactor

[cjgrant, 2017/07/05 16:16:48]

Done.

 
 // Invalid value of baseline.  Assigning this value to |baseline_| causes
@@ -188,15 +188,10 @@ void RestoreBreakList(RenderText* render_text, BreakList<T>* break_list) {
 
 namespace internal {
 
-// Value of |underline_thickness_| that indicates that underline metrics have
-// not been set explicitly.
-const SkScalar kUnderlineMetricsNotSet = -1.0f;

[cjgrant, 2017/06/30 19:04:25]

I don't see that underscore overrides are actually used anywhere.  If they are,
I need to figure out how to get codesearch to show that.

[msw, 2017/06/30 20:51:23]

Afaict, you're right; thanks for removing this unused code.

-
 SkiaTextRenderer::SkiaTextRenderer(Canvas* canvas)
     : canvas_(canvas),
       canvas_skia_(canvas->sk_canvas()),
-      underline_thickness_(kUnderlineMetricsNotSet),
-      underline_position_(0.0f) {
+      strike_thickness_factor_(kLineThickness) {
   DCHECK(canvas_skia_);
   flags_.setTextEncoding(cc::PaintFlags::kGlyphID_TextEncoding);
   flags_.setStyle(cc::PaintFlags::kFill_Style);
@@ -234,10 +229,8 @@ void SkiaTextRenderer::SetShader(std::unique_ptr<cc::PaintShader> shader) {
   flags_.setShader(std::move(shader));
 }
 
-void SkiaTextRenderer::SetUnderlineMetrics(SkScalar thickness,
-                                           SkScalar position) {
-  underline_thickness_ = thickness;
-  underline_position_ = position;
+void SkiaTextRenderer::SetStrikeThicknessFactor(SkScalar factor) {
+  strike_thickness_factor_ = factor;
 }
 
 void SkiaTextRenderer::DrawPosText(const SkPoint* pos,
@@ -260,21 +253,17 @@ void SkiaTextRenderer::DrawDecorations(int x,
 
 void SkiaTextRenderer::DrawUnderline(int x, int y, int width) {
   SkScalar x_scalar = SkIntToScalar(x);
+  const SkScalar text_size = flags_.getTextSize();
   SkRect r = SkRect::MakeLTRB(
-      x_scalar, y + underline_position_, x_scalar + width,
-      y + underline_position_ + underline_thickness_);
-  if (underline_thickness_ == kUnderlineMetricsNotSet) {
-    const SkScalar text_size = flags_.getTextSize();
-    r.fTop = text_size * kUnderlineOffset + y;
-    r.fBottom = r.fTop + text_size * kLineThickness;
-  }
+      x_scalar, y + text_size * kUnderlineOffset, x_scalar + width,
+      y + (text_size * (kUnderlineOffset + kLineThickness)));
   canvas_skia_->drawRect(r, flags_);
 }
 
 void SkiaTextRenderer::DrawStrike(int x, int y, int width) const {
   const SkScalar text_size = flags_.getTextSize();
-  const SkScalar height = text_size * kLineThickness;
-  const SkScalar offset = text_size * kStrikeThroughOffset + y;
+  const SkScalar height = text_size * strike_thickness_factor_;
+  const SkScalar offset = text_size * kStrikeThroughOffset - height / 2 + y;
   SkScalar x_scalar = SkIntToScalar(x);
   const SkRect r =
       SkRect::MakeLTRB(x_scalar, offset, x_scalar + width, offset + height);
@@ -782,6 +771,7 @@ void RenderText::Draw(Canvas* canvas) {
 
   if (!text().empty()) {
     internal::SkiaTextRenderer renderer(canvas);
+    renderer.SetStrikeThicknessFactor(strike_thickness_factor_);

[cjgrant, 2017/06/30 19:04:25]

I must be doing this wrong, but I don't see a better approach.  What's the best
way to feed a RenderText setting through to SkiaTextRenderer?

[msw, 2017/06/30 20:51:23]

I would suggest removing SkiaTextRenderer::SetStrikeThicknessFactor *and* its
|strike_thickness_factor_| member, instead just pass the RenderText member's
value into the SkiaTextRenderer::DrawStrike functions. You'll need to plumb that
value through SkiaTextRenderer::DrawDecorations too; or my preference would be
to eliminate that helper and just inline its simple logic at the callsites.
Otherwise, this might be acceptable. Maybe asvitkine@ would have alternate
suggestions as the designer of SkiaTextRenderer, but he also doesn't actively
work in this area anymore.

[cjgrant, 2017/06/30 21:06:24]

I'd thought about plumbing through the call path, but doesn't that mean
supplying the value as an arg to DrawVisualText, through both the mac and
harfbuzz implementations, and down to the renderer?  I feel like we should just
configure the renderer here to save all the hops.  It's just hard for me to
fathom that this would be the first such setting that would warrant such
treatment.

I could also make a SkiaTextRendererParameters struct/class, housing only this
one property, and pass it in...  That'd look cleaner, and could accommodate
other properties.  

[msw, 2017/06/30 23:01:32]

You could add a simple accessor |RenderText::strike_thickness_factor() const {
return strike_thickness_factor_; }| that Mac/Harfbuzz subclasses could use,
that'd probably be my top preference (it seems more natural as a simple property
of the strike/underline, like the dimensions passed to DrawDecorations, not some
renderer configuration value). Otherwise, I prefer what you have now over
creating a params struct with one value.

[cjgrant, 2017/07/05 16:16:48]

I did your top-preference, and removed the DrawDecorations wrapper as well.

     DrawVisualText(&renderer);
   }
 
@@ -979,6 +969,10 @@ base::string16 RenderText::GetTextFromRange(const Range& range) const {
   return base::string16();
 }
 
+void RenderText::SetStrikeThicknessFactor(SkScalar factor) {

[msw, 2017/06/30 20:51:23]

nit: this could be a simple setter defined in the header: 
  void set_strike_thickness_factor(SkScalar f) { strike_thickness_factor_ = f; }
(ditto for the SkiaTextRenderer function, if you keep that around)

[cjgrant, 2017/07/05 16:16:48]

Done.

+  strike_thickness_factor_ = factor;
+}
+
 RenderText::RenderText()
     : horizontal_alignment_(base::i18n::IsRTL() ? ALIGN_RIGHT : ALIGN_LEFT),
       directionality_mode_(DIRECTIONALITY_FROM_TEXT),
@@ -1006,7 +1000,8 @@ RenderText::RenderText()
       subpixel_rendering_suppressed_(false),
       clip_to_display_rect_(true),
       baseline_(kInvalidBaseline),
-      cached_bounds_and_offset_valid_(false) {}
+      cached_bounds_and_offset_valid_(false),
+      strike_thickness_factor_(kLineThickness) {}
 
 SelectionModel RenderText::GetAdjacentSelectionModel(
     const SelectionModel& current,