[Mac] Update tab close button when user drags outside

[Mac] Update tab close button when user drags outside

When clicking inside a button and dragging the mouse out of the button's
bounds, a button should update its appearance to show that its action
will not fire if the user releases the mouse. This cl fixes a problem
with the tab close button where it would always show the mouse down
state, even if you clicked and dragged outside of it.

This cl also does a little refactoring, moving common init-time code to
a commonInit method (like its HoverCloseButton subclass).


R=avi@chromium.org
BUG=647757
Review-Url: https://codereview.chromium.org/2898343002
Cr-Commit-Position: refs/heads/master@{#476463}
Committed: https://chromium.googlesource.com/chromium/src/+/40f734ce03b4693acb36388b7892dc67f863b65b

[shrike, 2017-05-23 22:28:19]

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

[commit-bot: I haz the power, 2017-05-23 22:29:24]

Dry run: 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-05-23 23:00:18]

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

[commit-bot: I haz the power, 2017-05-23 23:00:18]

Dry run: This issue passed the CQ dry run.

[shrike, 2017-05-24 23:29:47]

Description was changed from

==========
[Mac] Update tab close button when user drags outside

When clicking inside a button and dragging the mouse out of the button's
bounds, a button should update its appearance to show that its action
will not fire if the user releases the mouse. This cl fixes a problem
with the tab close button where it would always show the mouse down
state, even if you clicked and dragged outside of it.

R=avi@chromium.org
BUG=647757
==========

to

==========
[Mac] Update tab close button when user drags outside

When clicking inside a button and dragging the mouse out of the button's
bounds, a button should update its appearance to show that its action
will not fire if the user releases the mouse. This cl fixes a problem
with the tab close button where it would always show the mouse down
state, even if you clicked and dragged outside of it.

This cl also does a little refactoring, moving common init-time code to
a commonInit method (like its HoverCloseButton subclass).


R=avi@chromium.org
BUG=647757
==========

[shrike, 2017-05-24 23:29:55]

avi@ - PTAL

[Avi (use Gerrit), 2017-05-25 03:59:54]

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button.mm
File ui/base/cocoa/hover_button.mm (left):

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button....
ui/base/cocoa/hover_button.mm:50: // http://crbug.com/28220
The crash in this bug seems plausible with your new code; do we still need to
have the same precaution to prevent a regression?

[shrike, 2017-05-25 17:07:22]

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button.mm
File ui/base/cocoa/hover_button.mm (left):

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button....
ui/base/cocoa/hover_button.mm:50: // http://crbug.com/28220
On 2017/05/25 03:59:54, Avi (ping after 24h) wrote:
> The crash in this bug seems plausible with your new code; do we still need to
> have the same precaution to prevent a regression?

The comment says that the button can get freed within the call to [super
mouseDown:], but with my change we no longer call [super mouseDown:]. It seems
like we should no longer need the retain?

[Avi (use Gerrit), 2017-05-25 18:24:24]

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button.mm
File ui/base/cocoa/hover_button.mm (left):

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button....
ui/base/cocoa/hover_button.mm:50: // http://crbug.com/28220
On 2017/05/25 17:07:22, shrike wrote:
> On 2017/05/25 03:59:54, Avi (ping after 24h) wrote:
> > The crash in this bug seems plausible with your new code; do we still need
to
> > have the same precaution to prevent a regression?
> 
> The comment says that the button can get freed within the call to [super
> mouseDown:], but with my change we no longer call [super mouseDown:]. It seems
> like we should no longer need the retain?

Right, but this moves the functionality of [super mouseDown:] here; it doesn't
solve the issue.

The issue is what happens if the user is clicking and tracking the mouse down
while the tab is being animated closed. In that situation, in your new code, you
have the same problem: you're in a loop in -mouseDown: while the object is
getting deleted from under you.

If you work through the repro steps of the bug and it doesn't crash with the new
code, OK, but it looks like it's a valid concern.

[shrike, 2017-05-25 18:31:00]

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button.mm
File ui/base/cocoa/hover_button.mm (left):

https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button....
ui/base/cocoa/hover_button.mm:50: // http://crbug.com/28220
On 2017/05/25 18:24:23, Avi (ping after 24h) wrote:
> Right, but this moves the functionality of [super mouseDown:] here; it doesn't
> solve the issue.
> 
> The issue is what happens if the user is clicking and tracking the mouse down
> while the tab is being animated closed. In that situation, in your new code,
you
> have the same problem: you're in a loop in -mouseDown: while the object is
> getting deleted from under you.
> 
> If you work through the repro steps of the bug and it doesn't crash with the
new
> code, OK, but it looks like it's a valid concern.

I see. I thought the issue was what was happening inside of [super mouseDown:].

I tried working through the repro steps but couldn't figure them out - do they
make sense to you? I'm not sure how to close the tab while I'm clicking down on
the button (if I'm understanding things correctly). I'm fine with putting the
retain (and comment) back, but I'm wondering how to actually repro the release
bug.

[Avi (use Gerrit), 2017-05-25 18:35:18]

On 2017/05/25 18:31:00, shrike wrote:
>
https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button.mm
> File ui/base/cocoa/hover_button.mm (left):
> 
>
https://codereview.chromium.org/2898343002/diff/1/ui/base/cocoa/hover_button....
> ui/base/cocoa/hover_button.mm:50: // http://crbug.com/28220
> On 2017/05/25 18:24:23, Avi (ping after 24h) wrote:
> > Right, but this moves the functionality of [super mouseDown:] here; it
doesn't
> > solve the issue.
> > 
> > The issue is what happens if the user is clicking and tracking the mouse
down
> > while the tab is being animated closed. In that situation, in your new code,
> you
> > have the same problem: you're in a loop in -mouseDown: while the object is
> > getting deleted from under you.
> > 
> > If you work through the repro steps of the bug and it doesn't crash with the
> new
> > code, OK, but it looks like it's a valid concern.
> 
> I see. I thought the issue was what was happening inside of [super
mouseDown:].
> 
> I tried working through the repro steps but couldn't figure them out - do they
> make sense to you? I'm not sure how to close the tab while I'm clicking down
on
> the button (if I'm understanding things correctly). I'm fine with putting the
> retain (and comment) back, but I'm wondering how to actually repro the release
> bug.

My understanding to repro is to make the change they suggest to slow down the
animation speed, start a tab close, and then click and start tracking the (X).

I'd be OK if you just left in the scoped retain, with an appropriate change to
the comment.

[shrike, 2017-05-25 18:37:01]

On 2017/05/25 18:35:18, Avi (ping after 24h) wrote:
> animation speed, start a tab close, and then click and start tracking the (X).

That's what I tried, but no luck (this was with the changes in this cl backed
out).

> I'd be OK if you just left in the scoped retain, with an appropriate change to
> the comment.

Will do.

[shrike, 2017-05-25 18:53:43]

PTAL

[Avi (use Gerrit), 2017-05-25 19:00:21]

lgtm

👍

[shrike, 2017-06-01 22:17:39]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2017-06-01 22:18:18]

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-06-01 22:44:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496355459673030,
"parent_rev": "0d87c5aa9497e9806eaf7e18d46531201ce749d7", "commit_rev":
"34fc21ef6deb3bafac490dcf53d60911b94e46be"}

[commit-bot: I haz the power, 2017-06-01 22:44:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496355459673030,
"parent_rev": "165f26eefed56bdb1955508636948b0eee1a4a59", "commit_rev":
"40f734ce03b4693acb36388b7892dc67f863b65b"}

[commit-bot: I haz the power, 2017-06-01 22:44:38]

Description was changed from

==========
[Mac] Update tab close button when user drags outside

When clicking inside a button and dragging the mouse out of the button's
bounds, a button should update its appearance to show that its action
will not fire if the user releases the mouse. This cl fixes a problem
with the tab close button where it would always show the mouse down
state, even if you clicked and dragged outside of it.

This cl also does a little refactoring, moving common init-time code to
a commonInit method (like its HoverCloseButton subclass).


R=avi@chromium.org
BUG=647757
==========

to

==========
[Mac] Update tab close button when user drags outside

When clicking inside a button and dragging the mouse out of the button's
bounds, a button should update its appearance to show that its action
will not fire if the user releases the mouse. This cl fixes a problem
with the tab close button where it would always show the mouse down
state, even if you clicked and dragged outside of it.

This cl also does a little refactoring, moving common init-time code to
a commonInit method (like its HoverCloseButton subclass).


R=avi@chromium.org
BUG=647757

Review-Url: https://codereview.chromium.org/2898343002
Cr-Commit-Position: refs/heads/master@{#476463}
Committed:
https://chromium.googlesource.com/chromium/src/+/40f734ce03b4693acb36388b7892...
==========

[commit-bot: I haz the power, 2017-06-01 22:44:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/40f734ce03b4693acb36388b7892...