Improve toolbar button ink drop ripples at 1.25x

Improve toolbar button ink drop ripples at 1.25x

1. combining scaling and translation transforms on nested layers seems
   to create unfixable blurriness, so instead do all transforms on the
   painted layers.
2. adjust the size of the painted layers by a fractional amount to
   ensure pixel alignment of edges of layer (at start and end points of
   animation).

BUG=706228
Review-Url: https://codereview.chromium.org/2805813007
Cr-Commit-Position: refs/heads/master@{#464430}
Committed: https://chromium.googlesource.com/chromium/src/+/cfd215a5b6b3d1ab60398fc8c4a522b0d8ce611e

[Evan Stade, 2017-04-11 20:16:50]

Description was changed from

==========
wip - improve toolbar button ink drop ripples at 1.25x

1. combining scaling and translation transforms on nested layers seems
   to create unfixable blurriness, so instead do all transforms on the
   painted layers.
2. adjust the size of the painted layers by a fractional amount to
   ensure pixel alignment of edges of layer (at start and end points of
   animation).

BUG=706228
==========

to

==========
Improve toolbar button ink drop ripples at 1.25x

1. combining scaling and translation transforms on nested layers seems
   to create unfixable blurriness, so instead do all transforms on the
   painted layers.
2. adjust the size of the painted layers by a fractional amount to
   ensure pixel alignment of edges of layer (at start and end points of
   animation).

BUG=706228
==========

[Evan Stade, 2017-04-11 20:16:50]

estade@chromium.org changed reviewers:
+ bruthig@chromium.org

[Evan Stade, 2017-04-11 20:16:57]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 20:17:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 20:46:34]

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

[commit-bot: I haz the power, 2017-04-11 20:46:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-04-12 15:42:41]

(will look at test failures soon)

[Evan Stade, 2017-04-12 15:50:37]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 15:51:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 16:47:41]

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

[commit-bot: I haz the power, 2017-04-12 16:47:42]

Dry run: This issue passed the CQ dry run.

[bruthig, 2017-04-12 17:57:39]

Is it easy enough to add a test(s)?

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
File ui/views/animation/square_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
ui/views/animation/square_ink_drop_ripple.cc:441: gfx::SizeF size(desired_size);
nit: Consider renaming as |effective_size|.

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
ui/views/animation/square_ink_drop_ripple.cc:447: gfx::RectF
ripple_bounds((gfx::PointF(center_point_)), gfx::SizeF());
I'm assuming the 'extra' parenthesis are required...?

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
File ui/views/animation/square_ink_drop_ripple.h (right):

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
ui/views/animation/square_ink_drop_ripple.h:138: // of the given |desired_size|
and |corner_radius|. |desired_size| may be
nit: Consider 'The effective size may differ from |desired_size| at certain
scale...' instead of '|desired_size| may be...'

I find it weird to say |desired_size| may be modified when it's const.

[Evan Stade, 2017-04-12 23:48:28]

message: On 2017/04/12 17:57:39, bruthig wrote:
> Is it easy enough to add a test(s)?

No, it wasn't easy, but I added a test.

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
File ui/views/animation/square_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
ui/views/animation/square_ink_drop_ripple.cc:441: gfx::SizeF size(desired_size);
On 2017/04/12 17:57:39, bruthig wrote:
> nit: Consider renaming as |effective_size|.

it's used so often below I wanted to keep it shortish

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
ui/views/animation/square_ink_drop_ripple.cc:447: gfx::RectF
ripple_bounds((gfx::PointF(center_point_)), gfx::SizeF());
On 2017/04/12 17:57:39, bruthig wrote:
> I'm assuming the 'extra' parenthesis are required...?

yes. disambiguates a function call or some such.

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
File ui/views/animation/square_ink_drop_ripple.h (right):

https://codereview.chromium.org/2805813007/diff/60001/ui/views/animation/squa...
ui/views/animation/square_ink_drop_ripple.h:138: // of the given |desired_size|
and |corner_radius|. |desired_size| may be
On 2017/04/12 17:57:39, bruthig wrote:
> nit: Consider 'The effective size may differ from |desired_size| at certain
> scale...' instead of '|desired_size| may be...'
> 
> I find it weird to say |desired_size| may be modified when it's const.

I will apply this suggestion

[Evan Stade, 2017-04-12 23:48:45]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2017-04-12 23:49:41]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 23:49:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 01:27:59]

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

[commit-bot: I haz the power, 2017-04-13 01:28:00]

Dry run: This issue passed the CQ dry run.

[bruthig, 2017-04-13 15:59:21]

On 2017/04/12 23:48:28, Evan Stade wrote:
> message: On 2017/04/12 17:57:39, bruthig wrote:
> > Is it easy enough to add a test(s)?
> 
> No, it wasn't easy, but I added a test.

Thx!

As a discussion point though, I suspect that a WidgetTest fixture runs at least
1 order of magnitude slower than a testing::Test fixture and I feel like using a
WidgetTest simply to inject a device scale factor float is somewhat overkill.

If you're interested in re-working the test to not use a Wdiget I'd be happy to
discuss offline but I won't require it.

LGTM

[Evan Stade, 2017-04-13 16:12:43]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-04-13 16:13:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 16:17:52]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492099963491000,
"parent_rev": "9dd1986a8ffdd33a72ebab4063752c973f69432c", "commit_rev":
"cfd215a5b6b3d1ab60398fc8c4a522b0d8ce611e"}

[commit-bot: I haz the power, 2017-04-13 16:18:43]

Description was changed from

==========
Improve toolbar button ink drop ripples at 1.25x

1. combining scaling and translation transforms on nested layers seems
   to create unfixable blurriness, so instead do all transforms on the
   painted layers.
2. adjust the size of the painted layers by a fractional amount to
   ensure pixel alignment of edges of layer (at start and end points of
   animation).

BUG=706228
==========

to

==========
Improve toolbar button ink drop ripples at 1.25x

1. combining scaling and translation transforms on nested layers seems
   to create unfixable blurriness, so instead do all transforms on the
   painted layers.
2. adjust the size of the painted layers by a fractional amount to
   ensure pixel alignment of edges of layer (at start and end points of
   animation).

BUG=706228

Review-Url: https://codereview.chromium.org/2805813007
Cr-Commit-Position: refs/heads/master@{#464430}
Committed:
https://chromium.googlesource.com/chromium/src/+/cfd215a5b6b3d1ab60398fc8c4a5...
==========

[commit-bot: I haz the power, 2017-04-13 16:18:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/cfd215a5b6b3d1ab60398fc8c4a5...