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

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 <limits.h>
 
 #include <algorithm>
 #include <climits>
@@ skipping 36 matching lines @@
 namespace gfx {
 
 namespace {
 
 // Default color used for the text and cursor.
 const SkColor kDefaultColor = SK_ColorBLACK;
 
 // Default color used for drawing selection background.
 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
 // re-calculation of baseline.
 const int kInvalidBaseline = INT_MAX;
 
 int round(float value) {
   return static_cast<int>(floor(value + 0.5f));
 }
 
 // Given |font| and |display_width|, returns the width of the fade gradient.
@@ skipping 108 matching lines @@
           render_text->IndexOfAdjacentGrapheme(range.end(), CURSOR_FORWARD));
       break_list->ApplyValue(current_break->second, range);
     }
   }
 }
 
 }  // namespace
 
 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);
   flags_.setAntiAlias(true);
   flags_.setSubpixelText(true);
   flags_.setLCDRenderText(true);
   flags_.setHinting(cc::PaintFlags::kNormal_Hinting);
 }
 
 SkiaTextRenderer::~SkiaTextRenderer() {
@@ skipping 17 matching lines @@
 }
 
 void SkiaTextRenderer::SetForegroundColor(SkColor foreground) {
   flags_.setColor(foreground);
 }
 
 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,
                                    const uint16_t* glyphs,
                                    size_t glyph_count) {
   const size_t byte_length = glyph_count * sizeof(glyphs[0]);
   canvas_skia_->drawPosText(&glyphs[0], byte_length, &pos[0], flags_);
 }
 
 void SkiaTextRenderer::DrawDecorations(int x,
                                        int y,
                                        int width,
                                        bool underline,
                                        bool strike) {
   if (underline)
     DrawUnderline(x, y, width);
   if (strike)
     DrawStrike(x, y, width);
 }
 
 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);
   canvas_skia_->drawRect(r, flags_);
 }
 
 StyleIterator::StyleIterator(const BreakList<SkColor>& colors,
                              const BreakList<BaselineStyle>& baselines,
                              const BreakList<Font::Weight>& weights,
                              const std::vector<BreakList<bool>>& styles)
@@ skipping 487 matching lines @@
 
     canvas->Save();
     canvas->ClipRect(clip_rect);
   }
 
   if (!text().empty() && focused())
     DrawSelection(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);
   }
 
   if (clip_to_display_rect())
     canvas->Restore();
 }
 
 bool RenderText::IsValidLogicalIndex(size_t index) const {
   // Check that the index is at a valid code point (not mid-surrgate-pair) and
   // that it's not truncated from the display text (its glyph may be shown).
@@ skipping 177 matching lines @@
       left_rect->origin() + Vector2d(0, lines()[line_index].baseline);
   return true;
 }
 
 base::string16 RenderText::GetTextFromRange(const Range& range) const {
   if (range.IsValid() && range.GetMin() < text().length())
     return text().substr(range.GetMin(), range.length());
   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),
       text_direction_(base::i18n::UNKNOWN_DIRECTION),
       cursor_enabled_(true),
       selection_color_(kDefaultColor),
       selection_background_focused_color_(kDefaultSelectionBackgroundColor),
       focused_(false),
       composition_range_(Range::InvalidRange()),
       colors_(kDefaultColor),
       baselines_(NORMAL_BASELINE),
       weights_(Font::Weight::NORMAL),
       styles_(NUM_TEXT_STYLES),
       composition_and_selection_styles_applied_(false),
       obscured_(false),
       obscured_reveal_index_(-1),
       truncate_length_(0),
       elide_behavior_(NO_ELIDE),
       text_elided_(false),
       min_line_height_(0),
       multiline_(false),
       max_lines_(0),
       word_wrap_behavior_(IGNORE_LONG_WORDS),
       replace_newline_chars_with_symbols_(true),
       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,
     BreakType break_type,
     VisualCursorDirection direction) {
   EnsureLayout();
 
   if (break_type == LINE_BREAK || text().empty())
     return EdgeSelectionModel(direction);
   if (break_type == CHARACTER_BREAK)
@@ skipping 619 matching lines @@
 
   for (; range_max < length; ++range_max)
     if (iter.IsEndOfWord(range_max) || iter.IsStartOfWord(range_max))
       break;
 
   return range.is_reversed() ? Range(range_max, range_min)
                              : Range(range_min, range_max);
 }
 
 }  // namespace gfx