mash: Remove shelf app menu item objects.

mash: Remove shelf app menu item objects.

Remove ash::ShelfApplicationMenuItem subclasses.
Add command ids to the base class to identify menu items.
Add ShelfItemDelegate::ExecuteCommand for menu item selection.
Move app menu item object logic into shelf item delegates.

Cleanup:
-Make some BrowserShortcutLauncherItemController functions local.
-Rename ChromeLauncherController::GetAppMenuItemsForTest, nix unused param.
-Simplify ExtensionAppWindowLauncherItemController (icon, registry, etc.)

BUG=557406
TEST=Automated; no shelf app menu behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2716403005
Cr-Commit-Position: refs/heads/master@{#454340}
Committed: https://chromium.googlesource.com/chromium/src/+/08eddf09b057e24f14e20ed473836970d0d8021b

[msw, 2017-02-28 23:34:18]

Description was changed from

==========
mash: Remove shelf app menu item objects.

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

to

==========
mash: Remove shelf app menu item objects.

Remove ash::ShelfApplicationMenuItem subclasses.
Add command ids to the base class to identify menu items.
Add ShelfItemDelegate::ExecuteCommand for menu item selection.
Move app menu item object logic into shelf item delegates.

Cleanup:
-Make some BrowserShortcutLauncherItemController functions local.
-Rename ChromeLauncherController::GetAppMenuItemsForTest, nix unused param.
-Simplify ExtensionAppWindowLauncherItemController (icon, registry, etc.)

BUG=557406
TEST=Automated; no shelf app menu behavior changes.
R=jamescook@chromium.org
==========

[msw, 2017-02-28 23:34:18]

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

[msw, 2017-02-28 23:37:44]

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

[commit-bot: I haz the power, 2017-02-28 23:39:06]

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-01 00:10:44]

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

[commit-bot: I haz the power, 2017-03-01 00:10:44]

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-01 00:31:19]

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

[commit-bot: I haz the power, 2017-03-01 00:32:07]

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-01 01:05:06]

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

[commit-bot: I haz the power, 2017-03-01 01:05:07]

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

[msw, 2017-03-01 17:36:13]

Hey James, please take a look; thanks!

[James Cook, 2017-03-01 19:50:49]

Approach looks good.

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

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:29:
AddSeparator(ui::SPACING_SEPARATOR);
DCHECK(delegate_)?

Or early exit for null delegate_ below?

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

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_controller.cc:77:
base::MakeUnique<ShelfApplicationMenuItem>(0, window.second));
Does 0 mean invalid command, or first command, or both? If it means both it
might be nice to use -1 or something for invalid.

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_delegate.h (right):

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_delegate.h:39: virtual void ExecuteCommand(uint32_t
command_id, int32_t event_flags) = 0;
nit: #include <stdint.h>

Also, event_flags is int, not int32_t above, would be nice to standardize.

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

https://codereview.chromium.org/2716403005/diff/60001/ash/public/cpp/shelf_ap...
ash/public/cpp/shelf_application_menu_item.h:34: uint32_t command_id_;
const?

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:129:
CHECK_LT(app_menu_items_.size(), std::numeric_limits<uint32_t>::max());
Is this useful? It looks like GetRunningApplications() always returns a small
positive integer.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:152:
DCHECK_NE(index, TabStripModel::kNoTab);
Should this early-exit? Is it possible a tab could close while the menu is open?

Also, I think this function could use a comment about the lifetime of
web_contents pointers vs. them existing in app_menu_items_. For example, if a
web contents goes away do we still cache an invalid pointer to it?

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:51:
// Also used as a tab-index flag for browser-specific commands that ignore tabs.
It might be nice to have a separate constant for this, like kAllTabs or kNoTabs.
I got confused below with some of the uses of kMaxItems.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:118:
registrar_.RemoveAll();
This isn't handled automatically by the registrar's destructor? If there's an
ordering issue that deserves a comment.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:213:
if (browser_menu_items_.size() >= kMaxItems)
LOL. I'd hate to meet this power-user. :-)

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:222:
content::WebContents* tab = tab_strip->GetWebContentsAt(tab_index);
I like "tab" -- briefer and clearer!

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:279:
}
Should this wipe the items and registrar?

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h:24:
// Item controller for an browser shortcut.
super nit: while you're here, can you update/clarify this comment? There's only
one browser shortcut, for example. Saying something about the expected menu
types would be nice too.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:183: virtual
ash::ShelfAppMenuItemList GetAppMenuItemsForTest(
optional: Do you know if we prefer ForTest vs ForTesting? I suspect we use both,
but I dunno if we have a preference for new code.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(left):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:42:
SkBitmap bmp = skia::ImageOperations::Resize(
So this stuff isn't necessary any more? Are there highdpi vs. lowdpi issues?

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:19:
class ExtensionAppWindowLauncherItemController
nit: while you're here, could you add a comment about which type of item/icon
this is?

[msw, 2017-03-02 02:43:37]

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

[commit-bot: I haz the power, 2017-03-02 02:44:32]

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-02 02:59:20]

Comments addressed. Please take another look; thanks!

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

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:29:
AddSeparator(ui::SPACING_SEPARATOR);
On 2017/03/01 19:50:48, James Cook wrote:
> DCHECK(delegate_)?
> 
> Or early exit for null delegate_ below?

I added a DCHECK in ExecuteCommand; tests use null to check menu contents
without executing anything.

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

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_controller.cc:77:
base::MakeUnique<ShelfApplicationMenuItem>(0, window.second));
On 2017/03/01 19:50:48, James Cook wrote:
> Does 0 mean invalid command, or first command, or both? If it means both it
> might be nice to use -1 or something for invalid.

I removed this code and left the standard no-menu comment, since this is unused
at the moment. I'll likely want to revise the |window_id_to_title_| map into a
vector of window structures with ids and titles for O(1) indexed access, etc.
Someday I'll be done with the prerequisite work to make this useful again.

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_delegate.h (right):

https://codereview.chromium.org/2716403005/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_delegate.h:39: virtual void ExecuteCommand(uint32_t
command_id, int32_t event_flags) = 0;
On 2017/03/01 19:50:48, James Cook wrote:
> nit: #include <stdint.h>
> 
> Also, event_flags is int, not int32_t above, would be nice to standardize.

Done.

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

https://codereview.chromium.org/2716403005/diff/60001/ash/public/cpp/shelf_ap...
ash/public/cpp/shelf_application_menu_item.h:34: uint32_t command_id_;
On 2017/03/01 19:50:48, James Cook wrote:
> const?

Done.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:129:
CHECK_LT(app_menu_items_.size(), std::numeric_limits<uint32_t>::max());
On 2017/03/01 19:50:48, James Cook wrote:
> Is this useful? It looks like GetRunningApplications() always returns a small
> positive integer.

These checks are indeed a bit paranoid... I doubt we'll ever get so many tabs
per window, browser windows, or app windows as to break these menus. Still, I
figured that explicitly checking (and clarifying the menu's capabilities) is
better than failing in some surprising way later on, but I'll remove this and
other checks if you prefer.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:152:
DCHECK_NE(index, TabStripModel::kNoTab);
On 2017/03/01 19:50:48, James Cook wrote:
> Should this early-exit? Is it possible a tab could close while the menu is
open?
> 
> Also, I think this function could use a comment about the lifetime of
> web_contents pointers vs. them existing in app_menu_items_. For example, if a
> web contents goes away do we still cache an invalid pointer to it?

Done.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:51:
// Also used as a tab-index flag for browser-specific commands that ignore tabs.
On 2017/03/01 19:50:48, James Cook wrote:
> It might be nice to have a separate constant for this, like kAllTabs or
kNoTabs.
> I got confused below with some of the uses of kMaxItems.

Done.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:118:
registrar_.RemoveAll();
On 2017/03/01 19:50:48, James Cook wrote:
> This isn't handled automatically by the registrar's destructor? If there's an
> ordering issue that deserves a comment.

You're right, it's automatically handled; I removed this.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:213:
if (browser_menu_items_.size() >= kMaxItems)
On 2017/03/01 19:50:48, James Cook wrote:
> LOL. I'd hate to meet this power-user. :-)

Haha, yeah; I'm not sure what would break first if a user tried to have 65k+
windows/tabs open. Sadly, this menu won't fit their needs.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:222:
content::WebContents* tab = tab_strip->GetWebContentsAt(tab_index);
On 2017/03/01 19:50:48, James Cook wrote:
> I like "tab" -- briefer and clearer!

Acknowledged.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:279:
}
On 2017/03/01 19:50:48, James Cook wrote:
> Should this wipe the items and registrar?

It doesn't seem too important either way, and I'd have to restructure the code
to do |browser_menu_items_.clear();| and |registrar_.RemoveAll();| in the early
return cases too. I left this and the other menu item arrays intact after
ExecuteCommand; let me know if your preference is to clear them.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h:24:
// Item controller for an browser shortcut.
On 2017/03/01 19:50:48, James Cook wrote:
> super nit: while you're here, can you update/clarify this comment? There's
only
> one browser shortcut, for example. Saying something about the expected menu
> types would be nice too.

Done.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:183: virtual
ash::ShelfAppMenuItemList GetAppMenuItemsForTest(
On 2017/03/01 19:50:48, James Cook wrote:
> optional: Do you know if we prefer ForTest vs ForTesting? I suspect we use
both,
> but I dunno if we have a preference for new code.

Well, I prefer ForTest (it's shorter and conveys the same thing), but our style
guide mentions ForTesting:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Also, ForTesting is also slightly more popular in the codebase: 3,675 files vs.
1,948 files...
So I changed this function to ForTesting and I'll use that from now on, I guess.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc
(left):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.cc:42:
SkBitmap bmp = skia::ImageOperations::Resize(
On 2017/03/01 19:50:48, James Cook wrote:
> So this stuff isn't necessary any more? Are there highdpi vs. lowdpi issues?

AFAICT, MenuItemView correctly clamps the icon's preferred height here:
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_item_view.cc...
Those values are either 32 (size of kMenuCheckIcon) or 16 (kMenuCheckSize); I
believe we are using 32 in practice. So, this code seems to be downscaling the
app icon smaller than necessary. The menu icons look fine (unchanged) for the
apps I tested (Files, Chrome Web Store, Chrome Remote Desktop), albeit that's on
linux-desktop over remote desktop; I didn't do manual testing on a high-dpi
device. I suspect this scaling is superfluous or even detrimental, but it's
tangential to my CL and I can leave it intact, if you prefer.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_item_controller.h:19:
class ExtensionAppWindowLauncherItemController
On 2017/03/01 19:50:48, James Cook wrote:
> nit: while you're here, could you add a comment about which type of item/icon
> this is?

Done for this and ArcAppWindowLauncherItemController; I also slightly updated
AppWindowLauncherItemController's comment.

[commit-bot: I haz the power, 2017-03-02 05:17:16]

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

[commit-bot: I haz the power, 2017-03-02 05:17:21]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-02 15:43:00]

LGTM with nits

Lemme know if you want me to look again, or just go ahead and land if the
changes are simple.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:129:
CHECK_LT(app_menu_items_.size(), std::numeric_limits<uint32_t>::max());
On 2017/03/02 02:59:20, msw wrote:
> On 2017/03/01 19:50:48, James Cook wrote:
> > Is this useful? It looks like GetRunningApplications() always returns a
small
> > positive integer.
> 
> These checks are indeed a bit paranoid... I doubt we'll ever get so many tabs
> per window, browser windows, or app windows as to break these menus. Still, I
> figured that explicitly checking (and clarifying the menu's capabilities) is
> better than failing in some surprising way later on, but I'll remove this and
> other checks if you prefer.

This check implies GetRunningApplications() can loop 4 billion (minus 1) times.
If you're truly concerned about GetRunningApplications() going wild it should
early exit.

These lists end up being menus, which can probably show a max of ~40 items even
on a 30 inch monitor.

Thoughts:
* Check vs. uint16_t (is that what you meant initially?)
* or Check vs. uint8_t
* and/or Early exit loop below

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:279:
}
On 2017/03/02 02:59:20, msw wrote:
> On 2017/03/01 19:50:48, James Cook wrote:
> > Should this wipe the items and registrar?
> 
> It doesn't seem too important either way, and I'd have to restructure the code
> to do |browser_menu_items_.clear();| and |registrar_.RemoveAll();| in the
early
> return cases too. I left this and the other menu item arrays intact after
> ExecuteCommand; let me know if your preference is to clear them.

Does this controller object get deleted when the menu closes? I'm guessing not.

If not, I think we should wipe the array (and especially the notification
listeners).

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

https://codereview.chromium.org/2716403005/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:28: :
ui::SimpleMenuModel(this), items_(std::move(items)), delegate_(delegate) {
I would comment either here or in constructor that it can be null in tests.

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

https://codereview.chromium.org/2716403005/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:58:
CHECK_LT(windows().size(), std::numeric_limits<uint32_t>::max());
ditto re: uint16 vs 8 vs other limit

[msw, 2017-03-02 18:37:48]

Thanks, comments addressed. Feel free to take another look; landing.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:129:
CHECK_LT(app_menu_items_.size(), std::numeric_limits<uint32_t>::max());
On 2017/03/02 15:43:00, James Cook wrote:
> On 2017/03/02 02:59:20, msw wrote:
> > On 2017/03/01 19:50:48, James Cook wrote:
> > > Is this useful? It looks like GetRunningApplications() always returns a
> small
> > > positive integer.
> > 
> > These checks are indeed a bit paranoid... I doubt we'll ever get so many
tabs
> > per window, browser windows, or app windows as to break these menus. Still,
I
> > figured that explicitly checking (and clarifying the menu's capabilities) is
> > better than failing in some surprising way later on, but I'll remove this
and
> > other checks if you prefer.
> 
> This check implies GetRunningApplications() can loop 4 billion (minus 1)
times.
> If you're truly concerned about GetRunningApplications() going wild it should
> early exit.
> 
> These lists end up being menus, which can probably show a max of ~40 items
even
> on a 30 inch monitor.
> 
> Thoughts:
> * Check vs. uint16_t (is that what you meant initially?)
> * or Check vs. uint8_t
> * and/or Early exit loop below

I was checking against uint32_t, because that's the extent of the command id
space, and I'm casting a potentially larger size_t value to that (see checked
cast below). The browser shortcut commands split that space for window and tab
ids (hence uint16_t there). I guess this might be more confusing than useful, so
I removed the check and others like it; the checked_cast should suffice.

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2716403005/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:279:
}
On 2017/03/02 15:43:00, James Cook wrote:
> On 2017/03/02 02:59:20, msw wrote:
> > On 2017/03/01 19:50:48, James Cook wrote:
> > > Should this wipe the items and registrar?
> > 
> > It doesn't seem too important either way, and I'd have to restructure the
code
> > to do |browser_menu_items_.clear();| and |registrar_.RemoveAll();| in the
> early
> > return cases too. I left this and the other menu item arrays intact after
> > ExecuteCommand; let me know if your preference is to clear them.
> 
> Does this controller object get deleted when the menu closes? I'm guessing
not.
> 
> If not, I think we should wipe the array (and especially the notification
> listeners).

Done. (here and for app shortcuts' cached menu item list)

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

https://codereview.chromium.org/2716403005/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_application_menu_model.cc:28: :
ui::SimpleMenuModel(this), items_(std::move(items)), delegate_(delegate) {
On 2017/03/02 15:43:00, James Cook wrote:
> I would comment either here or in constructor that it can be null in tests.

Done.

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

https://codereview.chromium.org/2716403005/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc:58:
CHECK_LT(windows().size(), std::numeric_limits<uint32_t>::max());
On 2017/03/02 15:43:00, James Cook wrote:
> ditto re: uint16 vs 8 vs other limit

Removed.

[msw, 2017-03-02 18:37:55]

The CQ bit was checked by msw@chromium.org

[msw, 2017-03-02 18:37:56]

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/2716403005/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-03-02 18:38:29]

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

[James Cook, 2017-03-02 18:59:32]

still lgtm

[commit-bot: I haz the power, 2017-03-02 19:35:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488479875966100,
"parent_rev": "8c9deb74cf4f697cb0f6bdb589f04085e8d73e63", "commit_rev":
"08eddf09b057e24f14e20ed473836970d0d8021b"}

[commit-bot: I haz the power, 2017-03-02 19:36:00]

Description was changed from

==========
mash: Remove shelf app menu item objects.

Remove ash::ShelfApplicationMenuItem subclasses.
Add command ids to the base class to identify menu items.
Add ShelfItemDelegate::ExecuteCommand for menu item selection.
Move app menu item object logic into shelf item delegates.

Cleanup:
-Make some BrowserShortcutLauncherItemController functions local.
-Rename ChromeLauncherController::GetAppMenuItemsForTest, nix unused param.
-Simplify ExtensionAppWindowLauncherItemController (icon, registry, etc.)

BUG=557406
TEST=Automated; no shelf app menu behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Remove shelf app menu item objects.

Remove ash::ShelfApplicationMenuItem subclasses.
Add command ids to the base class to identify menu items.
Add ShelfItemDelegate::ExecuteCommand for menu item selection.
Move app menu item object logic into shelf item delegates.

Cleanup:
-Make some BrowserShortcutLauncherItemController functions local.
-Rename ChromeLauncherController::GetAppMenuItemsForTest, nix unused param.
-Simplify ExtensionAppWindowLauncherItemController (icon, registry, etc.)

BUG=557406
TEST=Automated; no shelf app menu behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2716403005
Cr-Commit-Position: refs/heads/master@{#454340}
Committed:
https://chromium.googlesource.com/chromium/src/+/08eddf09b057e24f14e20ed47383...
==========

[commit-bot: I haz the power, 2017-03-02 19:36:01]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/08eddf09b057e24f14e20ed47383...