[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

MenuButton::Activate() creates a PressedLock, which is important if it doesn't
call into MenuController code. However, if it is running a menu through the
MenuController and a sibling menu is shown, we need to reset that PressedLock
so that buttons don't remain pressed.  Fix the bug and add a couple tests.

BUG=560852
Committed: https://crrev.com/d2db790c2898559a24402c131d22444c6d0ca4a4
Cr-Commit-Position: refs/heads/master@{#363547}

[Devlin, 2015-12-03 21:55:57]

Description was changed from

==========
Bookmark Bar Fix
BUG=
==========

to

==========
[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

MenuButton::Activate() creates a PressedLock, which is important if it doesn't
call into MenuController code. However, if it is running a menu through the
MenuController and a sibling menu is shown, we need to reset that PressedLock
so that buttons don't remain pressed.  Fix the bug and add a couple tests.

BUG=560852
==========

[Devlin, 2015-12-03 22:16:11]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-12-03 22:16:15]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-12-03 22:16:19]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-12-03 22:56:38]

rdevlin.cronin@chromium.org changed reviewers:
+ sky@chromium.org

[Devlin, 2015-12-03 22:56:38]

Scott, mind taking a look?

[sky, 2015-12-04 00:44:40]

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:134:
listener_->OnMenuButtonClicked(this, menu_position);
Under what case does this not show a menu?

[sky, 2015-12-04 00:45:18]

On 2015/12/04 00:44:40, sky wrote:
>
https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
> File ui/views/controls/button/menu_button.cc (right):
> 
>
https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
> ui/views/controls/button/menu_button.cc:134:
> listener_->OnMenuButtonClicked(this, menu_position);
> Under what case does this not show a menu?

Let me clarify that, under what conditions does the code mentioned not end up in
MenuController?

[Devlin, 2015-12-04 16:43:59]

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:134:
listener_->OnMenuButtonClicked(this, menu_position);
On 2015/12/04 00:44:40, sky wrote:
> Under what case does this not show a menu?

It won't end up in menu controller code if OnMenuButtonClicked doesn't actually
show a ("real") menu.  Looking through the overrides, I found at least two
places this happens:
- AvatarMenuButton: Looks like a custom view is being shown.
- ToolbarActionView (the one I know about for sure): We can show an extension
popup instead of a menu.

The alternative to this that I thought about was just taking the PressedLock in
Activate() out entirely, and requiring anyone that didn't want to show a menu to
instantiate their own pressed lock, but I was a little worried that there was a
case I was missing somewhere that would suddenly break, and it seemed a bit odd,
since the rest of the button state code responding to clicks is pretty much
handled by the button itself.  But I'm on the edge, so if that approach (or one
I haven't thought of) sounds better to you, I'm all for it.

[sky, 2015-12-04 16:56:53]

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:134:
listener_->OnMenuButtonClicked(this, menu_position);
On 2015/12/04 16:43:59, Devlin wrote:
> On 2015/12/04 00:44:40, sky wrote:
> > Under what case does this not show a menu?
> 
> It won't end up in menu controller code if OnMenuButtonClicked doesn't
actually
> show a ("real") menu.  Looking through the overrides, I found at least two
> places this happens:
> - AvatarMenuButton: Looks like a custom view is being shown.
> - ToolbarActionView (the one I know about for sure): We can show an extension
> popup instead of a menu.
> 
> The alternative to this that I thought about was just taking the PressedLock
in
> Activate() out entirely, and requiring anyone that didn't want to show a menu
to
> instantiate their own pressed lock, but I was a little worried that there was
a
> case I was missing somewhere that would suddenly break, and it seemed a bit
odd,
> since the rest of the button state code responding to clicks is pretty much
> handled by the button itself.  But I'm on the edge, so if that approach (or
one
> I haven't thought of) sounds better to you, I'm all for it.

I prefer letting the callsites handle the PressedLock for two reasons:
. We're (hopefully) going to move away from nested message loop menus. The way
we have the pressedlock here won't work in that world.
. It means we don't need an awkward api to reset the lock (SiblingMenuShow).

[Devlin, 2015-12-04 18:04:36]

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1495973002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:134:
listener_->OnMenuButtonClicked(this, menu_position);
On 2015/12/04 16:56:53, sky wrote:
> On 2015/12/04 16:43:59, Devlin wrote:
> > On 2015/12/04 00:44:40, sky wrote:
> > > Under what case does this not show a menu?
> > 
> > It won't end up in menu controller code if OnMenuButtonClicked doesn't
> actually
> > show a ("real") menu.  Looking through the overrides, I found at least two
> > places this happens:
> > - AvatarMenuButton: Looks like a custom view is being shown.
> > - ToolbarActionView (the one I know about for sure): We can show an
extension
> > popup instead of a menu.
> > 
> > The alternative to this that I thought about was just taking the PressedLock
> in
> > Activate() out entirely, and requiring anyone that didn't want to show a
menu
> to
> > instantiate their own pressed lock, but I was a little worried that there
was
> a
> > case I was missing somewhere that would suddenly break, and it seemed a bit
> odd,
> > since the rest of the button state code responding to clicks is pretty much
> > handled by the button itself.  But I'm on the edge, so if that approach (or
> one
> > I haven't thought of) sounds better to you, I'm all for it.
> 
> I prefer letting the callsites handle the PressedLock for two reasons:
> . We're (hopefully) going to move away from nested message loop menus. The way
> we have the pressedlock here won't work in that world.
> . It means we don't need an awkward api to reset the lock (SiblingMenuShow).

Okay, done.

Looking at them more, it seems like we shouldn't even need to update those two
special cases - ToolbarActionView already instantiates a PressedLock, and since
the AvatarMenuButton doesn't enter a nested loop, the pressed lock that exists
doesn't really do anything.  Sound right to you?

[sky, 2015-12-04 20:22:01]

Yes, LGTM

I think the avatar menu should really persist the lock until the bubble it shows
goes away. But that's separate.

[Devlin, 2015-12-05 00:21:02]

Patchset #4 (id:100001) has been deleted

[Devlin, 2015-12-05 00:28:27]

I very sillily forgot to re-run (all the) menu button tests with the last CL. 
Please take a look at the changes.  Sorry for the noise.

https://codereview.chromium.org/1495973002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1495973002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:222: if (state() ==
Button::STATE_HOVERED)
This is kind of awkward, but important since we entered HOVERED state here and,
unlike with a mouse, need to exit it once the gesture ends...

https://codereview.chromium.org/1495973002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button_unittest.cc (right):

https://codereview.chromium.org/1495973002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:301:
EXPECT_EQ(Button::STATE_HOVERED, menu_button_listener.last_source_state());
The state will start in HOVERED in Activate() and is set to PRESSED by menu
code.

[sky, 2015-12-07 16:05:42]

SLGTM

[Devlin, 2015-12-07 18:41:50]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-12-07 18:41:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1495973002/#ps120001
(title: "Test fix")

[commit-bot: I haz the power, 2015-12-07 18:44:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1495973002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1495973002/120001

[commit-bot: I haz the power, 2015-12-07 19:40:27]

Description was changed from

==========
[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

MenuButton::Activate() creates a PressedLock, which is important if it doesn't
call into MenuController code. However, if it is running a menu through the
MenuController and a sibling menu is shown, we need to reset that PressedLock
so that buttons don't remain pressed.  Fix the bug and add a couple tests.

BUG=560852
==========

to

==========
[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

MenuButton::Activate() creates a PressedLock, which is important if it doesn't
call into MenuController code. However, if it is running a menu through the
MenuController and a sibling menu is shown, we need to reset that PressedLock
so that buttons don't remain pressed.  Fix the bug and add a couple tests.

BUG=560852
==========

[commit-bot: I haz the power, 2015-12-07 19:40:28]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-12-07 19:41:23]

Description was changed from

==========
[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

MenuButton::Activate() creates a PressedLock, which is important if it doesn't
call into MenuController code. However, if it is running a menu through the
MenuController and a sibling menu is shown, we need to reset that PressedLock
so that buttons don't remain pressed.  Fix the bug and add a couple tests.

BUG=560852
==========

to

==========
[Views] If a sibling menu is selected, reset the MenuButton's PressedLock

MenuButton::Activate() creates a PressedLock, which is important if it doesn't
call into MenuController code. However, if it is running a menu through the
MenuController and a sibling menu is shown, we need to reset that PressedLock
so that buttons don't remain pressed.  Fix the bug and add a couple tests.

BUG=560852

Committed: https://crrev.com/d2db790c2898559a24402c131d22444c6d0ca4a4
Cr-Commit-Position: refs/heads/master@{#363547}
==========

[commit-bot: I haz the power, 2015-12-07 19:41:24]

Patchset 4 (id:??) landed as
https://crrev.com/d2db790c2898559a24402c131d22444c6d0ca4a4
Cr-Commit-Position: refs/heads/master@{#363547}