Consume TAP regardless of menu result to avoid setting HOT state in CustomButton.

Consume TAP regardless of menu result to avoid setting HOT state in CustomButton.

BUG=137704
TEST=Tap on wrench menu and tap out of wrench menu, button returns to normal state.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148375

[flackr, 2012-07-25 00:23:27]

Hi, we need to consume the TAP event regardless of menu result since at the
point that we exit the menu the TAP is stale and we won't get a subsequent
TAP_UP as we expect in CustomButton. PTAL, thanks!

[sky, 2012-07-25 00:48:42]

https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:225: ignore_result(Activate());
You don't need ignore_result here. Are you sure the bug isn't that this should
be if (!Activate()) ?

[flackr, 2012-07-25 01:31:59]

https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:225: ignore_result(Activate());
On 2012/07/25 00:48:42, sky wrote:
> You don't need ignore_result here. Are you sure the bug isn't that this should
> be if (!Activate()) ?

Yes, Activate blocks until the menu completion, after which it will return the
button state to BS_NORMAL which we don't want to change after the fact in
TextButton::OnGestureEvent. Removed ignore_result, done.

[sky, 2012-07-25 04:34:46]

On Tue, Jul 24, 2012 at 6:31 PM,  <flackr@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
> File ui/views/controls/button/menu_button.cc (right):
>
>
https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
> ui/views/controls/button/menu_button.cc:225: ignore_result(Activate());
> On 2012/07/25 00:48:42, sky wrote:
>>
>> You don't need ignore_result here. Are you sure the bug isn't that
>
> this should
>>
>> be if (!Activate()) ?
>
>
> Yes, Activate blocks until the menu completion, after which it will
> return the button state to BS_NORMAL which we don't want to change after
> the fact in TextButton::OnGestureEvent. Removed ignore_result, done.
>
> https://chromiumcodereview.appspot.com/10823004/

If it blocks and shows a menu false is returned. That's why I'm saying
it looks like the conditional is wrong.

  -Scott

[sky, 2012-07-25 04:35:27]

On Tue, Jul 24, 2012 at 9:34 PM, Scott Violet <sky@chromium.org> wrote:
> On Tue, Jul 24, 2012 at 6:31 PM,  <flackr@chromium.org> wrote:
>>
>>
https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
>> File ui/views/controls/button/menu_button.cc (right):
>>
>>
https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
>> ui/views/controls/button/menu_button.cc:225: ignore_result(Activate());
>> On 2012/07/25 00:48:42, sky wrote:
>>>
>>> You don't need ignore_result here. Are you sure the bug isn't that
>>
>> this should
>>>
>>> be if (!Activate()) ?
>>
>>
>> Yes, Activate blocks until the menu completion, after which it will
>> return the button state to BS_NORMAL which we don't want to change after
>> the fact in TextButton::OnGestureEvent. Removed ignore_result, done.
>>
>> https://chromiumcodereview.appspot.com/10823004/
>
> If it blocks and shows a menu false is returned. That's why I'm saying
> it looks like the conditional is wrong.
>
>   -Scott

And yes, the return value is counter intuitive.

  -Scott

[flackr, 2012-07-25 12:27:30]

Ah I see now, sorry. Returning that we didn't consume the latent TAP is actually
the right thing to do in case that would cause a capture as I believe it does
with processing mouse events, but it should return immediately either way as it
has processed the event and button state rather than calling
CustomButton::OnGestureEvent which won't preserve the state.

On 2012/07/25 04:35:27, sky wrote:
> On Tue, Jul 24, 2012 at 9:34 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> > On Tue, Jul 24, 2012 at 6:31 PM,  <mailto:flackr@chromium.org> wrote:
> >>
> >>
>
https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
> >> File ui/views/controls/button/menu_button.cc (right):
> >>
> >>
>
https://chromiumcodereview.appspot.com/10823004/diff/1/ui/views/controls/butt...
> >> ui/views/controls/button/menu_button.cc:225: ignore_result(Activate());
> >> On 2012/07/25 00:48:42, sky wrote:
> >>>
> >>> You don't need ignore_result here. Are you sure the bug isn't that
> >>
> >> this should
> >>>
> >>> be if (!Activate()) ?
> >>
> >>
> >> Yes, Activate blocks until the menu completion, after which it will
> >> return the button state to BS_NORMAL which we don't want to change after
> >> the fact in TextButton::OnGestureEvent. Removed ignore_result, done.
> >>
> >> https://chromiumcodereview.appspot.com/10823004/
> >
> > If it blocks and shows a menu false is returned. That's why I'm saying
> > it looks like the conditional is wrong.
> >
> >   -Scott
> 
> And yes, the return value is counter intuitive.
> 
>   -Scott

[sky, 2012-07-25 16:10:51]

LGTM

[commit-bot: I haz the power, 2012-07-25 16:55:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10823004/5003

[commit-bot: I haz the power, 2012-07-25 18:44:18]

Change committed as 148375