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

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

Merge ItemSelected and GetAppMenuItems mojo interface functions.
(return app menu items with the action taken from selection handling)
Remove the non-mojo ash::ShelfItemDelegate base class.

Remove some unused mash pinning and reordering functionality.
(it'll be fixed later, when mash and cash can use the same pattern)

Replace ShelfApplicationMenuItem with the MenuItem mojo struct.
Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
TEST=No Chrome OS shelf item (or app menu) behavior changes; automated.
R=jamescook@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2718563008
Cr-Commit-Position: refs/heads/master@{#456179}
Committed: https://chromium.googlesource.com/chromium/src/+/77414f1023d40d82ec1b636f67a99316b26210be

[msw, 2017-03-03 00:23:01]

Patchset #2 (id:20001) has been deleted

[msw, 2017-03-03 00:23:08]

Patchset #2 (id:40001) has been deleted

[msw, 2017-03-07 21:58:55]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 21:59:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 22:04:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 22:04:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[msw, 2017-03-07 23:35:41]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 23:36:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 00:24:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 00:24:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2017-03-08 01:33:58]

Description was changed from

==========
Convert ShelfItemDelegate to mojom subclass.

BUG=557406
==========

to

==========
Convert ShelfItemDelegate to mojom subclass.

Use the ShelfItemDelegate mojo interface as the common base class.
Combine ItemSelected and GetAppMenuItems (return menu items).
Remove ShelfApplicationMenuItem, use MenuItem mojo struct.

Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
==========

[msw, 2017-03-08 19:28:05]

Description was changed from

==========
Convert ShelfItemDelegate to mojom subclass.

Use the ShelfItemDelegate mojo interface as the common base class.
Combine ItemSelected and GetAppMenuItems (return menu items).
Remove ShelfApplicationMenuItem, use MenuItem mojo struct.

Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
==========

to

==========
Convert ShelfItemDelegate to mojom subclass.

Use the ShelfItemDelegate mojo interface as the common base class.

Merge ItemSelected and GetAppMenuItems in mojo; keep the impl helper.
(return app menu items with the action taken from selection handling)

Remove some unused mash pinning and reordering functionality.
(it'll be fixed later, when mash and cash can use the same pattern)

Repleace ShelfApplicationMenuItem with the MenuItem mojo struct.
Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
TEST=No Chrome OS shelf item (or app menu) behavior changes; automated.
R=jamescook@chromium.org
==========

[msw, 2017-03-08 19:28:05]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2017-03-08 19:41:17]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 19:42:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-03-08 21:29:18]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2017-03-08 22:30:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 22:30:56]

Dry run: This issue passed the CQ dry run.

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

I like the approach.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_item_delegate.h (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_item_delegate.h:26: void
ItemSelectedBySource(ShelfLaunchSource source);
Does this class exist just to provide this method? If so, could the whole thing
go away and everyone just use mojom::ShelfItemDelegate*? Maybe the places that
need the default arguments can have a local helper function. Or this file could
become shelf_item_util with that helper.

I think you added this method because I asked you to. Whoops on my part.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:453: scoped_root_window_for_new_windows_.reset(
Do you think this should live in AfterItemSelected()?

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:491: base::Bind(&ShelfView::AfterItemSelected,
base::Unretained(this), item,
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.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:1603: const ui::Event& e = *event.get();
how about evt or event (vs. event_in) vs. inline *event below.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:1620: }
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?

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:13: NONE,
nit: document these. Everything in mojoms needs to be well documented.

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:27: enum ShelfLaunchSource { UNKNOWN,
APP_LIST, APP_LIST_SEARCH, };
ditto

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

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:56: // The callback reports the action taken
and any app menu items to show.
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.)

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:62: array<MenuItem> menu_items);
I like how you return both pieces of data atomically. Nice docs too.

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:78: skia.mojom.Bitmap image;
Is image optional? How is it used? Can it be a null image?

https://codereview.chromium.org/2718563008/diff/160001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:1987:
items.push_back(ash::mojom::MenuItem::New());
These don't need dummy data?

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:108:
MenuItemList items = GetAppMenuItems(event ? event->flags() : ui::EF_NONE);
optional: maybe just do this in the two cases below that need it?

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:128:
callback.Run(ActivateContent(content), std::move(items));
optional nit: maybe put the result of ActivateContent() in a local var, might be
a little easier to read

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:573: return
ash::SHELF_ACTION_WINDOW_MINIMIZED;
Weird that it was NONE before. Was that a bug?

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:100:
base::Closure run_loop_quit_closure,
optional: you could also take a RunLoop* run_loop and call run_loop->Quit() to
be symmetric with ShelfAction*

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:125:
run_loop.RunUntilIdle();
Shouldn't this be run_loop.Run()?

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc:18: class
ChromeShelfItemDelegate : public ash::ShelfItemDelegate {
drive-by: docs?

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc:20: explicit
ChromeShelfItemDelegate(const std::string& app_id,
drive-by: explicit not needed

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc:33:
callback.Run(ash::SHELF_ACTION_NEW_WINDOW_CREATED, MenuItemList());
Does this actually create a new window for all items? I'm not sure if this item
delegate is for the browser shortcut, apps or both.

[msw, 2017-03-10 01:42:15]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 01:42:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 02:50:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 02:50:57]

Dry run: This issue passed the CQ dry run.

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

Comments addressed. Please take another look, thanks!

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_item_delegate.h (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_item_delegate.h:26: void
ItemSelectedBySource(ShelfLaunchSource source);
On 2017/03/09 01:09:46, James Cook wrote:
> Does this class exist just to provide this method? If so, could the whole
thing
> go away and everyone just use mojom::ShelfItemDelegate*? Maybe the places that
> need the default arguments can have a local helper function. Or this file
could
> become shelf_item_util with that helper.
> 
> I think you added this method because I asked you to. Whoops on my part.

Done.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:453: scoped_root_window_for_new_windows_.reset(
On 2017/03/09 01:09:46, James Cook wrote:
> Do you think this should live in AfterItemSelected()?

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

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:491: base::Bind(&ShelfView::AfterItemSelected,
base::Unretained(this), item,
On 2017/03/09 01:09:46, James Cook wrote:
> 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.

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

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:1603: const ui::Event& e = *event.get();
On 2017/03/09 01:09:46, James Cook wrote:
> how about evt or event (vs. event_in) vs. inline *event below.

Done.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:1620: }
On 2017/03/09 01:09:46, James Cook wrote:
> 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?

Done. Using a nested if block.

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:13: NONE,
On 2017/03/09 01:09:47, James Cook wrote:
> nit: document these. Everything in mojoms needs to be well documented.

Done.

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:27: enum ShelfLaunchSource { UNKNOWN,
APP_LIST, APP_LIST_SEARCH, };
On 2017/03/09 01:09:46, James Cook wrote:
> ditto
> 
> arguably the mojo docs are more important than the c++ docs

Done.

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:56: // The callback reports the action taken
and any app menu items to show.
On 2017/03/09 01:09:47, James Cook wrote:
> 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.)

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

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:62: array<MenuItem> menu_items);
On 2017/03/09 01:09:47, James Cook wrote:
> I like how you return both pieces of data atomically. Nice docs too.

Acknowledged.

https://codereview.chromium.org/2718563008/diff/160001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:78: skia.mojom.Bitmap image;
On 2017/03/09 01:09:47, James Cook wrote:
> Is image optional? How is it used? Can it be a null image?

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.

https://codereview.chromium.org/2718563008/diff/160001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:1987:
items.push_back(ash::mojom::MenuItem::New());
On 2017/03/09 01:09:47, James Cook wrote:
> These don't need dummy data?

Nope! The test only cares that a menu shows; empty labels and null images are 
fine; it just needs two items to show a menu; added a comment.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:108:
MenuItemList items = GetAppMenuItems(event ? event->flags() : ui::EF_NONE);
On 2017/03/09 01:09:47, James Cook wrote:
> optional: maybe just do this in the two cases below that need it?

Done.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:128:
callback.Run(ActivateContent(content), std::move(items));
On 2017/03/09 01:09:47, James Cook wrote:
> optional nit: maybe put the result of ActivateContent() in a local var, might
be
> a little easier to read

Done.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:573: return
ash::SHELF_ACTION_WINDOW_MINIMIZED;
On 2017/03/09 01:09:47, James Cook wrote:
> Weird that it was NONE before. Was that a bug?

Yeah, it seems so... the difference between these values only would have had an
effect on UMA, not the resulting behavior, afaict.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:100:
base::Closure run_loop_quit_closure,
On 2017/03/09 01:09:47, James Cook wrote:
> optional: you could also take a RunLoop* run_loop and call run_loop->Quit() to
> be symmetric with ShelfAction*

Done.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:125:
run_loop.RunUntilIdle();
On 2017/03/09 01:09:47, James Cook wrote:
> Shouldn't this be run_loop.Run()?

Done.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc:18: class
ChromeShelfItemDelegate : public ash::ShelfItemDelegate {
On 2017/03/09 01:09:47, James Cook wrote:
> drive-by: docs?

It's unused at the moment, and the wrong pattern for going forward; removed.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc:20: explicit
ChromeShelfItemDelegate(const std::string& app_id,
On 2017/03/09 01:09:47, James Cook wrote:
> drive-by: explicit not needed

Removed.

https://codereview.chromium.org/2718563008/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc:33:
callback.Run(ash::SHELF_ACTION_NEW_WINDOW_CREATED, MenuItemList());
On 2017/03/09 01:09:47, James Cook wrote:
> Does this actually create a new window for all items? I'm not sure if this
item
> delegate is for the browser shortcut, apps or both.

Unused; removed.

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

LGTM.

nit: I think you should update the CL description since there's no
ShelfItemDelegate subclass anymore.

Nice patch!

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:453: scoped_root_window_for_new_windows_.reset(
On 2017/03/10 06:17:56, msw wrote:
> On 2017/03/09 01:09:46, James Cook wrote:
> > Do you think this should live in AfterItemSelected()?
> 
> I don't think so; item selection itself frequently creates new windows, and
> those should likely respect this, right?

Yeah, makes sense.

https://codereview.chromium.org/2718563008/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:491: base::Bind(&ShelfView::AfterItemSelected,
base::Unretained(this), item,
On 2017/03/10 06:17:56, msw wrote:
> On 2017/03/09 01:09:46, James Cook wrote:
> > 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.
> 
> Added WeakPtr use to skip AfterItemSelected if the ShelfView is destroyed.

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

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:1619: // For the app list menu no TRIGGERED ink
drop effect is needed and it
I think what's confusing me where is that this comment talks about the app list
but the code is running triggered on line 1621. I think this comment should live
up by 1608.

Or the entire if() block from 1608 to 1623 could move into a MaybeShowMenu()
method that allows you to early exit but still cleanup
scoped_root_window_for_new_windows_ in this method.

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/wm_sh...
File ash/common/shelf/wm_shelf.cc (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/wm_sh...
ash/common/shelf/wm_shelf.cc:33: void NoopCallback(ShelfAction,
std::vector<mojom::MenuItemPtr>) {}
btw, I like how you leave out the argument names on functions like this.

https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:86: mojo.common.mojom.String16 label;  // The
string label, may be empty.
Nice docs throughout this file, btw. Details like this "string may be empty" are
really valuable to me.

https://codereview.chromium.org/2718563008/diff/180001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2718563008/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:104:
std::vector<ash::mojom::MenuItemPtr>());
optional: Out of curiosity, WDTY of using an optional array parameter? All these
inlined empty vectors are awkward to read. I think the calls would all end up
like callback.Run(ACTION_TYPE, base::nullopt_t).

Aside: It's annoying C++ won't let you do "using ash::mojom::MenuItemPtr" on a
typedef. :-(

[msw, 2017-03-10 17:52:35]

Description was changed from

==========
Convert ShelfItemDelegate to mojom subclass.

Use the ShelfItemDelegate mojo interface as the common base class.

Merge ItemSelected and GetAppMenuItems in mojo; keep the impl helper.
(return app menu items with the action taken from selection handling)

Remove some unused mash pinning and reordering functionality.
(it'll be fixed later, when mash and cash can use the same pattern)

Repleace ShelfApplicationMenuItem with the MenuItem mojo struct.
Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
TEST=No Chrome OS shelf item (or app menu) behavior changes; automated.
R=jamescook@chromium.org
==========

to

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

Merge ItemSelected and GetAppMenuItems mojo interface functions.
(return app menu items with the action taken from selection handling)
Remove the non-mojo ash::ShelfItemDelegate base class.

Remove some unused mash pinning and reordering functionality.
(it'll be fixed later, when mash and cash can use the same pattern)

Replace ShelfApplicationMenuItem with the MenuItem mojo struct.
Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
TEST=No Chrome OS shelf item (or app menu) behavior changes; automated.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[msw, 2017-03-10 17:52:35]

msw@chromium.org changed reviewers:
+ tsepez@chromium.org

[msw, 2017-03-10 17:54:50]

+Tom for ash/public/interfaces OWNERS review.
James, I'll address your other comments shortly.

[Tom Sepez, 2017-03-10 18:09:12]

The changes themselves LGTM, but we should chat sometime about what prevents
clients from messing with each other's shelf items if the calls to change image
etc. take an easily guessable app ID as a primary key ...

https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:47: SetItemImage(string app_id,
skia.mojom.Bitmap image);
e.g. here.

[msw, 2017-03-10 18:21:06]

On 2017/03/10 18:09:12, Tom Sepez wrote:
> The changes themselves LGTM, but we should chat sometime about what prevents
> clients from messing with each other's shelf items if the calls to change
image
> etc. take an easily guessable app ID as a primary key ...
> 
>
https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
> File ash/public/interfaces/shelf.mojom (right):
> 
>
https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
> ash/public/interfaces/shelf.mojom:47: SetItemImage(string app_id,
> skia.mojom.Bitmap image);
> e.g. here.

That's a very good question; I hadn't thought of that. I suppose we are just
relying on the shelf client (Chrome) to do the right thing (ie. only set each
app's requested title/icon/etc.). Isn't that also true for all other chrome->ash
interaction schemes (wallpaper, volume control, shelf
alignment/auto-hide/pinning, etc.)? Perhaps that's not good enough? Let me know
if you'd like me to file a bug about this general topic. This might be of more
concern once app-specific clients beyond chrome can connect to Ash.

[Tom Sepez, 2017-03-10 18:38:45]

> That's a very good question; I hadn't thought of that. I suppose we are just
> relying on the shelf client (Chrome) to do the right thing (ie. only set each
> app's requested title/icon/etc.). Isn't that also true for all other
chrome->ash
> interaction schemes (wallpaper, volume control, shelf
> alignment/auto-hide/pinning, etc.)? Perhaps that's not good enough? Let me
know
> if you'd like me to file a bug about this general topic. This might be of more
> concern once app-specific clients beyond chrome can connect to Ash.

Yes, please file a bug so we can keep track of the eventual decision.  Thanks.

[msw, 2017-03-10 19:11:48]

On 2017/03/10 18:38:45, Tom Sepez wrote:
> > That's a very good question; I hadn't thought of that. I suppose we are just
> > relying on the shelf client (Chrome) to do the right thing (ie. only set
each
> > app's requested title/icon/etc.). Isn't that also true for all other
> chrome->ash
> > interaction schemes (wallpaper, volume control, shelf
> > alignment/auto-hide/pinning, etc.)? Perhaps that's not good enough? Let me
> know
> > if you'd like me to file a bug about this general topic. This might be of
more
> > concern once app-specific clients beyond chrome can connect to Ash.
> 
> Yes, please file a bug so we can keep track of the eventual decision.  Thanks.

I filed http://crbug.com/700462 - mash: Review security of Chrome/app -> ash
mojo interactions

[msw, 2017-03-10 20:44:45]

Comments addressed; landing.

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/shelf...
ash/common/shelf/shelf_view.cc:1619: // For the app list menu no TRIGGERED ink
drop effect is needed and it
On 2017/03/10 16:56:01, James Cook wrote:
> I think what's confusing me where is that this comment talks about the app
list
> but the code is running triggered on line 1621. I think this comment should
live
> up by 1608.
> 
> Or the entire if() block from 1608 to 1623 could move into a MaybeShowMenu()
> method that allows you to early exit but still cleanup
> scoped_root_window_for_new_windows_ in this method.

Done (moved the comment up and simplified it a bit)

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/wm_sh...
File ash/common/shelf/wm_shelf.cc (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/common/shelf/wm_sh...
ash/common/shelf/wm_shelf.cc:33: void NoopCallback(ShelfAction,
std::vector<mojom::MenuItemPtr>) {}
On 2017/03/10 16:56:01, James Cook wrote:
> btw, I like how you leave out the argument names on functions like this.

Acknowledged.

https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2718563008/diff/180001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:86: mojo.common.mojom.String16 label;  // The
string label, may be empty.
On 2017/03/10 16:56:01, James Cook wrote:
> Nice docs throughout this file, btw. Details like this "string may be empty"
are
> really valuable to me.

Acknowledged.

https://codereview.chromium.org/2718563008/diff/180001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2718563008/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:104:
std::vector<ash::mojom::MenuItemPtr>());
On 2017/03/10 16:56:01, James Cook wrote:
> optional: Out of curiosity, WDTY of using an optional array parameter? All
these
> inlined empty vectors are awkward to read. I think the calls would all end up
> like callback.Run(ACTION_TYPE, base::nullopt_t).
> 
> Aside: It's annoying C++ won't let you do "using ash::mojom::MenuItemPtr" on a
> typedef. :-(

Done; I also added a using statement in launcher_item_controller.h

[msw, 2017-03-10 20:44:53]

The CQ bit was checked by msw@chromium.org

[msw, 2017-03-10 20:44:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2718563008/#ps200001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-03-10 20:45:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 21:58:10]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489178693681940,
"parent_rev": "2e90a7a5c0009a95b5d602ee9a3632edea75afdb", "commit_rev":
"77414f1023d40d82ec1b636f67a99316b26210be"}

[commit-bot: I haz the power, 2017-03-10 21:58:52]

Description was changed from

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

Merge ItemSelected and GetAppMenuItems mojo interface functions.
(return app menu items with the action taken from selection handling)
Remove the non-mojo ash::ShelfItemDelegate base class.

Remove some unused mash pinning and reordering functionality.
(it'll be fixed later, when mash and cash can use the same pattern)

Replace ShelfApplicationMenuItem with the MenuItem mojo struct.
Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
TEST=No Chrome OS shelf item (or app menu) behavior changes; automated.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

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

Merge ItemSelected and GetAppMenuItems mojo interface functions.
(return app menu items with the action taken from selection handling)
Remove the non-mojo ash::ShelfItemDelegate base class.

Remove some unused mash pinning and reordering functionality.
(it'll be fixed later, when mash and cash can use the same pattern)

Replace ShelfApplicationMenuItem with the MenuItem mojo struct.
Add ShelfAction and ShelfLaunchSource mojo enums and traits.

BUG=557406
TEST=No Chrome OS shelf item (or app menu) behavior changes; automated.
R=jamescook@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2718563008
Cr-Commit-Position: refs/heads/master@{#456179}
Committed:
https://chromium.googlesource.com/chromium/src/+/77414f1023d40d82ec1b636f67a9...
==========

[commit-bot: I haz the power, 2017-03-10 21:58:53]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/77414f1023d40d82ec1b636f67a9...