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

ash/public/interfaces/shelf.mojom

Index: ash/public/interfaces/shelf.mojom
diff --git a/ash/public/interfaces/shelf.mojom b/ash/public/interfaces/shelf.mojom
index 020a5577d5bf1cfe4b37ce6f58b9293df4d87b65..305330a0c621e4e8af206cccf49d48486f1e5042 100644
--- a/ash/public/interfaces/shelf.mojom
+++ b/ash/public/interfaces/shelf.mojom
@@ -4,7 +4,18 @@
 
 module ash.mojom;
 
+import "mojo/common/string16.mojom";
 import "skia/public/interfaces/bitmap.mojom";
+import "ui/events/mojo/event.mojom";
+
+// These values match ash::ShelfAction.
+enum ShelfAction {
+  NONE,

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

nit: document these. Everything in mojoms needs to be well documented.

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

Done.

+  WINDOW_CREATED,
+  WINDOW_ACTIVATED,
+  WINDOW_MINIMIZED,
+  APP_LIST_SHOWN,
+};
 
 // These values match ash::ShelfAlignment.
 enum ShelfAlignment { BOTTOM, LEFT, RIGHT, BOTTOM_LOCKED, };
@@ -12,6 +23,9 @@ enum ShelfAlignment { BOTTOM, LEFT, RIGHT, BOTTOM_LOCKED, };
 // These values match ash::ShelfAutoHideBehavior.
 enum ShelfAutoHideBehavior { ALWAYS, NEVER, HIDDEN, };
 
+// These values match ash::ShelfLaunchSource.
+enum ShelfLaunchSource { UNKNOWN, APP_LIST, APP_LIST_SEARCH, };

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

ditto

arguably the mojo docs are more important than the c++ docs

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

Done.

+
 // The Shelf controller allows clients (eg. Chrome) to control the ash shelf.
 interface ShelfController {
   // Observers are immediately notified of the current shelf states when added.
@@ -34,31 +48,35 @@ interface ShelfObserver {
   OnAutoHideBehaviorChanged(ShelfAutoHideBehavior auto_hide, int64 display_id);
 };
 
-// ShelfItemDelegate handles command execution and observes shelf item changes.
+// ShelfItemDelegate handles shelf item selection, menu command execution, etc.
 interface ShelfItemDelegate {
-  // Called when a pinned shelf item is invoked without an open window.
-  LaunchItem();
-
-  // Called on invocation of a shelf item's context menu command.
+  // Called when the user selects a shelf item. The event, display, and source
+  // info should be provided if known; some implementations use these arguments.
+  // Defaults: (nullptr, kInvalidDisplayId, LAUNCH_FROM_UNKNOWN)
+  // The callback reports the action taken and any app menu items to show.

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

Is this used for context menus? Either way it would be nice to say something,
especially since MenuItem below implies it's used for context menus.

(I agree with using the same data structures for context and non-context menus,
I just think the docs need to be super clear that that's how it works.)

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

Done. This and MenuItem are not used for context menus yet.

+  //
+  // TODO(msw): Use the Event location to determine the display id? 
+  ItemSelected(ui.mojom.Event event,
+               int64 display_id,
+               ShelfLaunchSource source) => (ShelfAction action,
+                                             array<MenuItem> menu_items);

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

I like how you return both pieces of data atomically. Nice docs too.

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

Acknowledged.

+
+  // Called on invocation of a shelf item's context or application menu command.
   ExecuteCommand(uint32 command_id, int32 event_flags);
 
-  // Called when a shelf item is pinned or unpinned.
-  ItemPinned();
-  ItemUnpinned();
-
-  // Called when a pinned shelf item is reordered.
-  // |order| is the index of the item on the shelf.
-  ItemReordered(uint32 order);
+  // Closes all windows associated with this shelf item.
+  Close();
 };
 
-// ContextMenuItems may be used to supplement ash shelf item context menus.
-struct ContextMenuItem {
+// MenuItems are used to populate context and application menus for shelf items.
+struct MenuItem {
   enum Type { ITEM, CHECK, RADIO, SEPARATOR, SUBMENU };
 
   Type type;
   uint32 command_id;
-  string? label;
-  array<ContextMenuItem>? submenu;
+  mojo.common.mojom.String16 label;
+  skia.mojom.Bitmap image;

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

Is image optional? How is it used? Can it be a null image?

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

It's not 'optional' in the sense of having a question mark here and being
base::Optional in code, but it definitely supports null SkBitmap objects (that
return true for isNull), added member comments, and mentioned this.

+  array<MenuItem>? submenu;
   bool enabled;
   bool checked;
   uint32 radio_group_id;
@@ -76,5 +94,5 @@ struct ShelfItem {
   skia.mojom.Bitmap image;
 
   // Additional context menu items (eg. 'New Incognito Window').
-  array<ContextMenuItem>? context_menu_items;
+  array<MenuItem>? context_menu_items;
 };