Selects last item in a menu when pressing 'up' initially

Selects last item in a menu when pressing 'up' initially

This makes the behavior match native platforms.

BUG=376384
TEST=On linux - press Alt+F, then Up Arrow key, then Enter.
     Expected: The last item "Exit" was selected and invoked, quitting Chrome.

Committed: https://crrev.com/52eb03bee6e55a7fdbbb049a3977a6a2000046ec
Cr-Commit-Position: refs/heads/master@{#339761}

[varkha, 2015-07-13 18:18:18]

Patchset #1 (id:1) has been deleted

[varkha, 2015-07-13 18:18:25]

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

[varkha, 2015-07-13 18:19:33]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org

[varkha, 2015-07-13 18:19:33]

sadrul@, can you please take a look? Noticed another discrepancy following up on
https://codereview.chromium.org/1129203003/.
Thanks!

[commit-bot: I haz the power, 2015-07-13 18:20:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/20001

[varkha, 2015-07-13 18:22:57]

Patchset #1 (id:20001) has been deleted

[varkha, 2015-07-13 18:23:04]

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

[commit-bot: I haz the power, 2015-07-13 18:23:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/40001

[commit-bot: I haz the power, 2015-07-13 18:47:39]

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

[commit-bot: I haz the power, 2015-07-13 18:47:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

(exceeded global retry quota)

[varkha, 2015-07-13 19:00:07]

Patchset #1 (id:40001) has been deleted

[varkha, 2015-07-13 19:00:16]

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

[commit-bot: I haz the power, 2015-07-13 19:02:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/60001

[commit-bot: I haz the power, 2015-07-13 20:41:49]

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

[commit-bot: I haz the power, 2015-07-13 20:41:50]

Dry run: This issue passed the CQ dry run.

[sadrul, 2015-07-14 04:17:41]

https://codereview.chromium.org/1230163004/diff/60001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1230163004/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1907: MenuItemView* to_select =
FindFirstSelectableMenuItem(item);
Hm. Perhaps we could add delta to FindFirstSelectableMenuItem() too, and pass
that from here? That way, we don't have to special case delta==1 here.

FindFirstSelectableMenuItem() is essentially equivalent to
FindNextSelectableMenuItem(index=-1, delta=1), right? Would it be possible to
cleanly merge the two into one?

[varkha, 2015-07-14 16:22:54]

PTAL. I have also added test for a few corner cases. Not sure if it is possible
to simplify the logic but I could replace int delta with an up / down enum if
you think this would make the code easier to read.

https://codereview.chromium.org/1230163004/diff/60001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1230163004/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1907: MenuItemView* to_select =
FindFirstSelectableMenuItem(item);
On 2015/07/14 04:17:41, sadrul wrote:
> Hm. Perhaps we could add delta to FindFirstSelectableMenuItem() too, and pass
> that from here? That way, we don't have to special case delta==1 here.
> 
> FindFirstSelectableMenuItem() is essentially equivalent to
> FindNextSelectableMenuItem(index=-1, delta=1), right? Would it be possible to
> cleanly merge the two into one?

Done.

[varkha, 2015-07-14 16:37:45]

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

[commit-bot: I haz the power, 2015-07-14 16:38:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/80001

[commit-bot: I haz the power, 2015-07-14 16:47:11]

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

[commit-bot: I haz the power, 2015-07-14 16:47:11]

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

(exceeded global retry quota)

[sadrul, 2015-07-15 19:21:53]

On 2015/07/14 16:22:54, varkha wrote:
> ... I could replace int delta with an up / down enum if
> you think this would make the code easier to read.

+1 That would be great!

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1967: bool include_all_items = index +
delta < 0 || index < 0;
Hm, what's the use of |include_all_items|?

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller_unittest.cc:61: TestMenuItemViewShown() :
MenuItemView(nullptr) {}
Can it set |submenu_| here? Would that allow not making the changes (which looks
odd) to CreateSubmenu()?

[varkha, 2015-07-15 19:51:34]

> +1 That would be great!
Sure, will do that. An answer and a question below.

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1967: bool include_all_items = index +
delta < 0 || index < 0;
On 2015/07/15 19:21:52, sadrul wrote:
> Hm, what's the use of |include_all_items|?

Just below. Normally we iterate on all but one current |index|. When
include_all_items is set (this happens for FindInitial calls) we need to
enumerate all items in hope of finding the first enabled (or the last for
reverse iteration).

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller_unittest.cc:61: TestMenuItemViewShown() :
MenuItemView(nullptr) {}
On 2015/07/15 19:21:52, sadrul wrote:
> Can it set |submenu_| here? Would that allow not making the changes (which
looks
> odd) to CreateSubmenu()?

It is probably possible but the way CreateSubmenu is written now doesn't really
allow meaningful overrides, does it (since it sets a private member that a
derived class would not see)? This was my motivation for changing it. Do we want
to preserve ability of a MenuItemView decendants to plug their own
implementations of a submenu?

[varkha, 2015-07-15 22:15:14]

Patchset #3 (id:100001) has been deleted

[varkha, 2015-07-15 22:23:01]

Patchset #3 (id:120001) has been deleted

[varkha, 2015-07-16 16:32:33]

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1230163004/diff/80001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller_unittest.cc:61: TestMenuItemViewShown() :
MenuItemView(nullptr) {}
On 2015/07/15 19:51:34, varkha wrote:
> On 2015/07/15 19:21:52, sadrul wrote:
> > Can it set |submenu_| here? Would that allow not making the changes (which
> looks
> > odd) to CreateSubmenu()?
> 
> It is probably possible but the way CreateSubmenu is written now doesn't
really
> allow meaningful overrides, does it (since it sets a private member that a
> derived class would not see)? This was my motivation for changing it. Do we
want
> to preserve ability of a MenuItemView decendants to plug their own
> implementations of a submenu?

I did that in Patch Set #4. Please see if you like PS3 or PS4 better. I think
submenu factory being virtual would only make sense if |submenu_| is truly
hidden but maybe I am missing something. I could also remove virtual attribute
from the Create/Has/GetSubmenu methods. They are never overridden in current
code.

[varkha, 2015-07-16 16:36:00]

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

[commit-bot: I haz the power, 2015-07-16 16:37:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/160001

[sadrul, 2015-07-19 01:11:49]

lgtm

https://codereview.chromium.org/1230163004/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/menu_item_view.h (right):

https://codereview.chromium.org/1230163004/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/menu_item_view.h:38: class TestMenuItemViewShown;
namespace test?

[varkha, 2015-07-21 19:30:08]

Patchset #6 (id:200001) has been deleted

[varkha, 2015-07-21 19:57:36]

Patchset #6 (id:220001) has been deleted

[varkha, 2015-07-21 20:00:33]

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

[varkha, 2015-07-21 20:00:44]

https://codereview.chromium.org/1230163004/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/menu_item_view.h (right):

https://codereview.chromium.org/1230163004/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/menu_item_view.h:38: class TestMenuItemViewShown;
On 2015/07/19 01:11:49, sadrul wrote:
> namespace test?

Done.

[commit-bot: I haz the power, 2015-07-21 20:00:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/240001

[commit-bot: I haz the power, 2015-07-21 22:14:28]

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

[commit-bot: I haz the power, 2015-07-21 22:14:29]

Dry run: This issue passed the CQ dry run.

[varkha, 2015-07-21 22:14:48]

The CQ bit was checked by varkha@chromium.org

[varkha, 2015-07-21 22:14:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1230163004/#ps240001
(title: "Selects last item in a menu when pressing 'up' initially (nits)")

[commit-bot: I haz the power, 2015-07-21 22:16:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230163004/240001

[commit-bot: I haz the power, 2015-07-21 22:22:32]

Committed patchset #6 (id:240001)

[commit-bot: I haz the power, 2015-07-21 22:24:26]

Patchset 6 (id:??) landed as
https://crrev.com/52eb03bee6e55a7fdbbb049a3977a6a2000046ec
Cr-Commit-Position: refs/heads/master@{#339761}