Keep reference view pressed while extension actions have a popup

Keep reference view pressed while extension actions have a popup

The reference view for an extension popup is either the
extension action button itself (if the extension action is
visible on the main bar) or the wrench menu/chevron (if the
action is overflowed). When a popup is opened, the reference
view should remain pressed for the duration of the popup.

BUG=414944
BUG=415171
Committed: https://crrev.com/7ff971438e42ae6577ee4d6223485908e597ffac
Cr-Commit-Position: refs/heads/master@{#296057}

[Devlin, 2014-09-18 18:37:42]

Patchset #2 (id:20001) has been deleted

[Devlin, 2014-09-18 18:37:47]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-09-18 18:39:33]

Patchset #1 (id:40001) has been deleted

[Devlin, 2014-09-18 18:40:54]

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

[Devlin, 2014-09-18 18:40:55]

Hey Scott, please take a look when you have time - I'd be interested in your
opinion of the general approach used here.  If it looks good, I'll go ahead and
add some tests. :)

[sky, 2014-09-18 20:02:24]

https://codereview.chromium.org/547303003/diff/60001/ui/views/controls/button...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/547303003/diff/60001/ui/views/controls/button...
ui/views/controls/button/menu_button.h:52: // Increases the |menus_visible_|
count by one and sets the state to be
This is very fragile and not something I want in the views side. Can you isolate
this in the one place that needs it, the extensions code?

[Devlin, 2014-09-18 20:53:51]

https://codereview.chromium.org/547303003/diff/60001/ui/views/controls/button...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/547303003/diff/60001/ui/views/controls/button...
ui/views/controls/button/menu_button.h:52: // Increases the |menus_visible_|
count by one and sets the state to be
On 2014/09/18 20:02:24, sky wrote:
> This is very fragile and not something I want in the views side. Can you
isolate
> this in the one place that needs it, the extensions code?

The problem with isolating all the logic in the extensions code is that it needs
to affect three different buttons - the extension action buttons themselves, and
the two overflow buttons (either the chevron or the wrench menu).  There are a
few ways we could put more of the logic there, though:
- Make a less-fragile interface on CustomButton like ForceState(), and then have
all the logic for that contained in BrowserActionView. (This was actually my
first approach, but I realized we were trying to do this already with
menu_visible_, so went that way instead).
- Make all of these buttons inherit from a FooMenuButton class that allows for
something like this.
- Introduce a delegate of some kind that is consulted before state is actually
set, and register the extension action with the popup as the delegate.

Any of those sound reasonable?  Or is there a cleaner solution?

[sky, 2014-09-18 22:16:56]

Is there a reason all the buttons can't extend MenuButton?

On Thu, Sep 18, 2014 at 1:53 PM,  <rdevlin.cronin@chromium.org> wrote:
>
>
https://codereview.chromium.org/547303003/diff/60001/ui/views/controls/button...
> File ui/views/controls/button/menu_button.h (right):
>
>
https://codereview.chromium.org/547303003/diff/60001/ui/views/controls/button...
> ui/views/controls/button/menu_button.h:52: // Increases the
> |menus_visible_| count by one and sets the state to be
> On 2014/09/18 20:02:24, sky wrote:
>>
>> This is very fragile and not something I want in the views side. Can
>
> you isolate
>>
>> this in the one place that needs it, the extensions code?
>
>
> The problem with isolating all the logic in the extensions code is that
> it needs to affect three different buttons - the extension action
> buttons themselves, and the two overflow buttons (either the chevron or
> the wrench menu).  There are a few ways we could put more of the logic
> there, though:
> - Make a less-fragile interface on CustomButton like ForceState(), and
> then have all the logic for that contained in BrowserActionView. (This
> was actually my first approach, but I realized we were trying to do this
> already with menu_visible_, so went that way instead).
> - Make all of these buttons inherit from a FooMenuButton class that
> allows for something like this.
> - Introduce a delegate of some kind that is consulted before state is
> actually set, and register the extension action with the popup as the
> delegate.
>
> Any of those sound reasonable?  Or is there a cleaner solution?
>
> https://codereview.chromium.org/547303003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2014-09-18 22:21:46]

They do all extend MenuButton (which is how Patch Set 1 works), but then it
means putting (at least some of) the logic in views code.

[sky, 2014-09-19 02:50:31]

Is the button you need to update always supplied to MenuRunner?

On Thu, Sep 18, 2014 at 3:21 PM,  <rdevlin.cronin@chromium.org> wrote:
> They do all extend MenuButton (which is how Patch Set 1 works), but then it
> means putting (at least some of) the logic in views code.
>
> https://codereview.chromium.org/547303003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2014-09-19 17:17:34]

Patchset #2 (id:80001) has been deleted

[Devlin, 2014-09-19 17:47:47]

Patchset #2 (id:100001) has been deleted

[Devlin, 2014-09-19 17:48:58]

On 2014/09/19 02:50:31, sky wrote:
> Is the button you need to update always supplied to MenuRunner?
> 
> On Thu, Sep 18, 2014 at 3:21 PM,  <mailto:rdevlin.cronin@chromium.org> wrote:
> > They do all extend MenuButton (which is how Patch Set 1 works), but then it
> > means putting (at least some of) the logic in views code.
> >
> > https://codereview.chromium.org/547303003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Discussed offline, and came up with the solution to use a lock (which can stack)
to keep the menu button pressed.

Next patch set up.  Take a look, and if it seems reasonable, I'll add some
tests. :)

[sky, 2014-09-19 21:21:59]

I only looked at the ui/views side. I like this route much better.

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

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:137: //SetState(STATE_NORMAL);
Fix these.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:31: class PressedLock {
How about some test coverage here?

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:33: PressedLock(MenuButton* menu_button);
explicit.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:96: void PressedLocked();
nit: IncrementPressedLocked, DecrementPressedLocked.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:216: struct State {
Can you add the lock to this class? It may not be possible because we copy the
thing. If that is the case, then create a class that has a State and a lock and
use that for menu_stack_.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:600: MenuButton* menu_button_;
Is this needed anymore?

[Devlin, 2014-09-19 22:37:49]

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

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:137: //SetState(STATE_NORMAL);
On 2014/09/19 21:21:59, sky wrote:
> Fix these.

Done.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:31: class PressedLock {
On 2014/09/19 21:21:59, sky wrote:
> How about some test coverage here?

Of course.  Just wanted to make sure this approach was sound before writing 'em.
:)

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:33: PressedLock(MenuButton* menu_button);
On 2014/09/19 21:21:59, sky wrote:
> explicit.

Done.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:96: void PressedLocked();
On 2014/09/19 21:21:59, sky wrote:
> nit: IncrementPressedLocked, DecrementPressedLocked.

Done.

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:216: struct State {
On 2014/09/19 21:21:59, sky wrote:
> Can you add the lock to this class? It may not be possible because we copy the
> thing. If that is the case, then create a class that has a State and a lock
and
> use that for menu_stack_.

Since we copy States around every now and then, adding the lock here would be
tricky (as you guessed).  But, if we wanted, we can get away with just having
menu_stack_ be a std::list of std::pair<State, linked_ptr<PressedLock> > and
avoid the class boiler plate. (We'll have to use linked_ptr in some incarnation
if we want to store menu_stack_ as a std::list still.)  Preferences?

https://codereview.chromium.org/547303003/diff/120001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:600: MenuButton* menu_button_;
On 2014/09/19 21:21:59, sky wrote:
> Is this needed anymore?

Actually, no.  Nifty.

[sky, 2014-09-22 16:06:11]

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

https://codereview.chromium.org/547303003/diff/160001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:173: RequestFocus();
Don't you want to ignore pressed/released too?

https://codereview.chromium.org/547303003/diff/160001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:218: if (state() != STATE_DISABLED &&
event->type() == ui::ET_GESTURE_TAP &&
Don't you need to early out here too?

[Devlin, 2014-09-22 16:22:38]

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

https://codereview.chromium.org/547303003/diff/160001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:173: RequestFocus();
On 2014/09/22 16:06:11, sky wrote:
> Don't you want to ignore pressed/released too?

I don't think we do.  I think that we only want to allow locking PRESSED to mean
that state won't be updated from mouse movements, versus also blocking mouse
events that typically invoke an action.  If a view wants to ignore mouse
clicks/gesture taps, I would think that it would do so by another method, e.g.
blocking the run loop and requiring capture (as a menu does).

Also, it would lead to, in my opinion, strange behavior - the menu button that
is clicked does not respond to clicks, but other menu buttons (even those of the
exact same type) do - something which seems very inconsistent.

https://codereview.chromium.org/547303003/diff/160001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:218: if (state() != STATE_DISABLED &&
event->type() == ui::ET_GESTURE_TAP &&
On 2014/09/22 16:06:11, sky wrote:
> Don't you need to early out here too?

See above.

[sky, 2014-09-22 17:02:25]

Ok, LGTM then

[Devlin, 2014-09-22 17:12:31]

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

[commit-bot: I haz the power, 2014-09-22 17:13:21]

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

[commit-bot: I haz the power, 2014-09-22 18:01:37]

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

[commit-bot: I haz the power, 2014-09-22 18:01:39]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Devlin, 2014-09-22 20:25:19]

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

[commit-bot: I haz the power, 2014-09-22 20:26:46]

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

[commit-bot: I haz the power, 2014-09-22 21:45:48]

Committed patchset #5 (id:180001) as 6e69e3ce3d1ab596b571931b028db4a66b7440a2

[commit-bot: I haz the power, 2014-09-22 21:46:38]

Patchset 5 (id:??) landed as
https://crrev.com/7ff971438e42ae6577ee4d6223485908e597ffac
Cr-Commit-Position: refs/heads/master@{#296057}