Close the media picker window when DesktopMediaPickerController is destructed.

Close the media picker window when DesktopMediaPickerController is destructed.

BUG=383999
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277914

[jiayl, 2014-06-13 16:17:48]

Could you take a look?
I don't know why the window is not automatically closed when the controller is
gone. I don't see anything holding a reference of the window. So this is more
like a work around. Please let me know if there is a better fix.

Thanks!

[jiayl, 2014-06-16 16:35:49]

shess@,

could you take a look?

[Scott Hess - ex-Googler, 2014-06-16 17:13:51]

On 2014/06/16 16:35:49, jiayl wrote:
> shess@,
> 
> could you take a look?

Apologies - looks like I wrote a comment, and then forgot to send it.

https://codereview.chromium.org/336833002/diff/1/chrome/browser/ui/cocoa/medi...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/336833002/diff/1/chrome/browser/ui/cocoa/medi...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:84:
[[self window] close];
Since this is being called from a destructor, I would put this _after_ all of
the places where you are disconnecting targets and delegates and stuff.

[Scott Hess - ex-Googler, 2014-06-16 17:15:59]

On 2014/06/16 17:13:51, shess wrote:
> On 2014/06/16 16:35:49, jiayl wrote:
> > shess@,
> > 
> > could you take a look?
> 
> Apologies - looks like I wrote a comment, and then forgot to send it.
> 
>
https://codereview.chromium.org/336833002/diff/1/chrome/browser/ui/cocoa/medi...
> File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
> (right):
> 
>
https://codereview.chromium.org/336833002/diff/1/chrome/browser/ui/cocoa/medi...
> chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:84:
> [[self window] close];
> Since this is being called from a destructor, I would put this _after_ all of
> the places where you are disconnecting targets and delegates and stuff.

To be clear, my concern about calling from a destructor is that we know for sure
that if some event or callback or something did get fired, it would probably
call into code in an unexpected state.  So it's important to make certain that
nothing of the sort can happen.

[jiayl, 2014-06-16 17:16:47]

On 2014/06/16 17:15:59, shess wrote:
> On 2014/06/16 17:13:51, shess wrote:
> > On 2014/06/16 16:35:49, jiayl wrote:
> > > shess@,
> > > 
> > > could you take a look?
> > 
> > Apologies - looks like I wrote a comment, and then forgot to send it.
> > 
> >
>
https://codereview.chromium.org/336833002/diff/1/chrome/browser/ui/cocoa/medi...
> > File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
> > (right):
> > 
> >
>
https://codereview.chromium.org/336833002/diff/1/chrome/browser/ui/cocoa/medi...
> > chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:84:
> > [[self window] close];
> > Since this is being called from a destructor, I would put this _after_ all
of
> > the places where you are disconnecting targets and delegates and stuff.
> 
> To be clear, my concern about calling from a destructor is that we know for
sure
> that if some event or callback or something did get fired, it would probably
> call into code in an unexpected state.  So it's important to make certain that
> nothing of the sort can happen.

Makes sense. Done.

[jiayl, 2014-06-17 15:33:15]

PTAL

[Scott Hess - ex-Googler, 2014-06-17 18:03:49]

lgtm

[jiayl, 2014-06-17 18:07:12]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-06-17 18:09:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/336833002/20001

[commit-bot: I haz the power, 2014-06-18 00:07:31]

Change committed as 277914