Not to show context menu if there are no visible items to show.

Not to show context menu if there are no visible items to show.

This patch prevents the item-less context menu from being visible. In cr.ui.menu, some menu items may be made hidden by Command.canExecute(). Even if all the menu items were hidden, the menu could be shown and the only the border was visible to user.

BUG=none
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216241

[yoshiki, 2013-08-07 09:36:24]

@arv: Could you take a look? Thanks.

[arv (Not doing code reviews), 2013-08-07 13:51:37]

https://codereview.chromium.org/22154002/diff/3001/ui/webui/resources/js/cr/u...
File ui/webui/resources/js/cr/ui/menu.js (right):

https://codereview.chromium.org/22154002/diff/3001/ui/webui/resources/js/cr/u...
ui/webui/resources/js/cr/ui/menu.js:161: get visibleItemLength() {
Do we really need to know the count? Maybe just do:

get hasVisibleItems() {
  return this.menuItems.some(function(item) {
    return !item.hidden;
  });
}

[yoshiki, 2013-08-07 14:53:02]

@arv: PTAL. Thanks.

https://codereview.chromium.org/22154002/diff/3001/ui/webui/resources/js/cr/u...
File ui/webui/resources/js/cr/ui/menu.js (right):

https://codereview.chromium.org/22154002/diff/3001/ui/webui/resources/js/cr/u...
ui/webui/resources/js/cr/ui/menu.js:161: get visibleItemLength() {
On 2013/08/07 13:51:37, arv wrote:
> Do we really need to know the count? Maybe just do:
> 
> get hasVisibleItems() {
>   return this.menuItems.some(function(item) {
>     return !item.hidden;
>   });
> }

Done. menuItems is an array-like object and doesn't have some() method, so I use
for-loop instead.

[arv (Not doing code reviews), 2013-08-07 15:30:40]

LGTM with nits

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
File ui/webui/resources/js/cr/ui/context_menu_handler.js (right):

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
ui/webui/resources/js/cr/ui/context_menu_handler.js:39: if
(!menu.hasVisibleItem())
hasVisibleItems

(plural form)

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
File ui/webui/resources/js/cr/ui/menu.js (right):

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
ui/webui/resources/js/cr/ui/menu.js:163: for (var i = 0, menuItem; menuItem =
this.menuItems[i]; i++) {
var menuItems = this.menuItems;
for (...

menuItems is a getter that does a querySelector so it is good to cache the
result.

[yoshiki, 2013-08-07 15:45:41]

Thanks for reviewing. Going to commit.

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
File ui/webui/resources/js/cr/ui/context_menu_handler.js (right):

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
ui/webui/resources/js/cr/ui/context_menu_handler.js:39: if
(!menu.hasVisibleItem())
On 2013/08/07 15:30:40, arv wrote:
> hasVisibleItems
> 
> (plural form)

Done.

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
File ui/webui/resources/js/cr/ui/menu.js (right):

https://codereview.chromium.org/22154002/diff/8001/ui/webui/resources/js/cr/u...
ui/webui/resources/js/cr/ui/menu.js:163: for (var i = 0, menuItem; menuItem =
this.menuItems[i]; i++) {
On 2013/08/07 15:30:40, arv wrote:
> var menuItems = this.menuItems;
> for (...
> 
> menuItems is a getter that does a querySelector so it is good to cache the
> result.

Done.

[commit-bot: I haz the power, 2013-08-07 15:46:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/22154002/15001

[commit-bot: I haz the power, 2013-08-07 18:52:31]

Change committed as 216241