views: disable ink drops altogether on Mac

views: disable ink drops altogether on Mac

These were previously disabled piecemeal using SetInkDropMode(), but that was
not the right abstraction layer to disable them, because many other controls
individually call SetInkDropMode() to customize their own behavior. Instead,
disable it inside InkDropHostView.

BUG=701888
Review-Url: https://codereview.chromium.org/2833133003
Cr-Commit-Position: refs/heads/master@{#466642}
Committed: https://chromium.googlesource.com/chromium/src/+/5d32b67b86c16add90f6ac864d9741a959435ca1

[Elly Fong-Jones, 2017-04-21 17:51:52]

Description was changed from

==========
views: disable ink drops altogether on Mac

These were previously disabled piecemeal using SetInkDropMode(), but that was
not the right abstraction layer to disable them, because many other controls
individually call SetInkDropMode() to customize their own behavior. Instead,
disable it inside InkDropHostView.

BUG=701888
==========

to

==========
views: disable ink drops altogether on Mac

These were previously disabled piecemeal using SetInkDropMode(), but that was
not the right abstraction layer to disable them, because many other controls
individually call SetInkDropMode() to customize their own behavior. Instead,
disable it inside InkDropHostView.

BUG=701888
==========

[Elly Fong-Jones, 2017-04-21 17:51:52]

ellyjones@chromium.org changed reviewers:
+ sky@chromium.org

[Elly Fong-Jones, 2017-04-21 17:52:04]

sky: ptal? :)

[bruthig, 2017-04-21 18:03:09]

On 2017/04/21 17:52:04, Elly Fong-Jones wrote:
> sky: ptal? :)

I'm curious, is there any sort of hover or press affordance?

[Elly Fong-Jones, 2017-04-21 18:09:09]

On 2017/04/21 18:03:09, bruthig wrote:
> On 2017/04/21 17:52:04, Elly Fong-Jones wrote:
> > sky: ptal? :)
> 
> I'm curious, is there any sort of hover or press affordance?

Yes - on Mac, hovered/pressed buttons get shaded darker than they usually are.

[bruthig, 2017-04-21 20:30:07]

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

[bruthig, 2017-04-21 20:30:08]

https://codereview.chromium.org/2833133003/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2833133003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_host_view.cc:280: ink_drop_ =
base::MakeUnique<InkDropStub>();
I think it would be better if you returned an InkDropStub from here when you are
on Mac.  This will suppress the InkDropHighlights for mouse hover and keyboard
focus as well.

[sky, 2017-04-21 21:08:35]

LGTM

Grumble... Yet another sign that ink drop effects are integrated at the wrong
level of abstraction. By that I mean having view classes directly extend ink
drop related classes when ink drop isn't used is wrong. Views should provides
abstractions that make it possible to do ink drop effects without changing the
class hierarchy.

[bruthig, 2017-04-21 21:23:15]

nlgtm

I don't understand how this approach is going to suppress the InkDropHighlight
for mouse hover.

On 2017/04/21 21:08:35, sky wrote:
> LGTM
> 
> Grumble... Yet another sign that ink drop effects are integrated at the wrong
> level of abstraction. By that I mean having view classes directly extend ink
> drop related classes when ink drop isn't used is wrong. Views should provides
> abstractions that make it possible to do ink drop effects without changing the
> class hierarchy.

I'm starting to pursue fixing this full time now.  Expect some early game plan
docs late next week.

[Elly Fong-Jones, 2017-04-24 13:41:53]

On 2017/04/21 21:23:15, bruthig wrote:
> nlgtm
> 
> I don't understand how this approach is going to suppress the InkDropHighlight
> for mouse hover.
> 
> On 2017/04/21 21:08:35, sky wrote:
> > LGTM
> > 
> > Grumble... Yet another sign that ink drop effects are integrated at the
wrong
> > level of abstraction. By that I mean having view classes directly extend ink
> > drop related classes when ink drop isn't used is wrong. Views should
provides
> > abstractions that make it possible to do ink drop effects without changing
the
> > class hierarchy.
> 
> I'm starting to pursue fixing this full time now.  Expect some early game plan
> docs late next week.

bruthig: please take a look at patchset 2 - is that the approach you had in
mind? I'm happy to do it this way instead pending your refactor.

[bruthig, 2017-04-24 13:58:02]

On 2017/04/24 13:41:53, Elly Fong-Jones wrote:
> On 2017/04/21 21:23:15, bruthig wrote:
> > nlgtm
> > 
> > I don't understand how this approach is going to suppress the
InkDropHighlight
> > for mouse hover.
> > 
> > On 2017/04/21 21:08:35, sky wrote:
> > > LGTM
> > > 
> > > Grumble... Yet another sign that ink drop effects are integrated at the
> wrong
> > > level of abstraction. By that I mean having view classes directly extend
ink
> > > drop related classes when ink drop isn't used is wrong. Views should
> provides
> > > abstractions that make it possible to do ink drop effects without changing
> the
> > > class hierarchy.
> > 
> > I'm starting to pursue fixing this full time now.  Expect some early game
plan
> > docs late next week.
> 
> bruthig: please take a look at patchset 2 - is that the approach you had in
> mind? I'm happy to do it this way instead pending your refactor.

lgtm, thanks!

[Elly Fong-Jones, 2017-04-24 15:14:22]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-04-24 15:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2833133003/#ps20001
(title: "use InkDropStub")

[commit-bot: I haz the power, 2017-04-24 15:14:48]

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-24 16:20:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493046862130810,
"parent_rev": "0d18c2297bbc827cec2ec5d6de64fc92d280368a", "commit_rev":
"5d32b67b86c16add90f6ac864d9741a959435ca1"}

[commit-bot: I haz the power, 2017-04-24 16:20:51]

Description was changed from

==========
views: disable ink drops altogether on Mac

These were previously disabled piecemeal using SetInkDropMode(), but that was
not the right abstraction layer to disable them, because many other controls
individually call SetInkDropMode() to customize their own behavior. Instead,
disable it inside InkDropHostView.

BUG=701888
==========

to

==========
views: disable ink drops altogether on Mac

These were previously disabled piecemeal using SetInkDropMode(), but that was
not the right abstraction layer to disable them, because many other controls
individually call SetInkDropMode() to customize their own behavior. Instead,
disable it inside InkDropHostView.

BUG=701888

Review-Url: https://codereview.chromium.org/2833133003
Cr-Commit-Position: refs/heads/master@{#466642}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d32b67b86c16add90f6ac864d97...
==========

[commit-bot: I haz the power, 2017-04-24 16:20:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5d32b67b86c16add90f6ac864d97...

[findit-for-me, 2017-04-24 17:13:35]

Findit(https://goo.gl/kROfz5) identified this CL at revision 466642 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Elly Fong-Jones, 2017-04-24 17:21:01]

On 2017/04/24 17:13:35, findit-for-me wrote:
> Findit(https://goo.gl/kROfz5) identified this CL at revision 466642 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

Findit is correct - I will put up a CL to fix this test breakage in a moment.

[Elly Fong-Jones, 2017-04-24 17:34:42]

On 2017/04/24 17:21:01, Elly Fong-Jones wrote:
> On 2017/04/24 17:13:35, findit-for-me wrote:
> > Findit(https://goo.gl/kROfz5) identified this CL at revision 466642 as the
> > culprit for
> > failures in the build cycles as shown on:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> Findit is correct - I will put up a CL to fix this test breakage in a moment.

https://codereview.chromium.org/2835243002/ is the fix, TBR sky@ and sending to
cq :)