Adjust text fade width and alpha

Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390
Committed: https://crrev.com/d6fa6e49521e7e1c08987d92fff36561453d1e34
Cr-Commit-Position: refs/heads/master@{#363188}

[Tomasz Moniuszko, 2015-12-02 12:58:59]

Description was changed from

==========
Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390
==========

to

==========
Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390
==========

[Tomasz Moniuszko, 2015-12-02 12:58:59]

tmoniuszko@opera.com changed reviewers:
+ pkasting@google.com

[Tomasz Moniuszko, 2015-12-02 12:59:50]

tmoniuszko@opera.com changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[Peter Kasting, 2015-12-03 05:29:25]

LGTM

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:101: const int average_character_width =
font_list.GetExpectedTextWidth(1);
While here: Instead of getting the width of 1 char and multiplying by 3, get the
width of 3 chars.  This returns the same value save for being slightly less
prone to rounding error.

Also use this where you want 4 char widths below.

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:105: return static_cast<int>(floor(gradient_width + 0.5));
Nit: For now, define a round() helper in this file and call that here and below
(see my comment there); I've separately sent a proposal to the C++11 arbiters to
allow std::round() from C++11.

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:147: const SkColor fade_color = SkColorSetA(color, alpha);
How about this:

  // In general, fade down to 0 alpha.  But when the available width is less
  // than four characters, linearly ramp up the fade target alpha to as high as
  // 20% at zero width.  This allows the user to see the last faded characters a
  // little better when there are only a few characters shown.
  const float width_fraction =
      text_rect.width() / static_cast<float>(font_list.GetExpectedTextWidth(4));
  const SkAlpha kAlphaAtZeroWidth = 51;
  const SkAlpha alpha = (width_fraction < 1) ?
      static_cast<SkAlpha>(round((1 - width_fraction) * kAlphaAtZeroWidth)) : 0;
  const SkColor fade_color = SkColorSetA(color, alpha);

[Tomasz Moniuszko, 2015-12-03 11:18:42]

Many thanks for review suggestions and accepting this patch!

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:101: const int average_character_width =
font_list.GetExpectedTextWidth(1);
On 2015/12/03 05:29:25, Peter Kasting wrote:
> While here: Instead of getting the width of 1 char and multiplying by 3, get
the
> width of 3 chars.  This returns the same value save for being slightly less
> prone to rounding error.
> 
> Also use this where you want 4 char widths below.

Done.

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:105: return static_cast<int>(floor(gradient_width + 0.5));
On 2015/12/03 05:29:25, Peter Kasting wrote:
> Nit: For now, define a round() helper in this file and call that here and
below
> (see my comment there); I've separately sent a proposal to the C++11 arbiters
to
> allow std::round() from C++11.

Done.

https://codereview.chromium.org/1493713002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:147: const SkColor fade_color = SkColorSetA(color, alpha);
On 2015/12/03 05:29:25, Peter Kasting wrote:
> How about this:
> 
>   // In general, fade down to 0 alpha.  But when the available width is less
>   // than four characters, linearly ramp up the fade target alpha to as high
as
>   // 20% at zero width.  This allows the user to see the last faded characters
a
>   // little better when there are only a few characters shown.
>   const float width_fraction =
>       text_rect.width() /
static_cast<float>(font_list.GetExpectedTextWidth(4));
>   const SkAlpha kAlphaAtZeroWidth = 51;
>   const SkAlpha alpha = (width_fraction < 1) ?
>       static_cast<SkAlpha>(round((1 - width_fraction) * kAlphaAtZeroWidth)) :
0;
>   const SkColor fade_color = SkColorSetA(color, alpha);

Done.

[Tomasz Moniuszko, 2015-12-03 11:19:05]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-12-03 11:19:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1493713002/#ps20001
(title: "Apply review suggestions")

[commit-bot: I haz the power, 2015-12-03 11:19:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1493713002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1493713002/20001

[commit-bot: I haz the power, 2015-12-03 11:29:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 11:29:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tomasz Moniuszko, 2015-12-03 11:47:16]

tmoniuszko@opera.com changed reviewers:
+ msw@chromium.org

[Tomasz Moniuszko, 2015-12-03 12:00:17]

On 2015/12/03 11:29:36, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

@msw: It seems that this patch still needs code owner acceptance. Could you
please have a look at it?

[msw, 2015-12-03 17:08:32]

lgtm with nits

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:103: // The .5 here is helpful if one of the characters is
a space.
nit: remove this line, it's not really applicable with the change from 2.5->3

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:109: return round(gradient_width + 0.5);
nit: shouldn't |round| add the 0.5 internally? (otherwise it's a floor)
(verify the other call below still works as intended if you change that)

[Peter Kasting, 2015-12-03 17:50:18]

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:96: int round(double value) {
Change this to taking a float, then follow my next comment.

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:105: const double average_character_width =
font_list.GetExpectedTextWidth(3);
Avoid unnecessary floating-point math, and fix the variable name, as follows:

  const int narrow_width = font_list.GetExpectedTextWidth(3);
  const int gradient_width = std::min(narrow_width, round(display_width / 3.f));
  DCHECK_GE(gradient_width, 0);
  return gradient_width;

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:109: return round(gradient_width + 0.5);
On 2015/12/03 17:08:32, msw wrote:
> nit: shouldn't |round| add the 0.5 internally? (otherwise it's a floor)
> (verify the other call below still works as intended if you change that)

Yes, round() should be adding the 0.5.  And the code down below is buggy without
round() doing that.

[Tomasz Moniuszko, 2015-12-04 11:08:18]

Thanks again for review and suggestions!

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:96: int round(double value) {
On 2015/12/03 17:50:17, Peter Kasting wrote:
> Change this to taking a float, then follow my next comment.

Done.

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:103: // The .5 here is helpful if one of the characters is
a space.
On 2015/12/03 17:08:32, msw wrote:
> nit: remove this line, it's not really applicable with the change from 2.5->3

Done.

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:105: const double average_character_width =
font_list.GetExpectedTextWidth(3);
On 2015/12/03 17:50:17, Peter Kasting wrote:
> Avoid unnecessary floating-point math, and fix the variable name, as follows:
> 
>   const int narrow_width = font_list.GetExpectedTextWidth(3);
>   const int gradient_width = std::min(narrow_width, round(display_width /
3.f));
>   DCHECK_GE(gradient_width, 0);
>   return gradient_width;

Done.

https://codereview.chromium.org/1493713002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:109: return round(gradient_width + 0.5);
On 2015/12/03 17:08:32, msw wrote:
> nit: shouldn't |round| add the 0.5 internally? (otherwise it's a floor)
> (verify the other call below still works as intended if you change that)

Done.

[Tomasz Moniuszko, 2015-12-04 11:08:34]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-12-04 11:08:35]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1493713002/#ps40001
(title: "Apply more review suggestions")

[commit-bot: I haz the power, 2015-12-04 11:08:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1493713002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1493713002/40001

[commit-bot: I haz the power, 2015-12-04 12:06:29]

Description was changed from

==========
Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390
==========

to

==========
Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390
==========

[commit-bot: I haz the power, 2015-12-04 12:06:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-04 12:07:16]

Description was changed from

==========
Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390
==========

to

==========
Adjust text fade width and alpha

Slightly increase fade gradient width.

Use 0 target alpha for wide texts. Linearly increase alpha for narrower
texts.

BUG=563390

Committed: https://crrev.com/d6fa6e49521e7e1c08987d92fff36561453d1e34
Cr-Commit-Position: refs/heads/master@{#363188}
==========

[commit-bot: I haz the power, 2015-12-04 12:07:17]

Patchset 3 (id:??) landed as
https://crrev.com/d6fa6e49521e7e1c08987d92fff36561453d1e34
Cr-Commit-Position: refs/heads/master@{#363188}