Add MD ink drop ripple to app list button

Add MD ink drop ripple to app list button

This also needed following changes:
 - Add AppListPresenter::IsVisible() (plus all necessary plumbing) to be
   able to determine whether pressing app list button is going to show
   or hide the app list;
 - Modify InkDropHostView::SetHasInkDrop() to get a 3-valued enum
   instead of a bool which lets us disable default gesture handling for
   ink drops;
 - Modify CustomButton to not reshow pending ink drop when mouse is
   dragged back onto the button if the button is notify-on-press.

BUG=612567
Committed: https://crrev.com/01e15eecfca3da9f4188045fb0dfb174e7edb87a
Cr-Commit-Position: refs/heads/master@{#403459}

[mohsen, 2016-06-16 19:06:28]

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

[mohsen, 2016-06-16 19:06:28]

Please take a look... (I'll add some tests later)

[bruthig, 2016-06-17 17:58:45]

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:91: bool is_material =
ash::MaterialDesignController::IsShelfMaterial();
nit: const, same below

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:124: AnimateInkDrop(views::InkDropState::HIDDEN);
What's the use case for this?  Won't the ripple be hidden due to the HIDDEN call
in OnAppListDismissed()?

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1768: (performed_action !=
ShelfItemDelegate::kAppListMenuShown &&
A doc saying the AppListButton handles the ink drop animations itself would be
useful.

https://codereview.chromium.org/2070143003/diff/80001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2070143003/diff/80001/ash/shell.h#newcode264
ash/shell.h:264: bool IsApplistVisible() const;
You might want to run this change by jamescook@ sooner rather than later.  I
think he was trying to eliminate the Shelf helper methods in ash::Shell.

https://codereview.chromium.org/2070143003/diff/80001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/2070143003/diff/80001/ui/views/controls/butto...
ui/views/controls/button/custom_button.cc:166: bool should_show_pending =
nit: const, and should probably be consistent and add it to
|should_enter_pushed| above as well.

[mohsen, 2016-06-17 22:21:29]

Patchset #6 (id:100001) has been deleted

[mohsen, 2016-06-18 06:25:16]

Added tests and addressed review comments. Please take another look...

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:91: bool is_material =
ash::MaterialDesignController::IsShelfMaterial();
On 2016/06/17 at 17:58:45, bruthig wrote:
> nit: const, same below

Done.

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:124: AnimateInkDrop(views::InkDropState::HIDDEN);
On 2016/06/17 at 17:58:45, bruthig wrote:
> What's the use case for this?  Won't the ripple be hidden due to the HIDDEN
call in OnAppListDismissed()?

I guess this was needed for when default gesture handling was not disabled yet
to hide pending ripples that were not leading to app list activation. Not needed
anymore. Thanks for catching this.

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2070143003/diff/80001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1768: (performed_action !=
ShelfItemDelegate::kAppListMenuShown &&
On 2016/06/17 at 17:58:45, bruthig wrote:
> A doc saying the AppListButton handles the ink drop animations itself would be
useful.

Added a comment.

https://codereview.chromium.org/2070143003/diff/80001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2070143003/diff/80001/ash/shell.h#newcode264
ash/shell.h:264: bool IsApplistVisible() const;
On 2016/06/17 at 17:58:45, bruthig wrote:
> You might want to run this change by jamescook@ sooner rather than later.  I
think he was trying to eliminate the Shelf helper methods in ash::Shell.

Sure. Will send the patch to jamescook@ ASAP. But, I guess whatever we do for
other helpers can be done for this one, too.

https://codereview.chromium.org/2070143003/diff/80001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/2070143003/diff/80001/ui/views/controls/butto...
ui/views/controls/button/custom_button.cc:166: bool should_show_pending =
On 2016/06/17 at 17:58:45, bruthig wrote:
> nit: const, and should probably be consistent and add it to
|should_enter_pushed| above as well.

Done.

[bruthig, 2016-06-20 14:44:33]

All in all looking good, just some requests for test changes.

https://codereview.chromium.org/2070143003/diff/120001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/120001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2163: // Tests that changing visiblity of the
app list transitions app list button's
These tests should probably use the EventGenerator to ensure the full event
dispatching is used.  Otherwise we won't know if the
InkDropHostView::InkDropGestureHandler is being used or not.

https://codereview.chromium.org/2070143003/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/2070143003/diff/120001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:170: if (should_show_pending &&
Can you add/update some tests for this new logic?

[mohsen, 2016-06-21 06:33:24]

https://codereview.chromium.org/2070143003/diff/120001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/120001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2163: // Tests that changing visiblity of the
app list transitions app list button's
On 2016/06/20 at 14:44:33, bruthig wrote:
> These tests should probably use the EventGenerator to ensure the full event
dispatching is used.  Otherwise we won't know if the
InkDropHostView::InkDropGestureHandler is being used or not.

Done.

https://codereview.chromium.org/2070143003/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/2070143003/diff/120001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:170: if (should_show_pending &&
On 2016/06/20 at 14:44:33, bruthig wrote:
> Can you add/update some tests for this new logic?

Done.

[mohsen, 2016-06-21 18:56:35]

Updated tests. Please take a look...

[bruthig, 2016-06-22 19:07:29]

https://codereview.chromium.org/2070143003/diff/160001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/160001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2119: if (target_visibility_)
nit: This quick return isn't adding too much value, same goes for the one in
Dismiss().

https://codereview.chromium.org/2070143003/diff/160001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2128: void ToggleAppList(int64_t display_id)
override {
Interesting, I find it weird that the AppListPresenterImpl bases it's toggle off
of the current visibility as opposed to the target visibility.  Do you know if
the mus implementation does the same?

i.e. the following code is dependent on the animation timing, whereas it would
be deterministic if Toggle was based off of target visibility.

presenter_->Show(0);
presenter_->ToggleAppList(0);
...
presenter_->ToggleAppList(0);
ASSERT_EQ(true, presenter_->IsTargetVisibility());

Anyway, if we wanted to more thoroughly test the AppListButton this test fake
could be enhanced to behave in both ways but this would be going above and
beyond and I will leave it up to you if you want to do that.  If not, I think is
worth adding an explicit comment to the ToggleAppList() function or class that
says "this test double toggles based off of current visibility and not target
visibility."

https://codereview.chromium.org/2070143003/diff/160001/ui/views/animation/tes...
File ui/views/animation/test/ink_drop_host_view_test_api.cc (right):

https://codereview.chromium.org/2070143003/diff/160001/ui/views/animation/tes...
ui/views/animation/test/ink_drop_host_view_test_api.cc:16: if
(!host_view_->ink_drop_)
This should never result in true, InkDropHostViews should always have an
|ink_drop_| instance.

Perhaps we need a SetInkDrop(std::unique_ptr<InkDrop> ink_drop, bool
handles_gesture_event) overload?

https://codereview.chromium.org/2070143003/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/custom_button_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/160001/ui/views/controls/butt...
ui/views/controls/button/custom_button_unittest.cc:455: TEST_F(CustomButtonTest,
InkDropShowHideOnMouseDragged) {
nit: Can you add the suffix 'NotifyOnRelease'.

https://codereview.chromium.org/2070143003/diff/160001/ui/views/controls/butt...
ui/views/controls/button/custom_button_unittest.cc:461: 
nit: Can you add 'button()->set_notify_action(CustomButton::NOTIFY_ON_RELEASE);'
here to be explicit.

[mohsen, 2016-06-23 15:47:00]

Patchset #9 (id:180001) has been deleted

[mohsen, 2016-06-23 15:49:56]

https://codereview.chromium.org/2070143003/diff/160001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/160001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2119: if (target_visibility_)
On 2016/06/22 at 19:07:29, bruthig wrote:
> nit: This quick return isn't adding too much value, same goes for the one in
Dismiss().

Removed.

https://codereview.chromium.org/2070143003/diff/160001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2128: void ToggleAppList(int64_t display_id)
override {
On 2016/06/22 at 19:07:29, bruthig wrote:
> Interesting, I find it weird that the AppListPresenterImpl bases it's toggle
off of the current visibility as opposed to the target visibility.  Do you know
if the mus implementation does the same?
> 
> i.e. the following code is dependent on the animation timing, whereas it would
be deterministic if Toggle was based off of target visibility.
> 
> presenter_->Show(0);
> presenter_->ToggleAppList(0);
> ...
> presenter_->ToggleAppList(0);
> ASSERT_EQ(true, presenter_->IsTargetVisibility());
> 
> Anyway, if we wanted to more thoroughly test the AppListButton this test fake
could be enhanced to behave in both ways but this would be going above and
beyond and I will leave it up to you if you want to do that.  If not, I think is
worth adding an explicit comment to the ToggleAppList() function or class that
says "this test double toggles based off of current visibility and not target
visibility."

Yes, that's actually how AppListPresenterImpl prevents relaunching on a tap that
has closed it (by depending on the animation!). I believe Mus also uses
AppListPresenterImpl eventually, so that should behave the same.

I think existing test should suffice as the second way that uses target
visibility instead of the actual visibility is kind of already covered by tests
that start from a hidden app list: when user clicks on the button, the app list
gets hidden before the button receives the click and everything from that point
on is similar to when the app list was initially hidden.

Added some comments.

https://codereview.chromium.org/2070143003/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/custom_button_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/160001/ui/views/controls/butt...
ui/views/controls/button/custom_button_unittest.cc:455: TEST_F(CustomButtonTest,
InkDropShowHideOnMouseDragged) {
On 2016/06/22 at 19:07:29, bruthig wrote:
> nit: Can you add the suffix 'NotifyOnRelease'.

Done.

https://codereview.chromium.org/2070143003/diff/160001/ui/views/controls/butt...
ui/views/controls/button/custom_button_unittest.cc:461: 
On 2016/06/22 at 19:07:29, bruthig wrote:
> nit: Can you add
'button()->set_notify_action(CustomButton::NOTIFY_ON_RELEASE);' here to be
explicit.

Done.

[bruthig, 2016-06-23 16:11:21]

lgtm

[mohsen, 2016-06-23 16:40:53]

mohsen@chromium.org changed reviewers:
+ jamescook@chromium.org

[mohsen, 2016-06-23 16:40:54]

jamescook@: Can you take a look at ash changes please?

[James Cook, 2016-06-23 20:08:08]

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2132: bool is_visible_;
nit: initialize here, not in constructor

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2233: TestAppListShellDelegate*
shell_delegate_;  // Owned by Shell.
ditto

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2270: ui::test::EventGenerator
generator(shelf_view_->GetWidget()->GetNativeView());
would AshTestBase::GetEventGenerator() work here and below?

I'm trying to avoid direct usage of aura::Window / GetNativeView() in
//ash/shelf where I can.

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2468: }
Nice test suite. Thorough and well documented.

https://codereview.chromium.org/2070143003/diff/220001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2070143003/diff/220001/ash/shell.h#newcode259
ash/shell.h:259: bool IsApplistVisible() const;
I'm OK with this given the other 4 app-list methods on ash::Shell. In general,
though, I'm trying to move things to ash::WmShell and ash::WmShelf where
possible.

https://codereview.chromium.org/2070143003/diff/220001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/220001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:92: void SetHasInkDrop(bool
has_an_ink_drop, bool handles_gesture_event);
Having a method that takes two bools side by side, and only allows 3 states, is
confusing, especially when you already have a single-bool version. I would
prefer that you declare an enum for the 3 states and collapse the two methods
into one like SetHasInkDrop(InkDrop state).

Alternately you could have two methods, SetHasInkDrop() and
SetHandlesGestureEvents().

[mohsen, 2016-06-27 22:17:10]

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2132: bool is_visible_;
On 2016/06/23 at 20:08:07, James Cook wrote:
> nit: initialize here, not in constructor

Done.

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2233: TestAppListShellDelegate*
shell_delegate_;  // Owned by Shell.
On 2016/06/23 at 20:08:08, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2270: ui::test::EventGenerator
generator(shelf_view_->GetWidget()->GetNativeView());
On 2016/06/23 at 20:08:07, James Cook wrote:
> would AshTestBase::GetEventGenerator() work here and below?
> 
> I'm trying to avoid direct usage of aura::Window / GetNativeView() in
//ash/shelf where I can.

Yes, that would work. I just did not notice its existence! Done.

https://codereview.chromium.org/2070143003/diff/220001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2468: }
On 2016/06/23 at 20:08:07, James Cook wrote:
> Nice test suite. Thorough and well documented.

Thanks :-)

https://codereview.chromium.org/2070143003/diff/220001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2070143003/diff/220001/ash/shell.h#newcode259
ash/shell.h:259: bool IsApplistVisible() const;
On 2016/06/23 at 20:08:08, James Cook wrote:
> I'm OK with this given the other 4 app-list methods on ash::Shell. In general,
though, I'm trying to move things to ash::WmShell and ash::WmShelf where
possible.

Acknowledged.

https://codereview.chromium.org/2070143003/diff/220001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/220001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:92: void SetHasInkDrop(bool
has_an_ink_drop, bool handles_gesture_event);
On 2016/06/23 at 20:08:08, James Cook wrote:
> Having a method that takes two bools side by side, and only allows 3 states,
is confusing, especially when you already have a single-bool version. I would
prefer that you declare an enum for the 3 states and collapse the two methods
into one like SetHasInkDrop(InkDrop state).
> 
> Alternately you could have two methods, SetHasInkDrop() and
SetHandlesGestureEvents().

Right. I believe having two separate methods would also be confusing as one is
only meaningful if the other is set to true. It is also hard to manage
especially if they can be called in any order. Switched to using a 3-valued
enum.

[mohsen, 2016-06-27 22:43:47]

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

[commit-bot: I haz the power, 2016-06-27 22:46:53]

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

[James Cook, 2016-06-27 22:47:29]

LGTM

https://codereview.chromium.org/2070143003/diff/240001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/240001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:43: INK_DROP_WITHOUT_GESTURE_HANDLING,
I like this much better. Can you add a brief comment explaining why you would
choose with vs. without gesture handling?

[commit-bot: I haz the power, 2016-06-27 22:51:31]

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

[commit-bot: I haz the power, 2016-06-27 22:51:32]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mohsen, 2016-06-28 06:07:48]

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

[commit-bot: I haz the power, 2016-06-28 06:08: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, 2016-06-28 07:36:12]

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

[commit-bot: I haz the power, 2016-06-28 07:36:15]

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_...)

[mohsen, 2016-06-29 19:09:41]

Description was changed from

==========
Add MD ink drop ripple to app list button

BUG=612567
==========

to

==========
Add MD ink drop ripple to app list button

This also needed following changes:
 - Add AppListPresenter::IsVisible() (plus all necessary plumbing) to be
   able to determine whether pressing app list button is going to show
   or hide the app list;
 - Modify InkDropHostView::SetHasInkDrop() to get a 3-valued enum
   instead of a bool which lets us disable default gesture handling for
   ink drops;
 - Modify CustomButton to not reshow pending ink drop when mouse is
   dragged back onto the button if the button is notify-on-press.

BUG=612567
==========

[mohsen, 2016-06-29 19:16:16]

Patchset #15 (id:320001) has been deleted

[mohsen, 2016-06-29 21:35:54]

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

[commit-bot: I haz the power, 2016-06-29 21:38:14]

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

[mohsen, 2016-06-29 21:41:36]

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

[mohsen, 2016-06-29 21:41:37]

sky@: Can you please take an OWNERS look, please?

https://codereview.chromium.org/2070143003/diff/240001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/240001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:43: INK_DROP_WITHOUT_GESTURE_HANDLING,
On 2016/06/27 at 22:47:28, James Cook wrote:
> I like this much better. Can you add a brief comment explaining why you would
choose with vs. without gesture handling?

Done.

[commit-bot: I haz the power, 2016-06-29 22:35:09]

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

[commit-bot: I haz the power, 2016-06-29 22:35:11]

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

[sky, 2016-06-29 22:58:34]

https://codereview.chromium.org/2070143003/diff/360001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/360001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:97: void SetHasInkDrop(InkDropType
ink_drop_type);
SetHasInkDrop makes sense when the type is boolean, but now that it's an enum I
think SetInkDropType() makes more sense. WDYT?

[mohsen, 2016-06-29 23:54:34]

Patchset #17 (id:380001) has been deleted

[mohsen, 2016-06-29 23:58:17]

Patchset #17 (id:400001) has been deleted

[mohsen, 2016-06-30 00:03:45]

https://codereview.chromium.org/2070143003/diff/360001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/360001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:97: void SetHasInkDrop(InkDropType
ink_drop_type);
On 2016/06/29 at 22:58:34, sky wrote:
> SetHasInkDrop makes sense when the type is boolean, but now that it's an enum
I think SetInkDropType() makes more sense. WDYT?

Yeah, I had thought of SetInkDropType(), but that might be confused with
different types of InkDrop's and InkDropRipple's that we have. But, I think
InkDropMode and SetInkDropMode() should be good. Renamed.

[sky, 2016-06-30 00:35:34]

https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view_md.cc:190: if
(ui::MaterialDesignController::IsModeMaterial())
This class is only created when in md, so you can remove this.

https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:48: INK_DROP_WITH_GESTURE_HANDLING,
Sorry, one concern I have in looking at the changes to supporting code is that
it isn't clear why any of the code wants gesture handling. My concern is just
the naming, I think if INK_DROP_WITHOUT_GESTURE_HANDLING was named INK_DROP_ON
(or InkDropMode::ON) it would be easier to understand and
InkDropMode::ON_NO_GESTURE_HANDLER for the outliers.
Another option is to keep the boolean and instead have an explicit
SetIsGestureHandlingEnabled() to turn on/off the gesture handling. I actually
might go for that. WDYT?

[mohsen, 2016-06-30 08:54:07]

https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_item_view_md.cc:190: if
(ui::MaterialDesignController::IsModeMaterial())
On 2016/06/30 at 00:35:34, sky wrote:
> This class is only created when in md, so you can remove this.

Done.

https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:48: INK_DROP_WITH_GESTURE_HANDLING,
On 2016/06/30 at 00:35:34, sky wrote:
> Sorry, one concern I have in looking at the changes to supporting code is that
it isn't clear why any of the code wants gesture handling. My concern is just
the naming, I think if INK_DROP_WITHOUT_GESTURE_HANDLING was named INK_DROP_ON
(or InkDropMode::ON) it would be easier to understand and
InkDropMode::ON_NO_GESTURE_HANDLER for the outliers.
> Another option is to keep the boolean and instead have an explicit
SetIsGestureHandlingEnabled() to turn on/off the gesture handling. I actually
might go for that. WDYT?

Having two separate functions makes the code a bit complicated. First,
SetIsGestureHandlingEnabled() is not meaningful if ink drop is not enabled.
Second, they can probably be called in any order which might be unnecessarily
difficult to handle; especially that currently the difference between ink drop
being enabled vs. disabled is having an instance of InkDropImpl vs. InkDropStub.
This default gesture handler is also something that we are considering getting
rid of (see TODO above its definition in ink_drop_host_view.cc). All in all, I
think this approach is not worth its complexity. So, I opted for your first
suggestion of renaming enum values to OFF, ON, ON_NO_GESTURE_HANDLER.

[mohsen, 2016-06-30 08:54:17]

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

[commit-bot: I haz the power, 2016-06-30 08:54:30]

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-06-30 09:35:11]

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

[commit-bot: I haz the power, 2016-06-30 09:35:12]

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

[sky, 2016-06-30 16:57:39]

On 2016/06/30 08:54:07, mohsen wrote:
>
https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
> File chrome/browser/ui/views/download/download_item_view_md.cc (right):
> 
>
https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
> chrome/browser/ui/views/download/download_item_view_md.cc:190: if
> (ui::MaterialDesignController::IsModeMaterial())
> On 2016/06/30 at 00:35:34, sky wrote:
> > This class is only created when in md, so you can remove this.
> 
> Done.
> 
>
https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
> File ui/views/animation/ink_drop_host_view.h (right):
> 
>
https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
> ui/views/animation/ink_drop_host_view.h:48: INK_DROP_WITH_GESTURE_HANDLING,
> On 2016/06/30 at 00:35:34, sky wrote:
> > Sorry, one concern I have in looking at the changes to supporting code is
that
> it isn't clear why any of the code wants gesture handling. My concern is just
> the naming, I think if INK_DROP_WITHOUT_GESTURE_HANDLING was named INK_DROP_ON
> (or InkDropMode::ON) it would be easier to understand and
> InkDropMode::ON_NO_GESTURE_HANDLER for the outliers.
> > Another option is to keep the boolean and instead have an explicit
> SetIsGestureHandlingEnabled() to turn on/off the gesture handling. I actually
> might go for that. WDYT?
> 
> Having two separate functions makes the code a bit complicated. First,
> SetIsGestureHandlingEnabled() is not meaningful if ink drop is not enabled.
> Second, they can probably be called in any order which might be unnecessarily
> difficult to handle; especially that currently the difference between ink drop
> being enabled vs. disabled is having an instance of InkDropImpl vs.
InkDropStub.
> This default gesture handler is also something that we are considering getting
> rid of (see TODO above its definition in ink_drop_host_view.cc). All in all, I
> think this approach is not worth its complexity. So, I opted for your first
> suggestion of renaming enum values to OFF, ON, ON_NO_GESTURE_HANDLER.

To your point about awkwardness of additional api. I would say it's no worse
than set_ink_drop_visible_opacity(). But as I mentioned I'm fine with the new
naming. Review comments to follow.

[sky, 2016-06-30 16:57:48]

LGTM

https://codereview.chromium.org/2070143003/diff/440001/ui/app_list/presenter/...
File ui/app_list/presenter/app_list_presenter.h (right):

https://codereview.chromium.org/2070143003/diff/440001/ui/app_list/presenter/...
ui/app_list/presenter/app_list_presenter.h:35: // Returns actual visibility of
the app list.
nit: actual->current

https://codereview.chromium.org/2070143003/diff/440001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/440001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view_unittest.cc:18: namespace {
nit: nuke the namespace here, I don't think it's necessary.

[mohsen, 2016-07-01 06:06:52]

https://codereview.chromium.org/2070143003/diff/440001/ui/app_list/presenter/...
File ui/app_list/presenter/app_list_presenter.h (right):

https://codereview.chromium.org/2070143003/diff/440001/ui/app_list/presenter/...
ui/app_list/presenter/app_list_presenter.h:35: // Returns actual visibility of
the app list.
On 2016/06/30 at 16:57:48, sky wrote:
> nit: actual->current

Done.

https://codereview.chromium.org/2070143003/diff/440001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view_unittest.cc (right):

https://codereview.chromium.org/2070143003/diff/440001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view_unittest.cc:18: namespace {
On 2016/06/30 at 16:57:48, sky wrote:
> nit: nuke the namespace here, I don't think it's necessary.

Done.

[mohsen, 2016-07-01 06:12:26]

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

[commit-bot: I haz the power, 2016-07-01 06:12:33]

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-07-01 06:14:27]

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

[commit-bot: I haz the power, 2016-07-01 06:14:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[mohsen, 2016-07-01 06:23:57]

On 2016/06/30 at 16:57:39, sky wrote:
> On 2016/06/30 08:54:07, mohsen wrote:
> >
https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
> > File chrome/browser/ui/views/download/download_item_view_md.cc (right):
> > 
> >
https://codereview.chromium.org/2070143003/diff/420001/chrome/browser/ui/view...
> > chrome/browser/ui/views/download/download_item_view_md.cc:190: if
> > (ui::MaterialDesignController::IsModeMaterial())
> > On 2016/06/30 at 00:35:34, sky wrote:
> > > This class is only created when in md, so you can remove this.
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
> > File ui/views/animation/ink_drop_host_view.h (right):
> > 
> >
https://codereview.chromium.org/2070143003/diff/420001/ui/views/animation/ink...
> > ui/views/animation/ink_drop_host_view.h:48: INK_DROP_WITH_GESTURE_HANDLING,
> > On 2016/06/30 at 00:35:34, sky wrote:
> > > Sorry, one concern I have in looking at the changes to supporting code is
that
> > it isn't clear why any of the code wants gesture handling. My concern is
just
> > the naming, I think if INK_DROP_WITHOUT_GESTURE_HANDLING was named
INK_DROP_ON
> > (or InkDropMode::ON) it would be easier to understand and
> > InkDropMode::ON_NO_GESTURE_HANDLER for the outliers.
> > > Another option is to keep the boolean and instead have an explicit
> > SetIsGestureHandlingEnabled() to turn on/off the gesture handling. I
actually
> > might go for that. WDYT?
> > 
> > Having two separate functions makes the code a bit complicated. First,
> > SetIsGestureHandlingEnabled() is not meaningful if ink drop is not enabled.
> > Second, they can probably be called in any order which might be
unnecessarily
> > difficult to handle; especially that currently the difference between ink
drop
> > being enabled vs. disabled is having an instance of InkDropImpl vs.
InkDropStub.
> > This default gesture handler is also something that we are considering
getting
> > rid of (see TODO above its definition in ink_drop_host_view.cc). All in all,
I
> > think this approach is not worth its complexity. So, I opted for your first
> > suggestion of renaming enum values to OFF, ON, ON_NO_GESTURE_HANDLER.
> 
> To your point about awkwardness of additional api. I would say it's no worse
than set_ink_drop_visible_opacity(). But as I mentioned I'm fine with the new
naming. Review comments to follow.

I see. Yes, API-wise they're similar. Implementation-wise having
SetIsInkDropGestureHandlerEnabled() is a bit more complicated than
set_ink_drop_visibility(), because of the interplay of the ink drop and the
gesture handler (gesture handler should be created only if there is an ink
drop). We would need to add two more bool fields (|has_ink_drop_| and
|is_ink_drop_gesture_handler_enabled_|) to be able to handle it correctly.

[mohsen, 2016-07-01 06:29:58]

Patchset #20 (id:480001) has been deleted

[mohsen, 2016-07-01 06:30:59]

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

[commit-bot: I haz the power, 2016-07-01 06:31:11]

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

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

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

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-07-01 15:31:31]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-07-01 15:31:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
bruthig@chromium.org, jamescook@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2070143003/#ps500001
(title: "Rebased")

[commit-bot: I haz the power, 2016-07-01 15:31:42]

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-07-01 15:36:41]

Committed patchset #20 (id:500001)

[commit-bot: I haz the power, 2016-07-01 15:36:47]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 15:38:31]

Description was changed from

==========
Add MD ink drop ripple to app list button

This also needed following changes:
 - Add AppListPresenter::IsVisible() (plus all necessary plumbing) to be
   able to determine whether pressing app list button is going to show
   or hide the app list;
 - Modify InkDropHostView::SetHasInkDrop() to get a 3-valued enum
   instead of a bool which lets us disable default gesture handling for
   ink drops;
 - Modify CustomButton to not reshow pending ink drop when mouse is
   dragged back onto the button if the button is notify-on-press.

BUG=612567
==========

to

==========
Add MD ink drop ripple to app list button

This also needed following changes:
 - Add AppListPresenter::IsVisible() (plus all necessary plumbing) to be
   able to determine whether pressing app list button is going to show
   or hide the app list;
 - Modify InkDropHostView::SetHasInkDrop() to get a 3-valued enum
   instead of a bool which lets us disable default gesture handling for
   ink drops;
 - Modify CustomButton to not reshow pending ink drop when mouse is
   dragged back onto the button if the button is notify-on-press.

BUG=612567

Committed: https://crrev.com/01e15eecfca3da9f4188045fb0dfb174e7edb87a
Cr-Commit-Position: refs/heads/master@{#403459}
==========

[commit-bot: I haz the power, 2016-07-01 15:38:33]

Patchset 20 (id:??) landed as
https://crrev.com/01e15eecfca3da9f4188045fb0dfb174e7edb87a
Cr-Commit-Position: refs/heads/master@{#403459}