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

ash/common/shelf/shelf_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/common/shelf/shelf_view.h"
 
 #include <algorithm>
 #include <memory>
 
 #include "ash/common/ash_constants.h"
@@ skipping 432 matching lines @@
   // So, it is safe to be checked after handling overflow button.
   if (!ShouldEventActivateButton(sender, event))
     return;
 
   // Record the index for the last pressed shelf item.
   last_pressed_index_ = view_model_->GetIndexOfView(sender);
   DCHECK_LT(-1, last_pressed_index_);
 
   // Place new windows on the same display as the button.
   WmWindow* window = WmWindow::Get(sender->GetWidget()->GetNativeWindow());
   scoped_root_window_for_new_windows_.reset(

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

Do you think this should live in AfterItemSelected()?

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

I don't think so; item selection itself frequently creates new windows, and
those should likely respect this, right?

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

Yeah, makes sense.

       new ScopedRootWindowForNewWindows(window->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.
@@ skipping 12 matching lines @@
     case TYPE_APP_PANEL:
     case TYPE_DIALOG:
       break;
 
     case TYPE_UNDEFINED:
       NOTREACHED() << "ShelfItemType must be set.";
       break;
   }
 
   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));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ShelfView, FocusTraversable implementation:
 
 views::FocusSearch* ShelfView::GetFocusSearch() {
   return focus_search_.get();
 }
 
 views::FocusTraversable* ShelfView::GetFocusTraversableParent() {
@@ skipping 1084 matching lines @@
   // dragged item is moved back to its initial position. AnimateToIdealBounds
   // will be called again when the new item is added to the |view_model_| but
   // at this time the |view_model_| is inconsistent with the |model_|.
   if (!cancelling_drag_model_changed_)
     AnimateToIdealBounds();
 }
 
 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());
+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);
 
-  // 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;
+  // 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,
                                        const gfx::Point& point,
                                        ui::MenuSourceType source_type) {
   last_pressed_index_ = -1;
 
   const ShelfItem* item = ShelfItemForView(source);
   if (!item) {
     WmShell::Get()->ShowContextMenu(point, source_type);
@@ skipping 130 matching lines @@
 
 int ShelfView::CalculateShelfDistance(const gfx::Point& coordinate) const {
   const gfx::Rect bounds = GetBoundsInScreen();
   int distance = wm_shelf_->SelectValueForShelfAlignment(
       bounds.y() - coordinate.y(), coordinate.x() - bounds.right(),
       bounds.x() - coordinate.x());
   return distance > 0 ? distance : 0;
 }
 
 }  // namespace ash