[Mac] Tear down extension-installed bubble before main window closes.

[Mac] Tear down extension-installed bubble before main window closes.

The main window closed before the bubble, so when the bubble tried to
remove itself as a child window, it called a stale reference.

Additionally, revise a couple uses of this pattern to be more robust
by asking the window being closed to remove itself from its current
parent window (which would be nil in this case).

BUG=94172
TEST=see bug, monitor crash server.


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

[Scott Hess - ex-Googler, 2011-08-30 21:17:42]

Looks like someone forgot to replicate all of the skeleton when they copy/pasted
this from another implementation.

Rohit, the change to extension_popup_controller.mm is egregious, but I looked
around and saw a couple cases where the weak pointer could be easily worked
around.  I _think_ this idiom would also handle the case you fixed in issue
40057.

[asargent_no_longer_on_chrome, 2011-08-30 22:24:53]

This seems fine from my perspective, with the caveat that objective c is still
largely a crazy moon language to me and my opinion is not to be trusted.

[Scott Hess - ex-Googler, 2011-08-31 23:36:09]

Randomly rerouting for Mac review.

[Robert Sesek, 2011-09-01 16:20:39]

http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
(right):

http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:111:
// Watch to see if the parent window closes, and if so, close this one.
How about a test for this one?

[Scott Hess - ex-Googler, 2011-09-02 01:19:11]

http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
(right):

http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:111:
// Watch to see if the parent window closes, and if so, close this one.
On 2011/09/01 16:20:39, rsesek wrote:
> How about a test for this one?

Well, I'm fresh out of ideas.  I had a test which asserted that the window
wasn't visible, became visible at -showWindow:, and not visible after -close on
the parent.  Logging shows that it calls the desired method, but it still thinks
it's visible.

I could mock it and verify that the controller's -close is called when the
parent window is closed.  That's kinda the same.

[Robert Sesek, 2011-09-02 13:41:05]

On 2011/09/02 01:19:11, shess wrote:
>
http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
> File
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
> (right):
> 
>
http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
>
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:111:
> // Watch to see if the parent window closes, and if so, close this one.
> On 2011/09/01 16:20:39, rsesek wrote:
> > How about a test for this one?
> 
> Well, I'm fresh out of ideas.  I had a test which asserted that the window
> wasn't visible, became visible at -showWindow:, and not visible after -close
on
> the parent.  Logging shows that it calls the desired method, but it still
thinks
> it's visible.
> 
> I could mock it and verify that the controller's -close is called when the
> parent window is closed.  That's kinda the same.

Could you upload the test? You've piqued my interest...

[Scott Hess - ex-Googler, 2011-09-03 04:37:54]

On 2011/09/02 13:41:05, rsesek wrote:
> On 2011/09/02 01:19:11, shess wrote:
> >
>
http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
> > File
> chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
> > (right):
> > 
> >
>
http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
> >
>
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:111:
> > // Watch to see if the parent window closes, and if so, close this one.
> > On 2011/09/01 16:20:39, rsesek wrote:
> > > How about a test for this one?
> > 
> > Well, I'm fresh out of ideas.  I had a test which asserted that the window
> > wasn't visible, became visible at -showWindow:, and not visible after -close
> on
> > the parent.  Logging shows that it calls the desired method, but it still
> thinks
> > it's visible.
> > 
> > I could mock it and verify that the controller's -close is called when the
> > parent window is closed.  That's kinda the same.
> 
> Could you upload the test? You've piqued my interest...

It was the InfoBubbleWindow delayed close!  I only noticed it after I thought of
using a mock to monitor the close notification on the child, and it didn't
happen during the test (and things crashed in the testing framework's
window-cleanup code).

I left the notification-watcher in there, but if you think -isVisible is a
sufficient test, I'd be happy enough to remove it.

[Scott Hess - ex-Googler, 2011-09-03 14:07:21]

http://codereview.chromium.org/7740060/diff/8001/chrome/browser/ui/cocoa/exte...
File
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/7740060/diff/8001/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm:228:
addMockObserver:observer name:notification object:bubbleWindow];
I notice that the notifications need to be removed after closing the window...

[Robert Sesek, 2011-09-03 15:19:55]

On Saturday, September 3, 2011,  <shess@chromium.org> wrote:
> On 2011/09/02 13:41:05, rsesek wrote:
>>
>> On 2011/09/02 01:19:11, shess wrote:
>> >
>
>
http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
>>
>> > File
>> chrome/browser/ui/cocoa/extensions/
extension_installed_bubble_controller.mm
>> > (right):
>> >
>> >
>
>
http://codereview.chromium.org/7740060/diff/1/chrome/browser/ui/cocoa/extensi...
>>
>> >
>
> chrome/browser/ui/cocoa/extensions/
extension_installed_bubble_controller.mm:111:
>>
>> > // Watch to see if the parent window closes, and if so, close this one.
>> > On 2011/09/01 16:20:39, rsesek wrote:
>> > > How about a test for this one?
>> >
>> > Well, I'm fresh out of ideas.  I had a test which asserted that the
window
>> > wasn't visible, became visible at -showWindow:, and not visible after
-close
>> on
>> > the parent.  Logging shows that it calls the desired method, but it
still
>> thinks
>> > it's visible.
>> >
>> > I could mock it and verify that the controller's -close is called when
the
>> > parent window is closed.  That's kinda the same.
>
>> Could you upload the test? You've piqued my interest...
>
> It was the InfoBubbleWindow delayed close!

chrome/browser/ui/cocoa/run_loop_testing.h may be helpful here.

http://codereview.chromium.org/7740060/

-- 
rsesek / @chromium.org

[Scott Hess - ex-Googler, 2011-09-06 00:33:44]

On 2011/09/03 15:19:55, rsesek wrote:
> On Saturday, September 3, 2011,  <mailto:shess@chromium.org> wrote:
> > It was the InfoBubbleWindow delayed close!
> 
> chrome/browser/ui/cocoa/run_loop_testing.h may be helpful here.

Apologies - I meant that the InfoBubbleWindow delayed close logic was messing
things up.  It switches between just closing and running an animation, AFAICT
run_loop_testing.h is irrelevant.  Unless I misunderstand things.  Anyhow, the
current CL has a working test, it just turns off delayed close.

> http://codereview.chromium.org/7740060/

[Robert Sesek, 2011-09-06 16:08:06]

LGTM. Thanks for adding a test!

[commit-bot: I haz the power, 2011-09-06 19:14:55]

Change committed as 99784