[ash-md] Added different highlighting modes to the InkDropImpl.

Added a 'show highlight on ripple' mode to the InkDropImpl.

Depends on WIP: https://codereview.chromium.org/2473483013/ to fix the failing test:
InkDropImplHideAutoHighlightTest.DeactivatedAnimationSkippedWhenFocused_0

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests

Committed: https://crrev.com/ca8b19cd01cfb691bfa31e4605f4204dd8da8aab
Cr-Commit-Position: refs/heads/master@{#430490}

[bruthig, 2016-10-23 20:37:48]

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

[commit-bot: I haz the power, 2016-10-23 20:37:53]

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-10-23 20:57:23]

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

[commit-bot: I haz the power, 2016-10-23 20:57:23]

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

[bruthig, 2016-10-26 22:06:43]

mohsen@, consider this more an FYI at this point of the new
InkDropHostView::CreateInkDrop() method and an example of a state machine
implementation in InkDropImpl.

I still have to polish it up before sending it out for full review.

[bruthig, 2016-10-26 22:07:25]

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

[commit-bot: I haz the power, 2016-10-26 22:08:07]

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-10-26 22:57:20]

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

[commit-bot: I haz the power, 2016-10-26 22:57:20]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bruthig, 2016-11-01 01:08:29]

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

[commit-bot: I haz the power, 2016-11-01 01:08: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, 2016-11-01 01:22:02]

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

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

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

[bruthig, 2016-11-01 15:39:06]

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

[commit-bot: I haz the power, 2016-11-01 15:39:31]

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-01 16:23:05]

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

[commit-bot: I haz the power, 2016-11-01 16:23:06]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bruthig, 2016-11-01 21:00:10]

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

[bruthig, 2016-11-01 21:00:11]

sky@, can you please take a high level look first while I work on adding/fixing
the tests?  FYI I've also sent you a high level design doc offline that should
help navigating the change.

Thanks,
Ben

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:793: if (highlight_)
Add TODO to remove this check since all CreateInkDropHighlight() implementations
should create an instance now.

[sky, 2016-11-02 02:52:11]

Here's some random comments. I haven't looked at it all, but I think I looked at
the core change and enough supporting files to get a feel for it.

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
ash/common/shelf/overflow_button.cc:87: return
CreateDefaultFloodFillInkDropImpl();
Is there a way the type of button could be expressed to views so that views
(ideally code outside of views, but that's for another day) could know the type
to create?

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host.h (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host.h:42: // Returns a configured InkDrop.
I have to admit I'm confused by the function names here and what the
expectations area. When does one need to be overriden and not the others? Using
the name of the effect rather than the state the effect is used for makes it
confusing. Consider CreateInkDropForPress, and CreateInkDropForHoverAndFocus(),
that makes it clear what the the effect is used for.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.cc:260: std::unique_ptr<InkDropImpl>
ink_drop(new InkDropImpl(this));
MakeUnique.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:51: static gfx::Size
CalculateLargeInkDropSize(const gfx::Size small_size);
const gfx::Size&

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:54: static const int
kInkDropLargeCornerRadius;
constants before functions.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:89: // the highlight state.
Please document what the expectation is for subclasses, that is what they should
do.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:103: virtual void ShowOnHoverChanged() {}
optional: is there a reason you aren't making these pure virtual. Isn't it easy
to miss an interesting state?

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:811: if (highlight_state_)
I do like the new code. It's easier to understand.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.h (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.h:53: void SetShowHighlightOnHover(bool
show_highlight_on_hover);
Do these need to change once a button is created? Could the configuration be
passed into the constructor?

[bruthig, 2016-11-03 23:49:28]

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

[commit-bot: I haz the power, 2016-11-03 23:50:19]

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-04 00:46:25]

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

[commit-bot: I haz the power, 2016-11-04 00:46:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bruthig, 2016-11-04 18:10:13]

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

[commit-bot: I haz the power, 2016-11-04 18:10:38]

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-04 18:45:08]

Description was changed from

==========
[WIP] Added different highlighting modes to the InkDropImpl.

Desc: TODO

BUG=TODO
TEST=TODO
==========

to

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests
==========

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

Description was changed from

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests
==========

to

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Depends on WIP: https://codereview.chromium.org/2473483013/ to fix the failing
test:
InkDropImplHideAutoHighlightTest.DeactivatedAnimationSkippedWhenFocused_0 

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests
==========

[bruthig, 2016-11-04 18:50:37]

sky@, this is now ready for full review. Can you please take a look or consider
delegating to someone since this is a huge change.

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
ash/common/shelf/overflow_button.cc:87: return
CreateDefaultFloodFillInkDropImpl();
On 2016/11/02 02:52:11, sky wrote:
> Is there a way the type of button could be expressed to views so that views
> (ideally code outside of views, but that's for another day) could know the
type
> to create?

I'm currently entertaining a few ideas:

1. Injecting an InkDropFactory
2. Making an InkDropView which is a container and can host child views.  The
InkDropView could be outside of Views and this may allow us to remove the
InkDropHostView from the Views hierarchy.

Agreed though, these scattered CreateInk*Drop() methods are getting out of hand
and difficult to maintain.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host.h (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host.h:42: // Returns a configured InkDrop.
On 2016/11/02 02:52:11, sky wrote:
> I have to admit I'm confused by the function names here and what the
> expectations area. When does one need to be overriden and not the others?
Using
> the name of the effect rather than the state the effect is used for makes it
> confusing. Consider CreateInkDropForPress, and
CreateInkDropForHoverAndFocus(),
> that makes it clear what the the effect is used for.

I've updated the docs as an interim fix.  I will take this into consideration
when re-working how Views actually choose which ink drop style to use.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.cc:260: std::unique_ptr<InkDropImpl>
ink_drop(new InkDropImpl(this));
On 2016/11/02 02:52:11, sky wrote:
> MakeUnique.

Done.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:51: static gfx::Size
CalculateLargeInkDropSize(const gfx::Size small_size);
On 2016/11/02 02:52:11, sky wrote:
> const gfx::Size&

Done.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:54: static const int
kInkDropLargeCornerRadius;
On 2016/11/02 02:52:11, sky wrote:
> constants before functions.

Done.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:89: // the highlight state.
On 2016/11/02 02:52:11, sky wrote:
> Please document what the expectation is for subclasses, that is what they
should
> do.

Done.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:103: virtual void ShowOnHoverChanged() {}
On 2016/11/02 02:52:11, sky wrote:
> optional: is there a reason you aren't making these pure virtual. Isn't it
easy
> to miss an interesting state?

Done.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:793: if (highlight_)
On 2016/11/01 21:00:10, bruthig wrote:
> Add TODO to remove this check since all CreateInkDropHighlight()
implementations
> should create an instance now.

Done.

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:811: if (highlight_state_)
On 2016/11/02 02:52:11, sky wrote:
> I do like the new code. It's easier to understand.

Yaay :)

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.h (right):

https://codereview.chromium.org/2447523002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.h:53: void SetShowHighlightOnHover(bool
show_highlight_on_hover);
On 2016/11/02 02:52:11, sky wrote:
> Do these need to change once a button is created? Could the configuration be
> passed into the constructor?

Not currently, but having them as configuration methods allows clients to create
a base prototype and update the few things that it wants to be different.  For
example see ToolbarActionView::CreateInkDrop().

[commit-bot: I haz the power, 2016-11-04 19:08:30]

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

[commit-bot: I haz the power, 2016-11-04 19:08:31]

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

[sky, 2016-11-04 20:36:13]

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
ash/common/shelf/overflow_button.cc:87: return
CreateDefaultFloodFillInkDropImpl();
On 2016/11/04 18:50:36, bruthig wrote:
> On 2016/11/02 02:52:11, sky wrote:
> > Is there a way the type of button could be expressed to views so that views
> > (ideally code outside of views, but that's for another day) could know the
> type
> > to create?
> 
> I'm currently entertaining a few ideas:
> 
> 1. Injecting an InkDropFactory
> 2. Making an InkDropView which is a container and can host child views.  The
> InkDropView could be outside of Views and this may allow us to remove the
> InkDropHostView from the Views hierarchy.
> 
> Agreed though, these scattered CreateInk*Drop() methods are getting out of
hand
> and difficult to maintain.

I was thinking more of an enum. What are the set of variations that are needed?

[bruthig, 2016-11-07 18:38:47]

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2447523002/diff/60001/ash/common/shelf/overfl...
ash/common/shelf/overflow_button.cc:87: return
CreateDefaultFloodFillInkDropImpl();
On 2016/11/04 20:36:12, sky wrote:
> On 2016/11/04 18:50:36, bruthig wrote:
> > On 2016/11/02 02:52:11, sky wrote:
> > > Is there a way the type of button could be expressed to views so that
views
> > > (ideally code outside of views, but that's for another day) could know the
> > type
> > > to create?
> > 
> > I'm currently entertaining a few ideas:
> > 
> > 1. Injecting an InkDropFactory
> > 2. Making an InkDropView which is a container and can host child views.  The
> > InkDropView could be outside of Views and this may allow us to remove the
> > InkDropHostView from the Views hierarchy.
> > 
> > Agreed though, these scattered CreateInk*Drop() methods are getting out of
> hand
> > and difficult to maintain.
> 
> I was thinking more of an enum. What are the set of variations that are
needed?

So basically there are 3 parameters (Highlight mode - 3 values, show highlight
on hover - 2 values, and show highlight on focus - 2 values) which means there
are 12 different possible configurations and currently we we are using 6
distinct configurations of an InkDropImpl instance.

Keep in mind that this does not account for all of the different
ripple/highlight configurations which are in general very specific to the UI are
that they are being applied to and this is closely related to the InkDropImpl
configuration.  That is why I was thinking of grouping together the creation of
these three parts into factories.

So, yes I could create an enum in InkDropHostView with 6 values but I don't
think something as simple as an enum is a good long term solution.

[bruthig, 2016-11-07 19:04:57]

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

[commit-bot: I haz the power, 2016-11-07 19:05:34]

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-07 19:55:24]

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

[commit-bot: I haz the power, 2016-11-07 19:55:25]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-07 23:30:19]

Ok, LGTM

[bruthig, 2016-11-07 23:36:40]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-11-07 23:37:50]

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-07 23:48:23]

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

[commit-bot: I haz the power, 2016-11-07 23:48:24]

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

[bruthig, 2016-11-08 01:04:00]

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

[commit-bot: I haz the power, 2016-11-08 01:04:35]

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-08 01:51:39]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2016-11-08 01:51:41]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-08 01:51:41]

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/2447523002/#ps270001
(title: "Merge branch 'master' into ink_drop_highlight_mode")

[commit-bot: I haz the power, 2016-11-08 01:52:20]

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

[bruthig, 2016-11-08 01:54:24]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2016-11-08 02:04:35]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-11-08 02:04:36]

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/2447523002/#ps290001
(title: "Fixed InkDropHostView::GetInkDrop() to use CreateInkDrop().")

[commit-bot: I haz the power, 2016-11-08 02:04:57]

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-08 02:54:27]

Description was changed from

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Depends on WIP: https://codereview.chromium.org/2473483013/ to fix the failing
test:
InkDropImplHideAutoHighlightTest.DeactivatedAnimationSkippedWhenFocused_0 

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests
==========

to

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Depends on WIP: https://codereview.chromium.org/2473483013/ to fix the failing
test:
InkDropImplHideAutoHighlightTest.DeactivatedAnimationSkippedWhenFocused_0 

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests
==========

[commit-bot: I haz the power, 2016-11-08 02:54:29]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2016-11-08 03:27:31]

Description was changed from

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Depends on WIP: https://codereview.chromium.org/2473483013/ to fix the failing
test:
InkDropImplHideAutoHighlightTest.DeactivatedAnimationSkippedWhenFocused_0 

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests
==========

to

==========
Added a 'show highlight on ripple' mode to the InkDropImpl.

Depends on WIP: https://codereview.chromium.org/2473483013/ to fix the failing
test:
InkDropImplHideAutoHighlightTest.DeactivatedAnimationSkippedWhenFocused_0

Currently the InkDropImpl implements 2 highlight modes:

1. Based only off of hover/focus state.
2. Hide the ripple when a ripple is visible in addition to the hover/focus
   states. This is used by the toolbar buttons, e.g. Back, Forward, etc.

This CL adds a 3rd highlight mode:

3. Show the highlight when the ripple is visible in addition to the hover/focus
   states. This will be used by buttons in the system menu as well as the
   bookmark row buttons.

There are 2 major problems with the existing implementation:

1. InkDropHostView's were allowed to decide when a highlight is to be shown
   instead of the InkDropImpl making the decision. The main mechanism used to
   implement this was via CustomButton::ShouldShowInkDropHighlight(). This is
   problematic since the InkDropImpl will now request an InkDropHighlight
   instance even when the host view is not hovered/focused.
2. The InkDropImpl was informed of the highlighting strategy via a property of
   the InkDropRipple, namely InkDropRipple::OverridesHighlight(). This was used
   as a workaround since the overrides of InkDropHostView did not have an
   opportunity to configure InkDropImpl properties.

There are 3 major aspects to this change:

1. Add an overrideable InkDropHost::CreateInkDrop() method to allow subclasses
   to configure the InkDropImpl instance.
2. Remove CustomButton::ShouldShowInkDropHighlight(),
   InkDropHostView::ShouldShowInkDropForFocus(), and
   InkDropRipple::OverridesHighlight() and replace them with
   InkDropHost::CreateInkDrop() overrides that configure the InkDropImpl as
   desired.
3. Refactor InkDropImpl to support the new highlight mode using a state machine.

Follow-up work:

- Remove safe guards for null return values from
  InkDropHost::CreateInkDropHighlight()
- Implement a better mechanism to inject the desired type/configuration of the
  ink drop to the host view.

BUG=649734
TEST=views_unittests, ash_unittests

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

[commit-bot: I haz the power, 2016-11-08 03:27:32]

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

[stgao, 2016-11-17 20:29:03]

FYI: this CL seems to introduce a flaky test
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....