Add MD ink drop ripple to shelf app items

ash/shelf/shelf_view.cc

Index: ash/shelf/shelf_view.cc
diff --git a/ash/shelf/shelf_view.cc b/ash/shelf/shelf_view.cc
index f2bf74f0be601364d8a1a4a3925c5ff54c02601b..ec4c7b1f4336ac0cf757f7033300171f75a3ed10 100644
--- a/ash/shelf/shelf_view.cc
+++ b/ash/shelf/shelf_view.cc
@@ -418,7 +418,7 @@ void ShelfView::Init() {
     view_model_->Add(child, static_cast<int>(i - items.begin()));
     AddChildView(child);
   }
-  overflow_button_ = new OverflowButton(this, shelf_);
+  overflow_button_ = new OverflowButton(this);
   overflow_button_->set_context_menu_controller(this);
   ConfigureChildView(overflow_button_);
   AddChildView(overflow_button_);
@@ -713,6 +713,25 @@ void ShelfView::EndDrag(bool cancel) {
   drag_and_drop_shelf_id_ = 0;
 }
 
+bool ShelfView::WillIgnoreEventOnButton(View* view, const ui::Event& event) {
+  if (dragging())
+    return true;
+
+  // Ignore if we are already in a pointer event sequence started with a repost
+  // event.
+  if (is_repost_event_)

[bruthig, 2016/06/09 03:26:00]

The interplay of |is_repost_event_|, |last_pressed_index_|,
|closing_event_time|, and IsRepostEvent() is very difficult to follow.  I'm sure
this could be made much simpler, but perhaps it should not be included in this
change.

WDYT?

[mohsen, 2016/06/09 21:24:51]

Probably, similar names of |is_repost_event_| and IsRepostEvent() which have
different meanings make it confusing. Changed the former to
|is_repost_event_on_same_item_|. Does it look better?

[bruthig, 2016/06/10 15:25:19]

Definitely looks better!

+    return true;
+
+  if (event.flags() & ui::EF_IS_DOUBLE_CLICK)
+    return true;
+
+  // Ignore if this is a repost event on the last pressed shelf item.
+  int index = view_model_->GetIndexOfView(view);
+  if (index == -1)
+    return true;
+  return IsRepostEvent(event) && (last_pressed_index_ == index);
+}
+
 void ShelfView::PointerPressedOnButton(views::View* view,
                                        Pointer pointer,
                                        const ui::LocatedEvent& event) {
@@ -720,13 +739,13 @@ void ShelfView::PointerPressedOnButton(views::View* view,
     return;
 
   int index = view_model_->GetIndexOfView(view);
-  if (index == -1)
-    return;
+  if (index == -1 || view_model_->view_size() <= 1)
+    return;  // View is being deleted, ignore request.
 
   ShelfItemDelegate* item_delegate =
       item_manager_->GetShelfItemDelegate(model_->items()[index].id);
-  if (view_model_->view_size() <= 1 || !item_delegate->IsDraggable())
-    return;  // View is being deleted or not draggable, ignore request.
+  if (!item_delegate->IsDraggable())
+    return;  // View is not draggable, ignore request.
 
   // Only when the repost event occurs on the same shelf item, we should ignore
   // the call in ShelfView::ButtonPressed(...).
@@ -776,6 +795,13 @@ void ShelfView::PointerReleasedOnButton(views::View* view,
     drag_view_ = nullptr;
 }
 
+void ShelfView::ButtonPressed(views::Button* sender,
+                              const ui::Event& event,
+                              views::InkDrop* ink_drop) {
+  if (!ButtonPressedImpl(sender, event, ink_drop) && !is_repost_event_)
+    ink_drop->AnimateToState(views::InkDropState::HIDDEN);
+}
+
 void ShelfView::LayoutToIdealBounds() {
   if (bounds_animator_->IsAnimating()) {
     AnimateToIdealBounds();
@@ -1044,6 +1070,8 @@ void ShelfView::PrepareForDrag(Pointer pointer, const ui::LocatedEvent& event) {
   // Move the view to the front so that it appears on top of other views.
   ReorderChildView(drag_view_, -1);
   bounds_animator_->StopAnimatingView(drag_view_);
+
+  drag_view_->OnDragStarted();
 }
 
 void ShelfView::ContinueDrag(const ui::LocatedEvent& event) {
@@ -1396,6 +1424,97 @@ void ShelfView::UpdateOverflowRange(ShelfView* overflow_view) const {
   overflow_view->last_visible_index_ = last_overflow_index;
 }
 
+bool ShelfView::ButtonPressedImpl(views::Button* sender,

[bruthig, 2016/06/09 03:25:59]

Consider renaming ButtonPressed as ButtonPressedImpl and keeping the declaration
and definition in the same spot.  It will make the diff easier to read and will
save your name from showing up beside a lot of lines in git blame ;)

[mohsen, 2016/06/09 21:24:51]

Right. I was trying to keep the ordering suggested by style guides (function
overrides after other functions). Now that I removed ButtonPressedImpl() (see
below), I moved ButtonPressed() definition back to its original location which
should make diff easier to read. The declaration can't be moved back as it needs
to be in the public section.

+                                  const ui::Event& event,
+                                  views::InkDrop* ink_drop) {
+  // Do not handle mouse release during drag.

[bruthig, 2016/06/09 03:25:59]

Would it be possible to replace most of these quick returns with a call to
WillIgnoreEventOnButton()?
i.e.
if (sender == overflow_button_) {
  ...
}

if (WillIgnoreEventOnButton(...))
  return false;

{
  ScopedTargetRootWindow scoped_target(
...

[mohsen, 2016/06/09 21:24:51]

It was not possible because for the double-click case we should've returned true
rather than false. However, now that I removed ButtonPressedImpl() (see below),
it is possible. Done.

+  if (dragging())
+    return false;
+
+  if (sender == overflow_button_) {
+    ToggleOverflowBubble();
+    shelf_button_pressed_metric_tracker_.ButtonPressed(
+        event, sender, ShelfItemDelegate::kNoAction);
+    return false;
+  }
+
+  int view_index = view_model_->GetIndexOfView(sender);
+  // May be -1 while in the process of animating closed.
+  if (view_index == -1)
+    return false;
+
+  // If the menu was just closed by the same event as this one, we ignore
+  // the call and don't open the menu again. See crbug.com/343005 for more
+  // detail.
+  if (is_repost_event_)
+    return false;
+
+  // Record the index for the last pressed shelf item.
+  last_pressed_index_ = view_index;
+
+  // Don't activate the item twice on double-click. Otherwise the window starts
+  // animating open due to the first click, then immediately minimizes due to
+  // the second click. The user most likely intended to open or minimize the
+  // item once, not do both.
+  if (event.flags() & ui::EF_IS_DOUBLE_CLICK)
+    return true;

[bruthig, 2016/06/09 03:26:00]

Shouldn't this be a return false?

[mohsen, 2016/06/09 21:24:51]

The return value of the function means whether the ink drop ripple is taken care
of or not. If not, we hide the ripple. HOWEVER, hiding the ripple is not needed
anymore (it was probably needed once, but as the patch developed, the need is
gone). So, I removed ButtonPressedImpl() helper altogether and moved the code
back to ButtonPressed().

+
+  {
+    ScopedTargetRootWindow scoped_target(
+        sender->GetWidget()->GetNativeView()->GetRootWindow());
+    // Slow down activation animations if shift key is pressed.
+    std::unique_ptr<ui::ScopedAnimationDurationScaleMode> slowing_animations;
+    if (event.IsShiftDown()) {
+      slowing_animations.reset(new ui::ScopedAnimationDurationScaleMode(
+          ui::ScopedAnimationDurationScaleMode::SLOW_DURATION));
+    }
+
+    // Collect usage statistics before we decide what to do with the click.
+    switch (model_->items()[view_index].type) {
+      case TYPE_APP_SHORTCUT:
+      case TYPE_WINDOWED_APP:
+      case TYPE_PLATFORM_APP:
+      case TYPE_BROWSER_SHORTCUT:
+        Shell::GetInstance()->metrics()->RecordUserMetricsAction(
+            UMA_LAUNCHER_CLICK_ON_APP);
+        break;
+
+      case TYPE_APP_LIST:
+        Shell::GetInstance()->metrics()->RecordUserMetricsAction(
+            UMA_LAUNCHER_CLICK_ON_APPLIST_BUTTON);
+        break;
+
+      case TYPE_APP_PANEL:
+      case TYPE_DIALOG:
+      case TYPE_IME_MENU:
+        break;
+
+      case TYPE_UNDEFINED:
+        NOTREACHED() << "ShelfItemType must be set.";
+        break;
+    }
+
+    ShelfItemDelegate::PerformedAction performed_action =
+        item_manager_->GetShelfItemDelegate(model_->items()[view_index].id)
+            ->ItemSelected(event);
+
+    shelf_button_pressed_metric_tracker_.ButtonPressed(event, sender,
+                                                       performed_action);
+
+    if (performed_action != ShelfItemDelegate::kNewWindowCreated &&
+        ShowListMenuForView(model_->items()[view_index], sender, event,
+                            ink_drop)) {
+      // Menu is run synchronously, the menu is closed now and we need to go to
+      // the deactivated state.
+      ink_drop->AnimateToState(views::InkDropState::DEACTIVATED);

[bruthig, 2016/06/09 03:26:00]

It might make sense to put the DEACTIVATED call in ShowListMenuForView().

[mohsen, 2016/06/09 21:24:51]

Sure. Done.

+      return true;
+    }
+    ink_drop->AnimateToState(views::InkDropState::ACTION_TRIGGERED);
+  }
+
+  return true;
+}
+
 gfx::Rect ShelfView::GetBoundsForDragInsertInScreen() {
   gfx::Size preferred_size;
   if (is_overflow_mode()) {
@@ -1667,89 +1786,10 @@ void ShelfView::ShelfItemMoved(int start_index, int target_index) {
     AnimateToIdealBounds();
 }
 
-void ShelfView::ButtonPressed(views::Button* sender, const ui::Event& event) {
-  // Do not handle mouse release during drag.
-  if (dragging())
-    return;
-
-  if (sender == overflow_button_) {
-    ToggleOverflowBubble();
-    shelf_button_pressed_metric_tracker_.ButtonPressed(
-        event, sender, ShelfItemDelegate::kNoAction);
-    return;
-  }
-
-  int view_index = view_model_->GetIndexOfView(sender);
-  // May be -1 while in the process of animating closed.
-  if (view_index == -1)
-    return;
-
-  // If the menu was just closed by the same event as this one, we ignore
-  // the call and don't open the menu again. See crbug.com/343005 for more
-  // detail.
-  if (is_repost_event_)
-    return;
-
-  // Record the index for the last pressed shelf item.
-  last_pressed_index_ = view_index;
-
-  // Don't activate the item twice on double-click. Otherwise the window starts
-  // animating open due to the first click, then immediately minimizes due to
-  // the second click. The user most likely intended to open or minimize the
-  // item once, not do both.
-  if (event.flags() & ui::EF_IS_DOUBLE_CLICK)
-    return;
-
-  {
-    ScopedTargetRootWindow scoped_target(
-        sender->GetWidget()->GetNativeView()->GetRootWindow());
-    // Slow down activation animations if shift key is pressed.
-    std::unique_ptr<ui::ScopedAnimationDurationScaleMode> slowing_animations;
-    if (event.IsShiftDown()) {
-      slowing_animations.reset(new ui::ScopedAnimationDurationScaleMode(
-            ui::ScopedAnimationDurationScaleMode::SLOW_DURATION));
-    }
-
-    // Collect usage statistics before we decide what to do with the click.
-    switch (model_->items()[view_index].type) {
-      case TYPE_APP_SHORTCUT:
-      case TYPE_WINDOWED_APP:
-      case TYPE_PLATFORM_APP:
-      case TYPE_BROWSER_SHORTCUT:
-        Shell::GetInstance()->metrics()->RecordUserMetricsAction(
-            UMA_LAUNCHER_CLICK_ON_APP);
-        break;
-
-      case TYPE_APP_LIST:
-        Shell::GetInstance()->metrics()->RecordUserMetricsAction(
-            UMA_LAUNCHER_CLICK_ON_APPLIST_BUTTON);
-        break;
-
-      case TYPE_APP_PANEL:
-      case TYPE_DIALOG:
-      case TYPE_IME_MENU:
-        break;
-
-      case TYPE_UNDEFINED:
-        NOTREACHED() << "ShelfItemType must be set.";
-        break;
-    }
-
-    ShelfItemDelegate::PerformedAction performed_action =
-        item_manager_->GetShelfItemDelegate(model_->items()[view_index].id)
-            ->ItemSelected(event);
-
-    shelf_button_pressed_metric_tracker_.ButtonPressed(event, sender,
-                                                       performed_action);
-
-    if (performed_action != ShelfItemDelegate::kNewWindowCreated)
-      ShowListMenuForView(model_->items()[view_index], sender, event);
-  }
-}
-
-void ShelfView::ShowListMenuForView(const ShelfItem& item,
+bool ShelfView::ShowListMenuForView(const ShelfItem& item,
                                     views::View* source,
-                                    const ui::Event& event) {
+                                    const ui::Event& event,
+                                    views::InkDrop* ink_drop) {
   ShelfItemDelegate* item_delegate =
       item_manager_->GetShelfItemDelegate(item.id);
   std::unique_ptr<ui::MenuModel> list_menu_model(
@@ -1758,16 +1798,20 @@ void ShelfView::ShowListMenuForView(const ShelfItem& item,
   // Make sure we have a menu and it has at least two items in addition to the
   // application title and the 3 spacing separators.
   if (!list_menu_model.get() || list_menu_model->GetItemCount() <= 5)
-    return;
+    return false;
 
+  ink_drop->AnimateToState(views::InkDropState::ACTIVATED);
   context_menu_id_ = item.id;
   ShowMenu(list_menu_model.get(), source, gfx::Point(), false,
            ui::GetMenuSourceTypeForEvent(event));
+  return true;
 }
 
 void ShelfView::ShowContextMenuForView(views::View* source,
                                        const gfx::Point& point,
                                        ui::MenuSourceType source_type) {
+  last_pressed_index_ = -1;
+
   const ShelfItem* item = ShelfItemForView(source);
   if (!item) {
     Shell::GetInstance()->ShowContextMenu(point, source_type);
@@ -1878,10 +1922,9 @@ bool ShelfView::IsRepostEvent(const ui::Event& event) {
 
   base::TimeDelta delta =
       base::TimeDelta(event.time_stamp() - closing_event_time_);
-  closing_event_time_ = base::TimeDelta();
   // If the current (press down) event is a repost event, the time stamp of
   // these two events should be the same.
-  return (delta.InMilliseconds() == 0);
+  return delta.InMilliseconds() == 0;
 }
 
 const ShelfItem* ShelfView::ShelfItemForView(const views::View* view) const {