Disabled ink drop animations based on gfx::Animation::ShouldRenderRichAnimation().

Disabled ink drop animations based on gfx::Animation::ShouldRenderRichAnimation().

Forces an animation duration of 0 for all ink drop animations if
ShouldRenderRichAnimations() returns false.

BUG=658384
Committed: https://crrev.com/03ac98e06185f954738f1d3ec98c387b412c53f3
Cr-Commit-Position: refs/heads/master@{#434301}

[bruthig, 2016-11-22 21:04:24]

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

[commit-bot: I haz the power, 2016-11-22 21:05:03]

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, 2016-11-22 22:13:07]

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

[commit-bot: I haz the power, 2016-11-22 22:13:08]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-11-23 00:00:22]

Tested this on Windows.  When "unnecessary animations" are disabled on the
system, clicking goes straight to the terminal state of the ripple, which is
still distinct from the hover state (e.g. on a toolbar button, this transitions
from "round rect" to "circle").

I don't know whether we consider that sufficiently non-animated or not.  (The
"not" answer would be one that basically eliminates click feedback, I guess?) 
It seems OK to me.

From a code review perspective, it'd be nice to avoid the word "temporary" in
this patch unless we know this really is temporary, i.e. someone is going to try
to implement the larger fix.

[bruthig, 2016-11-23 20:42:58]

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

[commit-bot: I haz the power, 2016-11-23 20:43:56]

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

[bruthig, 2016-11-23 20:52:05]

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

[bruthig, 2016-11-23 20:52:06]

I've removed 'Temporary' from the comments.

Peter, can you do a full review?

[Peter Kasting, 2016-11-23 21:02:25]

LGTM

https://codereview.chromium.org/2520353004/diff/40001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2520353004/diff/40001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_ripple.cc:99: // Disable ink drop
animations for accessibility purposes. See
Nit: This sentence kinda sounds like we're always disabling the animation.  I
wonder if it should be more like "Ink drop animations are controlled by the
system animation settings for accessibility reasons."

[bruthig, 2016-11-23 21:43:36]

Sending to CQ...

https://codereview.chromium.org/2520353004/diff/40001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2520353004/diff/40001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_ripple.cc:99: // Disable ink drop
animations for accessibility purposes. See
On 2016/11/23 21:02:25, Peter Kasting wrote:
> Nit: This sentence kinda sounds like we're always disabling the animation.  I
> wonder if it should be more like "Ink drop animations are controlled by the
> system animation settings for accessibility reasons."

Done.

[bruthig, 2016-11-23 21:43:39]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-23 21:43:39]

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/2520353004/#ps60001
(title: "Updated comment wording.")

[commit-bot: I haz the power, 2016-11-23 21:44:43]

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, 2016-11-23 23:46:34]

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

[commit-bot: I haz the power, 2016-11-23 23:46:35]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[bruthig, 2016-11-23 23:48:07]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-11-23 23:49:14]

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, 2016-11-24 01:51:30]

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

[commit-bot: I haz the power, 2016-11-24 01:51:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[bruthig, 2016-11-24 04:11:48]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-11-24 04:12:28]

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, 2016-11-24 04:47:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479960708441060,
"parent_rev": "d837d00ab899029a025dbf3f7fe1d70a50d3d22b", "commit_rev":
"fb0e0c38e39a884ab4de660ab233d4ac1630df6a"}

[commit-bot: I haz the power, 2016-11-24 04:47:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-24 04:49:41]

Description was changed from

==========
Disabled ink drop animations based on
gfx::Animation::ShouldRenderRichAnimation().

Forces an animation duration of 0 for all ink drop animations if
ShouldRenderRichAnimations() returns false.

BUG=658384
==========

to

==========
Disabled ink drop animations based on
gfx::Animation::ShouldRenderRichAnimation().

Forces an animation duration of 0 for all ink drop animations if
ShouldRenderRichAnimations() returns false.

BUG=658384

Committed: https://crrev.com/03ac98e06185f954738f1d3ec98c387b412c53f3
Cr-Commit-Position: refs/heads/master@{#434301}
==========

[commit-bot: I haz the power, 2016-11-24 04:49:42]

Patchset 4 (id:??) landed as
https://crrev.com/03ac98e06185f954738f1d3ec98c387b412c53f3
Cr-Commit-Position: refs/heads/master@{#434301}

[bruthig, 2016-11-30 15:12:26]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2539913004/ by bruthig@chromium.org.

The reason for reverting is: Failing tests on windows bots.  I suspect the tests
are actually dependent on environment settings which is not good and will
require time to investigate..

[Peter Kasting, 2016-11-30 19:07:19]

On 2016/11/30 15:12:26, bruthig wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2539913004/ by mailto:bruthig@chromium.org.
> 
> The reason for reverting is: Failing tests on windows bots.  I suspect the
tests
> are actually dependent on environment settings which is not good and will
> require time to investigate..

While we can probably force the value of the flag to make sure the behavior is
consistent, the more interesting question is why we have tests that fail when
this behavior changes.

[bruthig, 2016-11-30 20:05:52]

On 2016/11/30 19:07:19, Peter Kasting wrote:
> On 2016/11/30 15:12:26, bruthig wrote:
> > A revert of this CL (patchset #4 id:60001) has been created in
> > https://codereview.chromium.org/2539913004/ by mailto:bruthig@chromium.org.
> > 
> > The reason for reverting is: Failing tests on windows bots.  I suspect the
> tests
> > are actually dependent on environment settings which is not good and will
> > require time to investigate..
> 
> While we can probably force the value of the flag to make sure the behavior is
> consistent, the more interesting question is why we have tests that fail when
> this behavior changes.

Agreed, that is why I went for a full revert until I have time to investigate.