Mac: Fix crash while dragging a tab alone in a window.

Mac: Fix crash while dragging a tab alone in a window.

Specifically, the crash could be repro'ed as follows (it also occurred under other circumstances as well): Have a window with one or more tabs in it. Create a new window with a single tab. Drag the tab in the new window, and while dragging right-click (and release).

This patch also prevents right-clicks while dragging from stopping the drag.

BUG=23591
TEST=See above.

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

[John Grabowski, 2009-10-05 01:49:29]

This seems a little hacky.

I realize this fixes a crash, so LGTM if you plan to follow up soon.

http://codereview.chromium.org/242139/diff/1/2
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/242139/diff/1/2#newcode635
Line 635: // TODO(viettrungluu): Find a better way to handle the tab
controller's
A thought: would it be cleaner for the new owner to grab the tab before the old
one releases it?  If so the grab (insertTabWithContents:?) could setTarget:self
and we wouldn't be left with a nil self here.

http://codereview.chromium.org/242139/diff/1/2#newcode635
Line 635: // TODO(viettrungluu): Find a better way to handle the tab
controller's
Please file bugs to go along with your TODOs and add bug number to the code

http://codereview.chromium.org/242139/diff/1/4
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/242139/diff/1/4#newcode269
Line 269: // TODO(viettrungluu): We can receive right-mouse-ups (and maybe even
When revisiting this please do 2 things:
- well-document all the funny event paths to help clarify what is going on
- write unit tests so nobody breaks things by accident

[pink (ping after 24hrs), 2009-10-05 16:19:23]

http://codereview.chromium.org/242139/diff/1/2
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/242139/diff/1/2#newcode635
Line 635: // TODO(viettrungluu): Find a better way to handle the tab
controller's
just use tabContentsArray_ and get the index'th controller. That way you don't
have to worry about the view at all. Of course, do that *before* you remove it
from the array (above).

http://codereview.chromium.org/242139/diff/1/3
File chrome/browser/cocoa/tab_view.h (right):

http://codereview.chromium.org/242139/diff/1/3#newcode57
Line 57: @property(readonly, nonatomic) TabController* controller;
no longer necessary

[viettrungluu, 2009-10-05 17:31:14]

Thanks. Please re-review.

http://codereview.chromium.org/242139/diff/1/2
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/242139/diff/1/2#newcode635
Line 635: // TODO(viettrungluu): Find a better way to handle the tab
controller's
On 2009/10/05 01:49:29, John Grabowski wrote:
> A thought: would it be cleaner for the new owner to grab the tab before the
old
> one releases it?  If so the grab (insertTabWithContents:?) could
setTarget:self
> and we wouldn't be left with a nil self here.

It would if that were the way things actually worked. (That seems like the way
things *should* work.) The reality is that the target is never set except in
|-newTab|, and that a new view/controller are created when the tab is dropped
(this isn't necessarily a bad approach). Also, when the tab is removed, at that
point it doesn't have a new owner yet.

http://codereview.chromium.org/242139/diff/1/2#newcode635
Line 635: // TODO(viettrungluu): Find a better way to handle the tab
controller's
On 2009/10/05 01:49:29, John Grabowski wrote:
> Please file bugs to go along with your TODOs and add bug number to the code

Will do.

http://codereview.chromium.org/242139/diff/1/2#newcode635
Line 635: // TODO(viettrungluu): Find a better way to handle the tab
controller's
On 2009/10/05 16:19:23, pink wrote:
> just use tabContentsArray_ and get the index'th controller. That way you don't
> have to worry about the view at all. Of course, do that *before* you remove it
> from the array (above).

Good idea, with s/tabContentsArray_/tabArray_/.

http://codereview.chromium.org/242139/diff/1/3
File chrome/browser/cocoa/tab_view.h (right):

http://codereview.chromium.org/242139/diff/1/3#newcode57
Line 57: @property(readonly, nonatomic) TabController* controller;
On 2009/10/05 16:19:23, pink wrote:
> no longer necessary

Yup.

http://codereview.chromium.org/242139/diff/1/4
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/242139/diff/1/4#newcode269
Line 269: // TODO(viettrungluu): We can receive right-mouse-ups (and maybe even
On 2009/10/05 01:49:29, John Grabowski wrote:
> When revisiting this please do 2 things:
> - well-document all the funny event paths to help clarify what is going on
> - write unit tests so nobody breaks things by accident

Will do.

[pink (ping after 24hrs), 2009-10-05 18:32:06]

lgtm either way, but see comment.

http://codereview.chromium.org/242139/diff/9/10
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/242139/diff/9/10#newcode635
Line 635: // TODO(viettrungluu): [crbug.com/23829] Find a better way to handle
the tab
if you say so. I don't really see why we need to find a better way to deal with
this. Cocoa has this issue throughout. It's probably even ok to unilaterally
clear the target even if it's not us, since if someone changed the target, it
will have broken assumptions about how this class expects to operate with the
tab (recall we set it in the first place and expect it to stay put).

[viettrungluu, 2009-10-05 18:49:09]

http://codereview.chromium.org/242139/diff/9/10
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/242139/diff/9/10#newcode635
Line 635: // TODO(viettrungluu): [crbug.com/23829] Find a better way to handle
the tab
On 2009/10/05 18:32:08, pink wrote:
> if you say so. I don't really see why we need to find a better way to deal
with
> this. Cocoa has this issue throughout. It's probably even ok to unilaterally
> clear the target even if it's not us, since if someone changed the target, it
> will have broken assumptions about how this class expects to operate with the
> tab (recall we set it in the first place and expect it to stay put).

I'd feel warmer and fuzzier (or something like that) if this were cleaner (at
the very least, I don't like the fact that we manually fiddle with the tab
controller's target), but it's pretty low-priority. I do agree that it's okay to
unilaterally clear the target though, so I've made that change.