Bug Fix: Desktop Capture Picker Not Modal

Bug Fix: Desktop Capture Picker Not Modal

When the chooseDesktopMedia is initiated from a background Javascript, the picker is not modal, and thus there is a chance the picker is totally covered by the main window, and hard for the user to find it out.

This CL forces the parent of the picker window to be the most recent active browser window in case of a background Javascript. Then the picker will be modal.

BUG=656420
Committed: https://crrev.com/4af22d6bdd6712738296923c5d04da4976f555c3
Cr-Commit-Position: refs/heads/master@{#427797}

[qiangchen, 2016-10-24 17:31:28]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-10-24 17:51:27]

Description was changed from

==========
Picker Modal

BUG=
==========

to

==========
Bug Fix: Desktop Capture Picker Not Modal

When the chooseDesktopMedia is initiated from a background Javascript, the
picker is not modal, and thus there is a chance the picker is totally covered by
the main window, and hard for the user to find it out.

This CL forces the parent of the picker window to be the most recent active
browser window in case of a background Javascript. Then the picker will be
modal.

BUG=656420
==========

[qiangchen, 2016-10-24 18:05:35]

qiangchen@chromium.org changed reviewers:
+ sergeyu@chromium.org

[qiangchen, 2016-10-24 18:05:35]

Can you take a look?

[qiangchen, 2016-10-24 21:07:54]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 21:08:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Ulanov, 2016-10-24 21:37:32]

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:124:
Browser* target_browser = chrome::FindAnyBrowser(
I'm not sure this is the right thing to do. This basically picks a random
window, which may be confusing in some cases, as the result may be a window that
is not the one the user currently interracts with (e.g. you click extension
button in one browser, dialog pops up in another one). I think it's better to
use the last active tabbed window for that profile.

The video in the bug shows extension button clicked. Why
GetTopLevelNativeWindow() doesn't return correct window in that case?

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:127:
content::WebContents* target_contents =
I don't think you need to get target content. Instead you can use 
target_browser->window()->GetNativeWindow() to get the NativeWindow for the
browser.

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:128:
target_browser->tab_strip_model()->GetActiveWebContents();
Need to check that target_browser isn't nullptr.

[qiangchen, 2016-10-24 22:18:40]

fixed and explained.

Thanks.

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:124:
Browser* target_browser = chrome::FindAnyBrowser(
On 2016/10/24 21:37:31, Sergey Ulanov wrote:
> I'm not sure this is the right thing to do. This basically picks a random
> window, which may be confusing in some cases, as the result may be a window
that
> is not the one the user currently interracts with (e.g. you click extension
> button in one browser, dialog pops up in another one). I think it's better to
> use the last active tabbed window for that profile.
> 
> The video in the bug shows extension button clicked. Why
> GetTopLevelNativeWindow() doesn't return correct window in that case?
> 

The chooseDesktopMedia is from the background Javascript of the extension, and
logically, the extension is not part of the tab it is interacting with, and
further the extension seems not to belong to any browser window. So
|web_contents->getTopLevelWindow| return null.

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:127:
content::WebContents* target_contents =
On 2016/10/24 21:37:31, Sergey Ulanov wrote:
> I don't think you need to get target content. Instead you can use 
> target_browser->window()->GetNativeWindow() to get the NativeWindow for the
> browser.

Done.

https://codereview.chromium.org/2428383008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:128:
target_browser->tab_strip_model()->GetActiveWebContents();
On 2016/10/24 21:37:31, Sergey Ulanov wrote:
> Need to check that target_browser isn't nullptr.

Done.

[qiangchen, 2016-10-24 22:39:28]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 22:40:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 22:53:06]

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

[commit-bot: I haz the power, 2016-10-24 22:53:07]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qiangchen, 2016-10-25 16:02:24]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 16:02:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 16:31:54]

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

[commit-bot: I haz the power, 2016-10-25 16:31:58]

Dry run: This issue passed the CQ dry run.

[qiangchen, 2016-10-25 23:40:24]

Follow up.

Any further comments?

Thanks.

[Sergey Ulanov, 2016-10-26 17:08:08]

https://codereview.chromium.org/2428383008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/2428383008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:122: if
(!parent_window) {
Please add a comment here to explain why we need this.

https://codereview.chromium.org/2428383008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:123:
Browser* target_browser = chrome::FindLastActive();
I think it's best to use chrome::FindLastActiveWithProfile() to ensure we show
the dialog in the right profile.

[qiangchen, 2016-10-26 18:39:17]

fixed

https://codereview.chromium.org/2428383008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/2428383008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:122: if
(!parent_window) {
On 2016/10/26 17:08:08, Sergey Ulanov wrote:
> Please add a comment here to explain why we need this.

Done.

https://codereview.chromium.org/2428383008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:123:
Browser* target_browser = chrome::FindLastActive();
On 2016/10/26 17:08:08, Sergey Ulanov wrote:
> I think it's best to use chrome::FindLastActiveWithProfile() to ensure we show
> the dialog in the right profile.

Done.

[Sergey Ulanov, 2016-10-26 19:19:44]

lgtm

https://codereview.chromium.org/2428383008/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/2428383008/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:122: // In
case of background Javascript, |parent_window| will be null. We are
s/of background Javascript/the call comes from a background extension page/
"background Javascript" is ambiguous as one may interpret it as a service
worker, which is not the case here.

[qiangchen, 2016-10-26 19:42:18]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-10-26 19:42:23]

https://codereview.chromium.org/2428383008/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/2428383008/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:122: // In
case of background Javascript, |parent_window| will be null. We are
On 2016/10/26 19:19:44, Sergey Ulanov wrote:
> s/of background Javascript/the call comes from a background extension page/
> "background Javascript" is ambiguous as one may interpret it as a service
> worker, which is not the case here.

Done.

[commit-bot: I haz the power, 2016-10-26 19:43:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiangchen, 2016-10-26 19:44:22]

The CQ bit was unchecked by qiangchen@chromium.org

[qiangchen, 2016-10-26 19:45:09]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-10-26 19:45:10]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2428383008/#ps80001
(title: "Comment")

[qiangchen, 2016-10-26 19:45:23]

The CQ bit was unchecked by qiangchen@chromium.org

[qiangchen, 2016-10-26 20:03:36]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-10-26 20:03:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2428383008/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-26 20:04:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 21:19:03]

Description was changed from

==========
Bug Fix: Desktop Capture Picker Not Modal

When the chooseDesktopMedia is initiated from a background Javascript, the
picker is not modal, and thus there is a chance the picker is totally covered by
the main window, and hard for the user to find it out.

This CL forces the parent of the picker window to be the most recent active
browser window in case of a background Javascript. Then the picker will be
modal.

BUG=656420
==========

to

==========
Bug Fix: Desktop Capture Picker Not Modal

When the chooseDesktopMedia is initiated from a background Javascript, the
picker is not modal, and thus there is a chance the picker is totally covered by
the main window, and hard for the user to find it out.

This CL forces the parent of the picker window to be the most recent active
browser window in case of a background Javascript. Then the picker will be
modal.

BUG=656420
==========

[commit-bot: I haz the power, 2016-10-26 21:19:04]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-26 21:23:11]

Description was changed from

==========
Bug Fix: Desktop Capture Picker Not Modal

When the chooseDesktopMedia is initiated from a background Javascript, the
picker is not modal, and thus there is a chance the picker is totally covered by
the main window, and hard for the user to find it out.

This CL forces the parent of the picker window to be the most recent active
browser window in case of a background Javascript. Then the picker will be
modal.

BUG=656420
==========

to

==========
Bug Fix: Desktop Capture Picker Not Modal

When the chooseDesktopMedia is initiated from a background Javascript, the
picker is not modal, and thus there is a chance the picker is totally covered by
the main window, and hard for the user to find it out.

This CL forces the parent of the picker window to be the most recent active
browser window in case of a background Javascript. Then the picker will be
modal.

BUG=656420

Committed: https://crrev.com/4af22d6bdd6712738296923c5d04da4976f555c3
Cr-Commit-Position: refs/heads/master@{#427797}
==========

[commit-bot: I haz the power, 2016-10-26 21:23:12]

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