mash: Use mojo for ShelfItemDelegate and [app] MenuItem.

ash/common/shelf/shelf_view.cc

Index: ash/common/shelf/shelf_view.cc
diff --git a/ash/common/shelf/shelf_view.cc b/ash/common/shelf/shelf_view.cc
index c46b168b96fe3cc818aa5e87dc511376a85e2528..df9f25516c01c6a44854f0d25c2e61f1b3f3c40e 100644
--- a/ash/common/shelf/shelf_view.cc
+++ b/ash/common/shelf/shelf_view.cc
@@ -483,26 +483,13 @@ void ShelfView::ButtonPressed(views::Button* sender,
   }
 
   const int64_t display_id = window->GetDisplayNearestWindow().id();
-  ShelfAction performed_action =
-      model_->GetShelfItemDelegate(model_->items()[last_pressed_index_].id)
-          ->ItemSelected(event.type(), event.flags(), display_id,
-                         LAUNCH_FROM_UNKNOWN);
-
-  shelf_button_pressed_metric_tracker_.ButtonPressed(event, sender,
-                                                     performed_action);
-
-  // For the app list menu no TRIGGERED ink drop effect is needed and it
-  // handles its own ACTIVATED/DEACTIVATED states.
-  if (performed_action == SHELF_ACTION_NEW_WINDOW_CREATED ||
-      (performed_action != SHELF_ACTION_APP_LIST_SHOWN &&
-       !ShowListMenuForView(model_->items()[last_pressed_index_], sender, event,
-                            ink_drop))) {
-    ink_drop->AnimateToState(views::InkDropState::ACTION_TRIGGERED);
-  }
-  // Allow the menu to clear |scoped_root_window_for_new_windows_| during
-  // OnMenuClosed.
-  if (!IsShowingMenu())
-    scoped_root_window_for_new_windows_.reset();
+
+  // Notify the item of its selection; handle the result in AfterItemSelected.
+  const ShelfItem& item = model_->items()[last_pressed_index_];
+  model_->GetShelfItemDelegate(item.id)->ItemSelected(
+      ui::Event::Clone(event), display_id, LAUNCH_FROM_UNKNOWN,
+      base::Bind(&ShelfView::AfterItemSelected, base::Unretained(this), item,

[James Cook, 2017/03/09 01:09:46]

Hrm. Is Unretained safe here? Could this cause a shutdown crash? Probably not,
because the shelfview is destroyed synchronously with ash shutdown?

What if you disconnected an external monitor and that shelf got destroyed? Or
(horrors) somebody adds a context menu item that changes monitor / shelf state.

[msw, 2017/03/10 06:17:56]

Added WeakPtr use to skip AfterItemSelected if the ShelfView is destroyed.

[James Cook, 2017/03/10 16:56:01]

nit: I would add a comment either here or in the header about the cases I'm
worried about, esp. monitor disconnect.

+                 sender, base::Passed(ui::Event::Clone(event)), ink_drop));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1607,26 +1594,34 @@ void ShelfView::ShelfItemMoved(int start_index, int target_index) {
 void ShelfView::OnSetShelfItemDelegate(ShelfID id,
                                        ShelfItemDelegate* item_delegate) {}
 
-bool ShelfView::ShowListMenuForView(const ShelfItem& item,
-                                    views::View* source,
-                                    const ui::Event& event,
-                                    views::InkDrop* ink_drop) {
-  ShelfItemDelegate* item_delegate = model_->GetShelfItemDelegate(item.id);
-  ShelfAppMenuItemList items = item_delegate->GetAppMenuItems(event.flags());
-
-  // The application list menu should only show for two or more items; return
-  // false here to ensure that other behavior is triggered (eg. activating or
-  // minimizing a single associated window, or launching a pinned shelf item).
-  if (items.size() < 2)
-    return false;
-
-  ink_drop->AnimateToState(views::InkDropState::ACTIVATED);
-  context_menu_id_ = item.id;
-  ShowMenu(base::MakeUnique<ShelfApplicationMenuModel>(
-               item.title, std::move(items), item_delegate),
-           source, gfx::Point(), false, ui::GetMenuSourceTypeForEvent(event),
-           ink_drop);
-  return true;
+void ShelfView::AfterItemSelected(const ShelfItem& item,
+                                  views::Button* sender,
+                                  std::unique_ptr<ui::Event> event,
+                                  views::InkDrop* ink_drop,
+                                  ShelfAction action,
+                                  ShelfItemDelegate::MenuItemList menu_items) {
+  const ui::Event& e = *event.get();

[James Cook, 2017/03/09 01:09:46]

how about evt or event (vs. event_in) vs. inline *event below.

[msw, 2017/03/10 06:17:56]

Done.

+  shelf_button_pressed_metric_tracker_.ButtonPressed(e, sender, action);
+
+  // Show the app menu if there are 2 or more items and no window was created.
+  if (action != SHELF_ACTION_NEW_WINDOW_CREATED &&
+      action != SHELF_ACTION_APP_LIST_SHOWN && menu_items.size() > 1) {
+    ink_drop->AnimateToState(views::InkDropState::ACTIVATED);
+    context_menu_id_ = item.id;
+    ShowMenu(base::MakeUnique<ShelfApplicationMenuModel>(
+                 item.title, std::move(menu_items),
+                 model_->GetShelfItemDelegate(item.id)),
+             sender, gfx::Point(), false, ui::GetMenuSourceTypeForEvent(e),
+             ink_drop);
+  } else if (action != SHELF_ACTION_APP_LIST_SHOWN) {
+    // For the app list menu no TRIGGERED ink drop effect is needed and it
+    // handles its own ACTIVATED/DEACTIVATED states.
+    ink_drop->AnimateToState(views::InkDropState::ACTION_TRIGGERED);
+  }

[James Cook, 2017/03/09 01:09:46]

I'm having a hard time following the booleans here. Is it possible to write this
in terms of positive comparisons, like if (action == NEW_WINDOW) {} else if
(action == APP_LIST_SHOWN) ... maybe with other helper functions?

[msw, 2017/03/10 06:17:56]

Done. Using a nested if block.

+  // Allow the menu to clear |scoped_root_window_for_new_windows_| during
+  // OnMenuClosed.
+  if (!IsShowingMenu())
+    scoped_root_window_for_new_windows_.reset();
 }
 
 void ShelfView::ShowContextMenuForView(views::View* source,