Do not filter the picker window with Aura.

Do not filter the picker window with Aura.
The picker window is embedded in the parent window and does not have its own native window.

TESTED=verified on Linux, Win and ChromeOS that the picker window is not listed to share with or without a delegate tab, and the chrome window is correctly listed.

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

[jiayl, 2014-06-13 16:41:54]

I verified this works on Linux and Windows.

Some questions:

1. are there any platforms using views but not Aura?
2. is this correct for ChromeOs? Do we still need RegisterAuraWindow?

[jiayl, 2014-06-13 16:42:20]

PTAL

[msw, 2014-06-13 16:53:03]

I'll let Sergey take the first crack at this; I only touched that specific code
block to do some refactoring. Otherwise, I was fixing the dialog widget
creation. (this odd dialog is the only one I've seen that can be web- or
window-modal)

On 2014/06/13 16:41:54, jiayl wrote:
> 1. are there any platforms using views but not Aura?
Not at the moment.
> 2. is this correct for ChromeOs? Do we still need RegisterAuraWindow?
No idea, sorry :(

[Sergey Ulanov, 2014-06-13 17:38:41]

https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views.cc:550: // own native window
with Aura. So we do not filter any window.
Actually it's not always the case. We still have a separate dialog window in
some cases - see line 547. 
If CreateWebModalDialogView() is used to create the dialog, then we don't want
to filter it, but we still want filtering when the dialog is created using
DialogDelegate::CreateDialogWidget()

[Sergey Ulanov, 2014-06-13 17:42:28]

On 2014/06/13 16:41:54, jiayl wrote:
> I verified this works on Linux and Windows.
> 
> Some questions:
> 
> 1. are there any platforms using views but not Aura?
> 2. is this correct for ChromeOs? Do we still need RegisterAuraWindow?

Yes. RegisterAuraWindow() is useful there when dialog needs to be filtered.

[jiayl, 2014-06-13 17:54:54]

https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views.cc:550: // own native window
with Aura. So we do not filter any window.
On 2014/06/13 17:38:41, Sergey Ulanov wrote:
> Actually it's not always the case. We still have a separate dialog window in
> some cases - see line 547. 
> If CreateWebModalDialogView() is used to create the dialog, then we don't want
> to filter it, but we still want filtering when the dialog is created using
> DialogDelegate::CreateDialogWidget()

But I tested both the desktopCapture example extension (i.e. CreateDialogWidget)
code path and the tab delegated code path (i.e. createWebModalDialogViews), the
picker dialog is not listed in the picker without filtering.

[jiayl, 2014-06-13 22:06:18]

On 2014/06/13 16:53:03, msw wrote:
> I'll let Sergey take the first crack at this; I only touched that specific
code
> block to do some refactoring. Otherwise, I was fixing the dialog widget
> creation. (this odd dialog is the only one I've seen that can be web- or
> window-modal)
> 
> On 2014/06/13 16:41:54, jiayl wrote:
> > 1. are there any platforms using views but not Aura?
> Not at the moment.
> > 2. is this correct for ChromeOs? Do we still need RegisterAuraWindow?
> No idea, sorry :(

I verified on ChromeOS that the filtering is not needed as well.

[Sergey Ulanov, 2014-06-13 22:34:51]

On 2014/06/13 17:54:54, jiayl wrote:
>
https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
> chrome/browser/ui/views/desktop_media_picker_views.cc:550: // own native
window
> with Aura. So we do not filter any window.
> On 2014/06/13 17:38:41, Sergey Ulanov wrote:
> > Actually it's not always the case. We still have a separate dialog window in
> > some cases - see line 547. 
> > If CreateWebModalDialogView() is used to create the dialog, then we don't
want
> > to filter it, but we still want filtering when the dialog is created using
> > DialogDelegate::CreateDialogWidget()
> 
> But I tested both the desktopCapture example extension (i.e.
CreateDialogWidget)
> code path and the tab delegated code path (i.e. createWebModalDialogViews),
the
> picker dialog is not listed in the picker without filtering.

Right. DialogDelegate::CreateDialogWidget() is used when the API is called from
a background extension page that doesn't have UI context.

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

On 2014/06/13 22:34:51, Sergey Ulanov wrote:
> On 2014/06/13 17:54:54, jiayl wrote:
> >
>
https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
> > File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> > 
> >
>
https://codereview.chromium.org/338503007/diff/1/chrome/browser/ui/views/desk...
> > chrome/browser/ui/views/desktop_media_picker_views.cc:550: // own native
> window
> > with Aura. So we do not filter any window.
> > On 2014/06/13 17:38:41, Sergey Ulanov wrote:
> > > Actually it's not always the case. We still have a separate dialog window
in
> > > some cases - see line 547. 
> > > If CreateWebModalDialogView() is used to create the dialog, then we don't
> want
> > > to filter it, but we still want filtering when the dialog is created using
> > > DialogDelegate::CreateDialogWidget()
> > 
> > But I tested both the desktopCapture example extension (i.e.
> CreateDialogWidget)
> > code path and the tab delegated code path (i.e. createWebModalDialogViews),
> the
> > picker dialog is not listed in the picker without filtering.
> 
> Right. DialogDelegate::CreateDialogWidget() is used when the API is called
from
> a background extension page that doesn't have UI context.

Updated the patch to only filter when there is a parent.
The problem that a parent is always passed even when there is no chrome window
associated with the call is tracked in crbug/385116

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

PTAL

[Sergey Ulanov, 2014-06-16 21:48:43]

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

https://codereview.chromium.org/338503007/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:572: if (!parent_window &&
!parent_web_contents) {
You need just !parent_web_contents. parent_window is not relevant when deciding
what type of window to show - see line 589. You may need !parent_window in line
581, for the case when Ash is not active.

[jiayl, 2014-06-16 22:07:34]

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

https://codereview.chromium.org/338503007/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:572: if (!parent_window &&
!parent_web_contents) {
On 2014/06/16 21:48:43, Sergey Ulanov wrote:
> You need just !parent_web_contents. parent_window is not relevant when
deciding
> what type of window to show - see line 589. You may need !parent_window in
line
> 581, for the case when Ash is not active.

That's not true. If the parent_web_contents is NULL but parent_window is not
NULL, the picker is still embedded in the parent_window and we should not
filter. I've tested it by always setting parent_web_contents NULL.

[Sergey Ulanov, 2014-06-16 22:14:01]

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

https://codereview.chromium.org/338503007/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:572: if (!parent_window &&
!parent_web_contents) {
On 2014/06/16 22:07:34, jiayl wrote:
> On 2014/06/16 21:48:43, Sergey Ulanov wrote:
> > You need just !parent_web_contents. parent_window is not relevant when
> deciding
> > what type of window to show - see line 589. You may need !parent_window in
> line
> > 581, for the case when Ash is not active.
> 
> That's not true. If the parent_web_contents is NULL but parent_window is not
> NULL, the picker is still embedded in the parent_window and we should not
> filter. I've tested it by always setting parent_web_contents NULL.

Did you try it with Ash? I think with Ash you'll get a different window and you
still need filtering even when parent_window!=null.

[jiayl, 2014-06-16 22:32:59]

On 2014/06/16 22:14:01, Sergey Ulanov wrote:
>
https://codereview.chromium.org/338503007/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/338503007/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/desktop_media_picker_views.cc:572: if (!parent_window
&&
> !parent_web_contents) {
> On 2014/06/16 22:07:34, jiayl wrote:
> > On 2014/06/16 21:48:43, Sergey Ulanov wrote:
> > > You need just !parent_web_contents. parent_window is not relevant when
> > deciding
> > > what type of window to show - see line 589. You may need !parent_window in
> > line
> > > 581, for the case when Ash is not active.
> > 
> > That's not true. If the parent_web_contents is NULL but parent_window is not
> > NULL, the picker is still embedded in the parent_window and we should not
> > filter. I've tested it by always setting parent_web_contents NULL.
> 
> Did you try it with Ash? I think with Ash you'll get a different window and
you
> still need filtering even when parent_window!=null.

I tested on a Pixel that the picker window is not listed as a sharable window
without filtering, when parnet_web_contents is NULL and parent_window not NULL.
The picker is not modal, but it's still not independently captured.

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

Ping reviewers.

[Sergey Ulanov, 2014-06-17 17:41:23]

lgtm

[msw, 2014-06-17 17:57:51]

lgtm

[jiayl, 2014-06-17 18:02:17]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-06-17 18:05:25]

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

[commit-bot: I haz the power, 2014-06-17 21:22:15]

Change committed as 277866