Fix DesktopMediaPickerViews to handle background parent_web_contents

Fix DesktopMediaPickerViews to handle background parent_web_contents

After r283702 DesktopMediaPickerViews is getting parent_web_contents even
when it corresponds to a background extension page.
DesktopMediaPickerViews was always trying to create a modal dialog when
web_contents is specified, and that doesn't work for background pages.
Now it explicitly checks if the web_contents is a background page and
in that case shows the picker in a separate window.

Also updated example for desktop capture API to make it easier to test
this scenario.

BUG=402579
Committed: https://crrev.com/71b32fcc875526596305a98c1a1194e690dc8719
Cr-Commit-Position: refs/heads/master@{#292055}

[Wez, 2014-08-23 21:51:21]

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

https://codereview.chromium.org/501713002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:415: bool modal_dialog =
nit: Suggest has_visible_parent

https://codereview.chromium.org/501713002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:421: widget =
DialogDelegate::CreateDialogWidget(this, context, NULL);
Why do we also want to change the no-parent_web_contents case? Or is it always
NULL if IsNeverVisible is true?

https://codereview.chromium.org/501713002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:429: // If the picker is
not modal then the picker window needs to be filtered.
nit: Clarify what it means to be filtered/why? What are we filtering?

[Sergey Ulanov, 2014-08-25 17:07:46]

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

https://codereview.chromium.org/501713002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:415: bool modal_dialog =
On 2014/08/23 21:51:21, Wez wrote:
> nit: Suggest has_visible_parent

IMO |modal_dialog| makes code below more readable.

https://codereview.chromium.org/501713002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:421: widget =
DialogDelegate::CreateDialogWidget(this, context, NULL);
On 2014/08/23 21:51:20, Wez wrote:
> Why do we also want to change the no-parent_web_contents case? Or is it always
> NULL if IsNeverVisible is true?

Currently this code is always called with parent_web_contents != NULL, so
currently this line is never executed.
When parent_web_contents is not visible we get parent_window that is an not
visible and has zero dimensions. If I pass that window it crashes here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&l...

Also, there is a cleanup CL for this code that I plan to land after this one:
https://codereview.chromium.org/502693003/ In that CL I'm removing parent_window
as it's not useful anymore.

https://codereview.chromium.org/501713002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:429: // If the picker is
not modal then the picker window needs to be filtered.
On 2014/08/23 21:51:21, Wez wrote:
> nit: Clarify what it means to be filtered/why? What are we filtering?

Removed this comment and updated the comment above to avoid duplication.

[Wez, 2014-08-25 21:32:02]

lgtm

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

https://codereview.chromium.org/501713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:425: // the picker window
from the list of available sources. Filtering is not
nit: ID -> Id
nit: ... so it can be filtered from the list of available sources.

"it" refers to the picker window (or possibly its Id), not to the
DesktopMediaList.

https://codereview.chromium.org/501713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:427: // it's embedded in
the parent window.
I think it'd be clearer to express *why* we need to filter here, first, then
explain the logic, e.g:

"If the picker is not modal to the calling web contents then it is displayed in
its own top-level window, so that window must be filtered out of the list of
top-level windows available for capture."

[Sergey Ulanov, 2014-08-25 22:03:52]

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

https://codereview.chromium.org/501713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:425: // the picker window
from the list of available sources. Filtering is not
On 2014/08/25 21:32:02, Wez wrote:
> nit: ID -> Id
> nit: ... so it can be filtered from the list of available sources.
> 
> "it" refers to the picker window (or possibly its Id), not to the
> DesktopMediaList.

Done.

https://codereview.chromium.org/501713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:427: // it's embedded in
the parent window.
On 2014/08/25 21:32:02, Wez wrote:
> I think it'd be clearer to express *why* we need to filter here, first, then
> explain the logic, e.g:
> 
> "If the picker is not modal to the calling web contents then it is displayed
in
> its own top-level window, so that window must be filtered out of the list of
> top-level windows available for capture."

Done.

[Sergey Ulanov, 2014-08-25 22:04:12]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:06:08]

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

[commit-bot: I haz the power, 2014-08-26 01:54:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 01:58:33]

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

[commit-bot: I haz the power, 2014-08-26 01:58:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sergey Ulanov, 2014-08-26 06:07:56]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-08-26 06:08:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/501713002/80001

[commit-bot: I haz the power, 2014-08-26 07:05:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-26 07:09:53]

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

[commit-bot: I haz the power, 2014-08-26 07:09:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sergey Ulanov, 2014-08-26 21:51:38]

sergeyu@chromium.org changed reviewers:
+ msw@chromium.org, scheib@chromium.org

[Sergey Ulanov, 2014-08-26 21:51:38]

Need OWNERS approvals:
 scheib@ - chrome/common/extensions/docs/
 msw@ - chrome/browser/ui/views/

[msw, 2014-08-26 22:16:46]

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

https://codereview.chromium.org/501713002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:385: gfx::NativeWindow
parent_window,
|parent_window| is no longer used, remove it and cleanup callers too.

[Sergey Ulanov, 2014-08-26 22:43:11]

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

https://codereview.chromium.org/501713002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:385: gfx::NativeWindow
parent_window,
On 2014/08/26 22:16:46, msw wrote:
> |parent_window| is no longer used, remove it and cleanup callers too.

Done.
I also have pending CL that removes this parameter from DesktopMediaPicker
interface: 
https://codereview.chromium.org/502693003/

[msw, 2014-08-26 23:03:16]

chrome/browser/ui/views/ lgtm

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

https://codereview.chromium.org/501713002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:385: gfx::NativeWindow
parent_window,
On 2014/08/26 22:43:11, Sergey Ulanov wrote:
> On 2014/08/26 22:16:46, msw wrote:
> > |parent_window| is no longer used, remove it and cleanup callers too.
> 
> Done.
> I also have pending CL that removes this parameter from DesktopMediaPicker
> interface: 
> https://codereview.chromium.org/502693003/

Nice, thanks.

[scheib, 2014-08-26 23:05:04]

chrome/common/extensions/docs/ LGTM

[Sergey Ulanov, 2014-08-26 23:12:08]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-08-26 23:13:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/501713002/100001

[commit-bot: I haz the power, 2014-08-27 00:15:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-27 00:33:30]

Committed patchset #5 (100001) as 21dca3d092473cbeef024f4788926c46f63c2f58

[thembrown, 2014-09-05 09:00:05]

Sorry for posting here, I don't have access to the Bug.

Is there any change that this could be merged into M38? Unfortunately it's not
easy to work around this in practice.

[Sergey Ulanov, 2014-09-05 17:26:27]

Yes, it's already merged to the M38 branch.


On Fri, Sep 5, 2014 at 2:00 AM, <thembrown@gmail.com> wrote:

> Sorry for posting here, I don't have access to the Bug.
>
> Is there any change that this could be merged into M38? Unfortunately it's
> not
> easy to work around this in practice.
>
> https://codereview.chromium.org/501713002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-10 02:48:15]

Patchset 5 (id:??) landed as
https://crrev.com/71b32fcc875526596305a98c1a1194e690dc8719
Cr-Commit-Position: refs/heads/master@{#292055}