[Mac] LocationBarDecoration MouseUp Fix

[Mac] LocationBarDecoration MouseUp Fix

The decoration should ignore mouse up events if a mouse down event
didn't happened before it

BUG=670621
Review-Url: https://codereview.chromium.org/2588543002
Cr-Commit-Position: refs/heads/master@{#449758}
Committed: https://chromium.googlesource.com/chromium/src/+/450149c5280ffb4ec4e6a5bda73ec42aa49c0fb1

[spqchan, 2016-12-20 01:02:31]

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

[spqchan, 2016-12-20 01:03:01]

Description was changed from

==========
[Mac] LocationBarDecoration MouseUp Fix

The decoration should ignore mouse up events if a mouse down event
didn't happened before it

BUG=
==========

to

==========
[Mac] LocationBarDecoration MouseUp Fix

The decoration should ignore mouse up events if a mouse down event
didn't happened before it

BUG=670621
==========

[spqchan, 2016-12-20 01:03:01]

spqchan@chromium.org changed reviewers:
+ shrike@chromium.org

[commit-bot: I haz the power, 2016-12-20 01:03:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spqchan, 2016-12-20 01:03:09]

PTAL

[commit-bot: I haz the power, 2016-12-20 01:21:17]

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

[commit-bot: I haz the power, 2016-12-20 01:21:18]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-01-09 22:25:49]

On 2016/12/20 01:03:09, spqchan wrote:
> PTAL

Ping, I think I sent this to you during your vacation. Sorry, my bad :)

[shrike, 2017-01-09 23:47:22]

On 2017/01/09 22:25:49, spqchan wrote:
> On 2016/12/20 01:03:09, spqchan wrote:
> > PTAL
> 
> Ping, I think I sent this to you during your vacation. Sorry, my bad :)

I haven't gotten through my backlog of e-mail :-/

I played with it and was still able to get two decorations with mouse over
hover. That happened twice and I was unable to really reproduce it, but I did
notice that if I

1. Click the star
2. Click the folder popup
3. Double-click another decoration

that I was able to get into that state. The double-click doesn't need to be all
that fast - maybe 500ms per click. I'm guessing there's some sort of race
condition.

[shrike, 2017-02-06 21:00:00]

Also, please be sure to add a test to prevent this from regressing in the
future.

[spqchan, 2017-02-07 18:25:09]

On 2017/02/06 21:00:00, shrike wrote:
> Also, please be sure to add a test to prevent this from regressing in the
> future.

Sorry for the delay, I can't seem to reproduce this. Can you send me a
screencast? Thanks!

[shrike, 2017-02-09 01:36:32]

On 2017/02/07 18:25:09, spqchan (SLOW Feb 8-9) wrote:
> On 2017/02/06 21:00:00, shrike wrote:
> > Also, please be sure to add a test to prevent this from regressing in the
> > future.
> 
> Sorry for the delay, I can't seem to reproduce this. Can you send me a
> screencast? Thanks!

I added a screencast of the problem to the bug.

[spqchan, 2017-02-09 08:20:15]

On 2017/02/09 01:36:32, shrike wrote:
> On 2017/02/07 18:25:09, spqchan (SLOW Feb 8-9) wrote:
> > On 2017/02/06 21:00:00, shrike wrote:
> > > Also, please be sure to add a test to prevent this from regressing in the
> > > future.
> > 
> > Sorry for the delay, I can't seem to reproduce this. Can you send me a
> > screencast? Thanks!
> 
> I added a screencast of the problem to the bug.

Awesome thanks! It looks like this issue only occurs with the new Harmony
dialogs.
This is happening because I have not implemented  the omnibox hover & active
states for the new Harmony dialogs so they're not set properly when the bubbles
open/closes. If you look closely, you'll notice that the bookmark's active state
is actually the hover state. 

Do you mind if I do that in a different CL? I'll have to implement the hover and
active states for all of the Harmony dialogs that anchor to the omnibox. 
I'll open a new bug for this

[shrike, 2017-02-09 17:31:47]

On 2017/02/09 08:20:15, spqchan (SLOW Feb 8-9) wrote:
> On 2017/02/09 01:36:32, shrike wrote:
> > On 2017/02/07 18:25:09, spqchan (SLOW Feb 8-9) wrote:
> > > On 2017/02/06 21:00:00, shrike wrote:
> > > > Also, please be sure to add a test to prevent this from regressing in
the
> > > > future.
> > > 
> > > Sorry for the delay, I can't seem to reproduce this. Can you send me a
> > > screencast? Thanks!
> > 
> > I added a screencast of the problem to the bug.
> 
> Awesome thanks! It looks like this issue only occurs with the new Harmony
> dialogs.
> This is happening because I have not implemented  the omnibox hover & active
> states for the new Harmony dialogs so they're not set properly when the
bubbles
> open/closes. If you look closely, you'll notice that the bookmark's active
state
> is actually the hover state. 
> 
> Do you mind if I do that in a different CL? I'll have to implement the hover
and
> active states for all of the Harmony dialogs that anchor to the omnibox. 
> I'll open a new bug for this

If it's a Harmony-only bug then it should be OK to fix it later. When you file
the bug please make it a blocker for the Harmony rollout.

[spqchan, 2017-02-09 18:44:53]

On 2017/02/09 17:31:47, shrike wrote:
> On 2017/02/09 08:20:15, spqchan (SLOW Feb 8-9) wrote:
> > On 2017/02/09 01:36:32, shrike wrote:
> > > On 2017/02/07 18:25:09, spqchan (SLOW Feb 8-9) wrote:
> > > > On 2017/02/06 21:00:00, shrike wrote:
> > > > > Also, please be sure to add a test to prevent this from regressing in
> the
> > > > > future.
> > > > 
> > > > Sorry for the delay, I can't seem to reproduce this. Can you send me a
> > > > screencast? Thanks!
> > > 
> > > I added a screencast of the problem to the bug.
> > 
> > Awesome thanks! It looks like this issue only occurs with the new Harmony
> > dialogs.
> > This is happening because I have not implemented  the omnibox hover & active
> > states for the new Harmony dialogs so they're not set properly when the
> bubbles
> > open/closes. If you look closely, you'll notice that the bookmark's active
> state
> > is actually the hover state. 
> > 
> > Do you mind if I do that in a different CL? I'll have to implement the hover
> and
> > active states for all of the Harmony dialogs that anchor to the omnibox. 
> > I'll open a new bug for this
> 
> If it's a Harmony-only bug then it should be OK to fix it later. When you file
> the bug please make it a blocker for the Harmony rollout.

Awesome, I'll do that now. What label should I use?

[spqchan, 2017-02-10 00:53:46]

On 2017/02/09 18:44:53, spqchan (SLOW Feb 8-9) wrote:
> On 2017/02/09 17:31:47, shrike wrote:
> > On 2017/02/09 08:20:15, spqchan (SLOW Feb 8-9) wrote:
> > > On 2017/02/09 01:36:32, shrike wrote:
> > > > On 2017/02/07 18:25:09, spqchan (SLOW Feb 8-9) wrote:
> > > > > On 2017/02/06 21:00:00, shrike wrote:
> > > > > > Also, please be sure to add a test to prevent this from regressing
in
> > the
> > > > > > future.
> > > > > 
> > > > > Sorry for the delay, I can't seem to reproduce this. Can you send me a
> > > > > screencast? Thanks!
> > > > 
> > > > I added a screencast of the problem to the bug.
> > > 
> > > Awesome thanks! It looks like this issue only occurs with the new Harmony
> > > dialogs.
> > > This is happening because I have not implemented  the omnibox hover &
active
> > > states for the new Harmony dialogs so they're not set properly when the
> > bubbles
> > > open/closes. If you look closely, you'll notice that the bookmark's active
> > state
> > > is actually the hover state. 
> > > 
> > > Do you mind if I do that in a different CL? I'll have to implement the
hover
> > and
> > > active states for all of the Harmony dialogs that anchor to the omnibox. 
> > > I'll open a new bug for this
> > 
> > If it's a Harmony-only bug then it should be OK to fix it later. When you
file
> > the bug please make it a blocker for the Harmony rollout.
> 
> Awesome, I'll do that now. What label should I use?

Ah never mind, looks like the bug already exists: http://crbug.com/678063

[spqchan, 2017-02-10 21:36:11]

spqchan@chromium.org changed reviewers:
+ avi@chromium.org

[spqchan, 2017-02-10 21:36:12]

PTAL

[Avi (use Gerrit), 2017-02-10 21:44:34]

lgtm

[spqchan, 2017-02-10 21:45:21]

On 2017/02/10 21:44:34, Avi wrote:
> lgtm

thanks!

[spqchan, 2017-02-10 21:45:25]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2017-02-10 21:46:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 22:18:18]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486763125108820, "parent_rev":
"29c855766ef0dfcbfd559a233806166769cfee1b", "commit_rev":
"450149c5280ffb4ec4e6a5bda73ec42aa49c0fb1"}

[commit-bot: I haz the power, 2017-02-10 22:18:55]

Description was changed from

==========
[Mac] LocationBarDecoration MouseUp Fix

The decoration should ignore mouse up events if a mouse down event
didn't happened before it

BUG=670621
==========

to

==========
[Mac] LocationBarDecoration MouseUp Fix

The decoration should ignore mouse up events if a mouse down event
didn't happened before it

BUG=670621

Review-Url: https://codereview.chromium.org/2588543002
Cr-Commit-Position: refs/heads/master@{#449758}
Committed:
https://chromium.googlesource.com/chromium/src/+/450149c5280ffb4ec4e6a5bda73e...
==========

[commit-bot: I haz the power, 2017-02-10 22:18:56]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/450149c5280ffb4ec4e6a5bda73e...