Merge ShelfItemDelegate::ItemSelected & LauncherItemDelegate::Activate.

ash/common/shelf/shelf_item_delegate.h

 // Copyright 2013 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.
 
 #ifndef ASH_COMMON_SHELF_SHELF_ITEM_DELEGATE_H_
 #define ASH_COMMON_SHELF_SHELF_ITEM_DELEGATE_H_
 
 #include "ash/ash_export.h"
 #include "ash/public/cpp/shelf_application_menu_item.h"
-
-namespace ui {
-class Event;
-}
+#include "ash/public/cpp/shelf_types.h"
+#include "ui/events/event_constants.h"
 
 namespace ash {
 
 // Delegate for the ShelfItem.
 class ASH_EXPORT ShelfItemDelegate {
  public:
-  // The return type for the ShelfItemDelegate::ItemSelected method.
-  enum PerformedAction {
-    // No action was taken.
-    kNoAction,
-    // A new window was created.
-    kNewWindowCreated,
-    // An existing window which was not currently active was activated.
-    kExistingWindowActivated,
-    // The currently active window was minimized.
-    kExistingWindowMinimized,
-    // The app list launcher menu was shown.
-    kAppListMenuShown,
-  };
-
   virtual ~ShelfItemDelegate() {}
 
-  // Invoked when the user clicks on a window entry in the launcher.
-  // |event| is the click event. The |event| is dispatched by a view
-  // and has an instance of |views::View| as the event target
-  // but not |aura::Window|. If the |event| is of type KeyEvent, it is assumed
-  // that this was triggered by keyboard action (Alt+<number>) and special
-  // handling might happen.
+  // Called when the user selects a shelf item. The event type and flags, the
+  // relevant display id, and the source of the selection are sometimes known.

[James Cook, 2017/02/15 00:44:39]

uh, "sometimes known"? Maybe tell the reader which fields are optional and which
aren't? And document what the default values should be?

[msw, 2017/02/15 19:59:17]

Heh, I didn't want to enumerate the various codepaths that call this function
and the different levels of context that they have. No fields are strictly
necessary, but when the information is available, it should be provided, since
some shelf item delegate implementations will vary the behavior based on the
arguments. I've revised the comment with suggested defaults.

   // Returns the action performed by selecting the item.
-  virtual PerformedAction ItemSelected(const ui::Event& event) = 0;
+  // TODO(msw): Replace ui::EventType with ui::mojom::EventType.
+  virtual ShelfAction ItemSelected(ui::EventType event_type,
+                                   int event_flags,
+                                   int64_t display_id,

[James Cook, 2017/02/15 00:44:39]

You could also provide two versions, one of which does not take display_id, and
require that it handle that case correctly.

[msw, 2017/02/15 19:59:17]

I'd rather not overload this function; I'm hoping to merge it with
GetAppMenuItems and convert it to mojo asap.

+                                   ShelfLaunchSource source) = 0;
 
   // Returns any application menu items that should appear for this shelf item.
   // |event_flags| specifies the flags of the event which triggered this menu.
   virtual ShelfAppMenuItemList GetAppMenuItems(int event_flags) = 0;
 
   // Closes all windows associated with this item.
   virtual void Close() = 0;
 };
 
 }  // namespace ash
 
 #endif  // ASH_COMMON_SHELF_SHELF_ITEM_DELEGATE_H_