mash: Cleanup ash shelf application menu code.

mash: Cleanup ash shelf application menu code.

Make chrome push menu state into ash, avoid some ash->chrome delegation.
Make ShelfItemDelegate supply a list of app menu items, not a menu model.
Make ShelfView construct the menu model, not chrome shelf item delegates.
Make ShelfView supply the app name (shelf item title), not each item delegate.

Nix redundant LauncherItemController::GetApplicationList, inclusion of app title item.
Nix app menu item support for leading separators and IsEnabled() (for app titles).
(ShelfView gets the app title from the shelf item when constructing the menu)

Move the item & model classes to ash (item is public, model is 'internal')
Lots of simplification, refactoring, test updates, and cleanup.

TODO: Generalize mojom::ContextMenuItem and eliminate this app menu item.
TODO: Generalize mojom::ShelfItemDelegate::ExecuteCommand for app menu items.

BUG=557406
TEST=Automated; no behavior change for menus shown when clicking shelf items with multiple open windows.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2671923002
Cr-Commit-Position: refs/heads/master@{#448692}
Committed: https://chromium.googlesource.com/chromium/src/+/84352d5d869c9c5a8092f4dc44dab7a6950a3b18

[msw, 2017-02-06 06:26:30]

Patchset #3 (id:40001) has been deleted

[msw, 2017-02-06 09:47:37]

Description was changed from

==========
mash: Cleanup ash shelf application menu code.

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

to

==========
mash: Cleanup ash shelf application menu code.

Make ShelfItemDelegate supply a list of app menu items, not a menu model.
(this works towards pushing menu state into ash, not delegating to chrome)
Remove the now redundant LauncherItemController::GetApplicationList.

Move the item & model classes to ash (item is public, model is 'internal')
Lots of simplification, refactoring, test updates, and cleanup.

BUG=557406
TEST=Automated; no behavior change for menus shown when clicking shelf items
with multiple open windows.
R=jamescook@chromium.org
==========

[msw, 2017-02-06 09:47:38]

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

[msw, 2017-02-06 10:38:07]

Description was changed from

==========
mash: Cleanup ash shelf application menu code.

Make ShelfItemDelegate supply a list of app menu items, not a menu model.
(this works towards pushing menu state into ash, not delegating to chrome)
Remove the now redundant LauncherItemController::GetApplicationList.

Move the item & model classes to ash (item is public, model is 'internal')
Lots of simplification, refactoring, test updates, and cleanup.

BUG=557406
TEST=Automated; no behavior change for menus shown when clicking shelf items
with multiple open windows.
R=jamescook@chromium.org
==========

to

==========
mash: Cleanup ash shelf application menu code.

Make chrome push menu state into ash, avoid some ash->chrome delegation.
Make ShelfItemDelegate supply a list of app menu items, not a menu model.
Make ShelfView construct the menu model, not chrome shelf item delegates.
Make ShelfView supply the app name (shelf item title), not each item delegate.

Nix redundant LauncherItemController::GetApplicationList, inclusion of app title
item.
Nix app menu item support for leading separators and IsEnabled() (for app
titles).
(ShelfView gets the app title from the shelf item when constructing the menu)

Move the item & model classes to ash (item is public, model is 'internal')
Lots of simplification, refactoring, test updates, and cleanup.

TODO: Generalize mojom::ContextMenuItem and eliminate this app menu item.
TODO: Generalize mojom::ShelfItemDelegate::ExecuteCommand for app menu items.

BUG=557406
TEST=Automated; no behavior change for menus shown when clicking shelf items
with multiple open windows.
R=jamescook@chromium.org
==========

[msw, 2017-02-06 10:48:42]

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

[commit-bot: I haz the power, 2017-02-06 10:49:01]

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-02-06 11:39:35]

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

[commit-bot: I haz the power, 2017-02-06 11:39:36]

Dry run: This issue passed the CQ dry run.

[msw, 2017-02-06 15:04:44]

Hey James, please take a look, thanks!

[James Cook, 2017-02-06 17:30:22]

Overall approach looks good. I really like how you separated out the data model
(the items themselves) versus the separators.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:16: const int kInvalidCommandId
= std::numeric_limits<int>::max();
nit: #include <limits>

or use -1

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:19:
"Ash.Shelf.Menu.NumItemsEnabledUponSelection";
I would just inline these histogram names below.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:32:
AddSeparator(ui::SPACING_SEPARATOR);
Do we want this if items_ is empty? In the old code it looks like
launcher_items_ could be empty. If that's no longer true,
DCHECK(!items_.empty());

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:54: return command_id >= 0 &&
static_cast<size_t>(command_id) < items_.size();
optional fyi: size_t{foo} or int{bar} is acceptable style for numeric
conversions these days.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.h:20: // A menu model listing open
applications associated with a shelf item.
optional: A little piece of ASCII art showing what a menu looks like would make
it more obvious what the expected behavior is.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model_unittest.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model_unittest.cc:80: for (size_t i = 0;
i < 3; i++) {
nit: this might be clearer if you unroll the loop

Also, do you think it's worth checking the titles?

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_controller.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_controller.cc:72: for (const auto& window :
window_id_to_title_)
nit: {}

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:1621: // The application list menu should only
show for two or more items.
Why? (I presume because clicking on a 1-item icon means switch?)

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_window_watcher_item_delegate.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_window_watcher_item_delegate.h:16: // ShelfItemDelegate
for the items created by ShelfWindowWatcher.
optional drive-by: Maybe give an example of the sort of shelf item that uses
ShelfWindowWatcher?

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
File ash/common/test/test_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
ash/common/test/test_shelf_item_delegate.cc:31: return
ash::ShelfApplicationMenuItems();
ditto?

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
File ash/common/test/test_shelf_item_delegate.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
ash/common/test/test_shelf_item_delegate.h:9: #include
"base/compiler_specific.h"
drive-by: not needed

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
ash/common/test/test_shelf_item_delegate.h:27: ash::ShelfApplicationMenuItems
GetAppMenuItems(int event_flags) override;
I presume ash:: is needed because of the test namespace?

https://codereview.chromium.org/2671923002/diff/80001/ash/public/cpp/shelf_ap...
File ash/public/cpp/shelf_application_menu_item.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/public/cpp/shelf_ap...
ash/public/cpp/shelf_application_menu_item.h:21: ShelfApplicationMenuItem(const
base::string16 title, const gfx::Image* icon);
nit: Note that null icon means empty/no icon.

https://codereview.chromium.org/2671923002/diff/80001/ash/public/cpp/shelf_ap...
ash/public/cpp/shelf_application_menu_item.h:30: virtual void Execute(int
event_flags);
It seems weird to me to have code in ash/public/cpp that has virtual functions.
That implies to me that either chrome implements it and ash calls it, or ash
implements it and chrome calls it, neither of which is allowed in mash.

Why does this live here?

https://codereview.chromium.org/2671923002/diff/80001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:2018:
items.push_back(base::MakeUnique<ShelfApplicationMenuItem>(title, nullptr));
Given the number of times the (title, nullptr) constructor is used in tests,
WDYT of providing a (title) constructor that uses no icon? It could delegate to
the main constructor.

https://codereview.chromium.org/2671923002/diff/80001/ash/shell/window_watche...
File ash/shell/window_watcher_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/shell/window_watche...
ash/shell/window_watcher_shelf_item_delegate.cc:36: return
ShelfApplicationMenuItems();
Given that the type of ShelfApplicationMenuItems isn't available at this line, I
would add a comment like "No items" or "Empty list".

(This is why I don't like "using" for lists of things, and why I prefer FooList
to Foos. However, both are used extensively in the code base, so I'm OK with
continuing to use SAMIs).

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:115:
WindowList windows_to_close = windows_;
optional thinking out loud: ShelfAppMenuItemList() is shorter than
ShelfApplicationMenuItems() and has the string List() in it, like WindowList.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:126: }
btw, thanks for sorting the functions

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.h
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.h:17:
public ash::ShelfApplicationMenuItem {
super nit: I would reorder the base classes, since SAMI is more important, this
also matches the function order below

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:229:
return controller_->GetAppMenuItems(item, 0 /* event_flags */).size();
int / size_t conversion?

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:230:
}
btw, I really like how you made GetAppMenuItems() just return real items and not
mix up the data model with separators.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:551:
EXPECT_EQ(1U, controller_->GetAppMenuItems(item1, 0).size());  // 1 window
super nit question: Do we prefer 1U or 1u ?

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3240:
// this shuold activate the other tab.
nit: should

[msw, 2017-02-07 08:53:25]

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

[commit-bot: I haz the power, 2017-02-07 08:53:35]

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

[msw, 2017-02-07 09:12:02]

Comments addressed. Please take another look, thanks!

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:16: const int kInvalidCommandId
= std::numeric_limits<int>::max();
On 2017/02/06 17:30:21, James Cook wrote:
> nit: #include <limits>
> 
> or use -1

I included <limits>, negative values sadly doesn't work for SimpleMenuModel
non-separator command ids:
https://cs.chromium.org/chromium/src/ui/base/models/simple_menu_model.cc?l=455

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:19:
"Ash.Shelf.Menu.NumItemsEnabledUponSelection";
On 2017/02/06 17:30:21, James Cook wrote:
> I would just inline these histogram names below.

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:32:
AddSeparator(ui::SPACING_SEPARATOR);
On 2017/02/06 17:30:21, James Cook wrote:
> Do we want this if items_ is empty? In the old code it looks like
> launcher_items_ could be empty. If that's no longer true,
> DCHECK(!items_.empty());

Hmm, there are tests for application menus without any items that I have
persisted, but the production code doesn't allow that to occur. I think we would
still want the title in that case, but it doesn't really seem to matter; I just
had to make sure that we don't add the last spacing separator (line 44) if there
are no items, otherwise the underlying menu code DCHECKS:
https://cs.chromium.org/chromium/src/ui/base/models/simple_menu_model.cc?l=134
Let me know if you want the behavior or testing changed here.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:54: return command_id >= 0 &&
static_cast<size_t>(command_id) < items_.size();
On 2017/02/06 17:30:21, James Cook wrote:
> optional fyi: size_t{foo} or int{bar} is acceptable style for numeric
> conversions these days.

Interesting, but I don't see any uses of that casting style with size_t in
codesearch (but do see uses with int). I think I'll leave this as-is for now.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.h:20: // A menu model listing open
applications associated with a shelf item.
On 2017/02/06 17:30:21, James Cook wrote:
> optional: A little piece of ASCII art showing what a menu looks like would
make
> it more obvious what the expected behavior is.

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model_unittest.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model_unittest.cc:80: for (size_t i = 0;
i < 3; i++) {
On 2017/02/06 17:30:21, James Cook wrote:
> nit: this might be clearer if you unroll the loop
> 
> Also, do you think it's worth checking the titles?

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_controller.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_controller.cc:72: for (const auto& window :
window_id_to_title_)
On 2017/02/06 17:30:21, James Cook wrote:
> nit: {}

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:1621: // The application list menu should only
show for two or more items.
On 2017/02/06 17:30:21, James Cook wrote:
> Why? (I presume because clicking on a 1-item icon means switch?)

Correct; left-clicking a shelf item with no windows should launch it, and
left-clicking an item with just one associated window should activate or
minimize that window, not show a menu. That's the pre-existing behavior; this
just changes the requirement 5 to 2 because GetAppMenuItems no longer includes
the shelf item title and surrounding separators. I expanded the comment.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_window_watcher_item_delegate.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_window_watcher_item_delegate.h:16: // ShelfItemDelegate
for the items created by ShelfWindowWatcher.
On 2017/02/06 17:30:21, James Cook wrote:
> optional drive-by: Maybe give an example of the sort of shelf item that uses
> ShelfWindowWatcher?

Hmm, the current users are the settings window, the task manager, panels, and
mash items (temporarily). Are there any more? (left as-is for now); as that's
not a terribly satisfying list.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
File ash/common/test/test_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
ash/common/test/test_shelf_item_delegate.cc:31: return
ash::ShelfApplicationMenuItems();
On 2017/02/06 17:30:21, James Cook wrote:
> ditto?

ditto (removed ash:: specifier)

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
File ash/common/test/test_shelf_item_delegate.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
ash/common/test/test_shelf_item_delegate.h:9: #include
"base/compiler_specific.h"
On 2017/02/06 17:30:21, James Cook wrote:
> drive-by: not needed

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/test/test_sh...
ash/common/test/test_shelf_item_delegate.h:27: ash::ShelfApplicationMenuItems
GetAppMenuItems(int event_flags) override;
On 2017/02/06 17:30:21, James Cook wrote:
> I presume ash:: is needed because of the test namespace?

Nope, I just missed this; removed.

https://codereview.chromium.org/2671923002/diff/80001/ash/public/cpp/shelf_ap...
File ash/public/cpp/shelf_application_menu_item.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/public/cpp/shelf_ap...
ash/public/cpp/shelf_application_menu_item.h:21: ShelfApplicationMenuItem(const
base::string16 title, const gfx::Image* icon);
On 2017/02/06 17:30:21, James Cook wrote:
> nit: Note that null icon means empty/no icon.

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/public/cpp/shelf_ap...
ash/public/cpp/shelf_application_menu_item.h:30: virtual void Execute(int
event_flags);
On 2017/02/06 17:30:21, James Cook wrote:
> It seems weird to me to have code in ash/public/cpp that has virtual
functions.
> That implies to me that either chrome implements it and ash calls it, or ash
> implements it and chrome calls it, neither of which is allowed in mash.
> 
> Why does this live here?

It is currently implemented by Chrome subclasses, but I aim to change that asap.
I aim to remove this class entirely, and instead use a ContextMenuItem
(generalized to also support application menus) from shelf.mojom. I'll move this
command execution to ShelfItemDelegate's ExecuteCommand function (also
generalized to support application menus in addition to context menus):
https://cs.chromium.org/chromium/src/ash/public/interfaces/shelf.mojom?l=43

https://codereview.chromium.org/2671923002/diff/80001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:2018:
items.push_back(base::MakeUnique<ShelfApplicationMenuItem>(title, nullptr));
On 2017/02/06 17:30:21, James Cook wrote:
> Given the number of times the (title, nullptr) constructor is used in tests,
> WDYT of providing a (title) constructor that uses no icon? It could delegate
to
> the main constructor.

Done, I added a default parameter and updated callers that pass nullptr.

https://codereview.chromium.org/2671923002/diff/80001/ash/shell/window_watche...
File ash/shell/window_watcher_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/shell/window_watche...
ash/shell/window_watcher_shelf_item_delegate.cc:36: return
ShelfApplicationMenuItems();
On 2017/02/06 17:30:21, James Cook wrote:
> Given that the type of ShelfApplicationMenuItems isn't available at this line,
I
> would add a comment like "No items" or "Empty list".
> 
> (This is why I don't like "using" for lists of things, and why I prefer
FooList
> to Foos. However, both are used extensively in the code base, so I'm OK with
> continuing to use SAMIs).

I added comments, renamed to ShelfAppMenuItemList, and I may use vectors of mojo
structs without a 'using' alias in the near future.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:115:
WindowList windows_to_close = windows_;
On 2017/02/06 17:30:21, James Cook wrote:
> optional thinking out loud: ShelfAppMenuItemList() is shorter than
> ShelfApplicationMenuItems() and has the string List() in it, like WindowList.

Done; I could also rename Application to App for all related identifiers if you
think that's an improvement, but I'm expecting this to churn again soon (using a
generalized shelf.mojom ContextMenuItem, as I noted).

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:126: }
On 2017/02/06 17:30:21, James Cook wrote:
> btw, thanks for sorting the functions

Acknowledged.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.h
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.h:17:
public ash::ShelfApplicationMenuItem {
On 2017/02/06 17:30:21, James Cook wrote:
> super nit: I would reorder the base classes, since SAMI is more important,
this
> also matches the function order below

Done.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:229:
return controller_->GetAppMenuItems(item, 0 /* event_flags */).size();
On 2017/02/06 17:30:21, James Cook wrote:
> int / size_t conversion?

Ah, I already inlined this functionality; I have now removed this method.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:230:
}
On 2017/02/06 17:30:22, James Cook wrote:
> btw, I really like how you made GetAppMenuItems() just return real items and
not
> mix up the data model with separators.

Acknowledged; but client-specified menu item lists may likely need to support
listing separators. ash::mojom::ContextMenuItem::Type already has SEPARATOR.

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:551:
EXPECT_EQ(1U, controller_->GetAppMenuItems(item1, 0).size());  // 1 window
On 2017/02/06 17:30:21, James Cook wrote:
> super nit question: Do we prefer 1U or 1u ?

Hmm, my grep foo is weak at the moment, so I don't have satisfying results, but
I support lowercase (it does seem more visibly distinct from numerals, which may
make the number values easier to read?); I changed these.
https://cs.chromium.org/search/?q=%5B0-9%5DU%5B%5Ea-zA-Z0-9%5D&type=cs

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2671923002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3240:
// this shuold activate the other tab.
On 2017/02/06 17:30:22, James Cook wrote:
> nit: should

Done.

[commit-bot: I haz the power, 2017-02-07 10:38:31]

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

[commit-bot: I haz the power, 2017-02-07 10:38:31]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-02-07 16:32:24]

LGTM. Nice patch.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:16: const int kInvalidCommandId
= std::numeric_limits<int>::max();
On 2017/02/07 09:12:00, msw wrote:
> On 2017/02/06 17:30:21, James Cook wrote:
> > nit: #include <limits>
> > 
> > or use -1
> 
> I included <limits>, negative values sadly doesn't work for SimpleMenuModel
> non-separator command ids:
> https://cs.chromium.org/chromium/src/ui/base/models/simple_menu_model.cc?l=455

Acknowledged.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:32:
AddSeparator(ui::SPACING_SEPARATOR);
On 2017/02/07 09:12:00, msw wrote:
> On 2017/02/06 17:30:21, James Cook wrote:
> > Do we want this if items_ is empty? In the old code it looks like
> > launcher_items_ could be empty. If that's no longer true,
> > DCHECK(!items_.empty());
> 
> Hmm, there are tests for application menus without any items that I have
> persisted, but the production code doesn't allow that to occur. I think we
would
> still want the title in that case, but it doesn't really seem to matter; I
just
> had to make sure that we don't add the last spacing separator (line 44) if
there
> are no items, otherwise the underlying menu code DCHECKS:
> https://cs.chromium.org/chromium/src/ui/base/models/simple_menu_model.cc?l=134
> Let me know if you want the behavior or testing changed here.

I would just add a comment somewhere in here that items_ can be empty in tests,
maybe above line 43 with what you just explained to me.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_window_watcher_item_delegate.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_window_watcher_item_delegate.h:16: // ShelfItemDelegate
for the items created by ShelfWindowWatcher.
On 2017/02/07 09:12:01, msw wrote:
> On 2017/02/06 17:30:21, James Cook wrote:
> > optional drive-by: Maybe give an example of the sort of shelf item that uses
> > ShelfWindowWatcher?
> 
> Hmm, the current users are the settings window, the task manager, panels, and
> mash items (temporarily). Are there any more? (left as-is for now); as that's
> not a terribly satisfying list.

<shrug> I like examples, but it's not a big deal.

https://codereview.chromium.org/2671923002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_application_menu_model_unittest.cc (right):

https://codereview.chromium.org/2671923002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_application_menu_model_unittest.cc:67: base::string16
title3 = base::ASCIIToUTF16("title3");
I like how you used different titles. "Vary your data" is one of my favorite
unit testing precepts.

[msw, 2017-02-07 18:19:15]

Comments addressed; thanks!

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_application_menu_model.cc (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:32:
AddSeparator(ui::SPACING_SEPARATOR);
On 2017/02/07 16:32:24, James Cook wrote:
> On 2017/02/07 09:12:00, msw wrote:
> > On 2017/02/06 17:30:21, James Cook wrote:
> > > Do we want this if items_ is empty? In the old code it looks like
> > > launcher_items_ could be empty. If that's no longer true,
> > > DCHECK(!items_.empty());
> > 
> > Hmm, there are tests for application menus without any items that I have
> > persisted, but the production code doesn't allow that to occur. I think we
> would
> > still want the title in that case, but it doesn't really seem to matter; I
> just
> > had to make sure that we don't add the last spacing separator (line 44) if
> there
> > are no items, otherwise the underlying menu code DCHECKS:
> >
https://cs.chromium.org/chromium/src/ui/base/models/simple_menu_model.cc?l=134
> > Let me know if you want the behavior or testing changed here.
> 
> I would just add a comment somewhere in here that items_ can be empty in
tests,
> maybe above line 43 with what you just explained to me.

Done.

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_window_watcher_item_delegate.h (right):

https://codereview.chromium.org/2671923002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_window_watcher_item_delegate.h:16: // ShelfItemDelegate
for the items created by ShelfWindowWatcher.
On 2017/02/07 16:32:24, James Cook wrote:
> On 2017/02/07 09:12:01, msw wrote:
> > On 2017/02/06 17:30:21, James Cook wrote:
> > > optional drive-by: Maybe give an example of the sort of shelf item that
uses
> > > ShelfWindowWatcher?
> > 
> > Hmm, the current users are the settings window, the task manager, panels,
and
> > mash items (temporarily). Are there any more? (left as-is for now); as
that's
> > not a terribly satisfying list.
> 
> <shrug> I like examples, but it's not a big deal.

I added these examples to the class comment.

https://codereview.chromium.org/2671923002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_application_menu_model_unittest.cc (right):

https://codereview.chromium.org/2671923002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_application_menu_model_unittest.cc:67: base::string16
title3 = base::ASCIIToUTF16("title3");
On 2017/02/07 16:32:24, James Cook wrote:
> I like how you used different titles. "Vary your data" is one of my favorite
> unit testing precepts.

Acknowledged.

[msw, 2017-02-07 18:19:24]

The CQ bit was checked by msw@chromium.org

[msw, 2017-02-07 18:19:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2671923002/#ps120001
(title: "Add comments")

[commit-bot: I haz the power, 2017-02-07 18:19:48]

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-02-07 19:27:11]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486491564504920,
"parent_rev": "c097fbdf154063ede201dd70561fec8974eeb227", "commit_rev":
"84352d5d869c9c5a8092f4dc44dab7a6950a3b18"}

[commit-bot: I haz the power, 2017-02-07 19:27:50]

Description was changed from

==========
mash: Cleanup ash shelf application menu code.

Make chrome push menu state into ash, avoid some ash->chrome delegation.
Make ShelfItemDelegate supply a list of app menu items, not a menu model.
Make ShelfView construct the menu model, not chrome shelf item delegates.
Make ShelfView supply the app name (shelf item title), not each item delegate.

Nix redundant LauncherItemController::GetApplicationList, inclusion of app title
item.
Nix app menu item support for leading separators and IsEnabled() (for app
titles).
(ShelfView gets the app title from the shelf item when constructing the menu)

Move the item & model classes to ash (item is public, model is 'internal')
Lots of simplification, refactoring, test updates, and cleanup.

TODO: Generalize mojom::ContextMenuItem and eliminate this app menu item.
TODO: Generalize mojom::ShelfItemDelegate::ExecuteCommand for app menu items.

BUG=557406
TEST=Automated; no behavior change for menus shown when clicking shelf items
with multiple open windows.
R=jamescook@chromium.org
==========

to

==========
mash: Cleanup ash shelf application menu code.

Make chrome push menu state into ash, avoid some ash->chrome delegation.
Make ShelfItemDelegate supply a list of app menu items, not a menu model.
Make ShelfView construct the menu model, not chrome shelf item delegates.
Make ShelfView supply the app name (shelf item title), not each item delegate.

Nix redundant LauncherItemController::GetApplicationList, inclusion of app title
item.
Nix app menu item support for leading separators and IsEnabled() (for app
titles).
(ShelfView gets the app title from the shelf item when constructing the menu)

Move the item & model classes to ash (item is public, model is 'internal')
Lots of simplification, refactoring, test updates, and cleanup.

TODO: Generalize mojom::ContextMenuItem and eliminate this app menu item.
TODO: Generalize mojom::ShelfItemDelegate::ExecuteCommand for app menu items.

BUG=557406
TEST=Automated; no behavior change for menus shown when clicking shelf items
with multiple open windows.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2671923002
Cr-Commit-Position: refs/heads/master@{#448692}
Committed:
https://chromium.googlesource.com/chromium/src/+/84352d5d869c9c5a8092f4dc44da...
==========

[commit-bot: I haz the power, 2017-02-07 19:27:52]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/84352d5d869c9c5a8092f4dc44da...