Fix a crash when chooseDesktopMedia is called with no target tab

Fix a crash when chooseDesktopMedia is called with no target tab

DesktopMediaPicker was getting created with web_contents returned by
web_contents(), which is part of WebContentsObserver interface,
and which does not get set when the target tab is not specified.

Also, fix handling of a case when the method is called when there are
no web contents to attach the picker dialog to.

BUG=375576, 364050
TEST=from a extension with desktopCapture permission:
chrome.desktopCapture.chooseDesktopMedia(['screen'], undefined, function())
does not crash

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

[zel, 2014-05-28 03:09:25]

On 2014/05/28 03:05:22, tbarzic wrote:

Are there any other repro steps here other than this test?

[tbarzic, 2014-05-28 03:17:41]

On 2014/05/28 03:09:25, zel wrote:
> On 2014/05/28 03:05:22, tbarzic wrote:
> 
> Are there any other repro steps here other than this test?

it's enough to call the chooseDesktopMedia with the second arg not set (and the
argument is optional).

[zel, 2014-05-28 03:41:40]

On 2014/05/28 03:17:41, tbarzic wrote:
> On 2014/05/28 03:09:25, zel wrote:
> > On 2014/05/28 03:05:22, tbarzic wrote:
> > 
> > Are there any other repro steps here other than this test?
> 
> it's enough to call the chooseDesktopMedia with the second arg not set (and
the
> argument is optional).

Hmm, maybe we should just pop-up a regular (window or system modal) dialog in
such cases. What do you say Sergey?

[tbarzic, 2014-05-28 03:49:18]

On 2014/05/28 03:41:40, zel wrote:
> On 2014/05/28 03:17:41, tbarzic wrote:
> > On 2014/05/28 03:09:25, zel wrote:
> > > On 2014/05/28 03:05:22, tbarzic wrote:
> > > 
> > > Are there any other repro steps here other than this test?
> > 
> > it's enough to call the chooseDesktopMedia with the second arg not set (and
> the
> > argument is optional).
> 
> Hmm, maybe we should just pop-up a regular (window or system modal) dialog in
> such cases. What do you say Sergey?

I updated the description with small explanation. Main problem was that
web_contents() does not return the web contents associated with the extension
function, but the web_contents that's being observed by the
DesktopCaptureChooseDesktopMediaFunction.

Another issue could arise if there are no open tabs when the function gets
called (e.g. from a  page), in which case web contents will be NULL.

[zel, 2014-05-28 04:06:34]

https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:142: error_
= kNoTabsAvailableError;
I still think we could support this scenario as well if the UI displays system
modal dialog in the case when parent_window and web_contents are NULL. I am not
sure that we have real-life use case of that tough.

[Sergey Ulanov, 2014-06-01 01:31:38]

Toni, thanks for fixing this issue. I think ideally we should also support the
case when the API is called from a background page.

https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:142: error_
= kNoTabsAvailableError;
On 2014/05/28 04:06:34, zel wrote:
> I still think we could support this scenario as well if the UI displays system
> modal dialog in the case when parent_window and web_contents are NULL. I am
not
> sure that we have real-life use case of that tough.

I agree. I think in this case we should pass null parent_web_contents to
DesktopMediaPickerDialog and then it should use
DialogDelegate::CreateDialogWidget() to create dialog instead of
WebContentsModalDialogManager to show the dialog (the way it worked before
https://codereview.chromium.org/281383008/diff/90001/chrome/browser/ui/views/...).

[tbarzic, 2014-06-03 01:18:51]

https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:142: error_
= kNoTabsAvailableError;
On 2014/06/01 01:31:38, Sergey Ulanov wrote:
> On 2014/05/28 04:06:34, zel wrote:
> > I still think we could support this scenario as well if the UI displays
system
> > modal dialog in the case when parent_window and web_contents are NULL. I am
> not
> > sure that we have real-life use case of that tough.
> 
> I agree. I think in this case we should pass null parent_web_contents to
> DesktopMediaPickerDialog and then it should use
> DialogDelegate::CreateDialogWidget() to create dialog instead of
> WebContentsModalDialogManager to show the dialog (the way it worked before
>
https://codereview.chromium.org/281383008/diff/90001/chrome/browser/ui/views/...).

We still need to select the parent_window to use if GetAssociatedWebContents
returns NULL. Looking at the code, views seem to handle the situation where
parent_window is NULL well. I'm not sure what should be done on Mac, though

[Sergey Ulanov, 2014-06-03 01:21:26]

On 2014/06/03 01:18:51, tbarzic wrote:
>
https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
> (right):
> 
>
https://codereview.chromium.org/307453010/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:142:
error_
> = kNoTabsAvailableError;
> On 2014/06/01 01:31:38, Sergey Ulanov wrote:
> > On 2014/05/28 04:06:34, zel wrote:
> > > I still think we could support this scenario as well if the UI displays
> system
> > > modal dialog in the case when parent_window and web_contents are NULL. I
am
> > not
> > > sure that we have real-life use case of that tough.
> > 
> > I agree. I think in this case we should pass null parent_web_contents to
> > DesktopMediaPickerDialog and then it should use
> > DialogDelegate::CreateDialogWidget() to create dialog instead of
> > WebContentsModalDialogManager to show the dialog (the way it worked before
> >
>
https://codereview.chromium.org/281383008/diff/90001/chrome/browser/ui/views/...).
> 
> We still need to select the parent_window to use if GetAssociatedWebContents
> returns NULL. Looking at the code, views seem to handle the situation where
> parent_window is NULL well. I'm not sure what should be done on Mac, though

Yes, Mac implementation should handle the case when parent_window=NULL

[Sergey Ulanov, 2014-06-03 01:27:29]

please add BUG=364050.
LGTM. Thanks for fixing it!

https://codereview.chromium.org/307453010/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/307453010/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:522: if
(parent_web_contents) {
nit: add comment to explain what happens here, particularly that the window is
shown differently depending on who calls the API

[tbarzic, 2014-06-03 20:34:11]

+sky as an owner

[zel, 2014-06-03 21:11:36]

lgtm

[sky, 2014-06-03 22:07:20]

https://codereview.chromium.org/307453010/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/307453010/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:147:
parent_window = ash::Shell::GetPrimaryRootWindow();
Why is the primary root the right one here? And as this is async might you get
the wrong thing? Can't you get the information you need at creation time?

[tbarzic, 2014-06-03 22:25:33]

https://codereview.chromium.org/307453010/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/307453010/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:147:
parent_window = ash::Shell::GetPrimaryRootWindow();
On 2014/06/03 22:07:20, sky wrote:
> Why is the primary root the right one here? And as this is async might you get
> the wrong thing? Can't you get the information you need at creation time?

the primary root is used only when there is no web contents associated with the
extension function, e.g. when the function is called from background page, which
doesn't have a window associated with it (and no open browser windows, as
GetAssociatedWebContents will try to find the current browser's active web
contents in this case).
So this is essentially a fallback that's used when there's no other context to
use.
If you have a better option, I'm open to suggestions.

[sky, 2014-06-03 23:09:09]

ic, LGTM then

[tbarzic, 2014-06-04 00:10:02]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-06-04 00:10:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tbarzic@chromium.org/307453010/60001

[commit-bot: I haz the power, 2014-06-04 11:20:26]

Change committed as 274771