|
|
Created:
4 years, 1 month ago by qiangchen Modified:
4 years, 1 month ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBug 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}
Patch Set 1 : Modal #
Total comments: 6
Patch Set 2 : Fix by comment #
Total comments: 4
Patch Set 3 : Comment #
Total comments: 2
Patch Set 4 : Comment #Patch Set 5 : Rebase #Messages
Total messages: 34 (22 generated)
Patchset #1 (id:1) has been deleted
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@chromium.org changed reviewers: + sergeyu@chromium.org
Can you take a look?
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
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.
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Follow up. Any further comments? Thanks.
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.
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.
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.
The CQ bit was checked by qiangchen@chromium.org
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.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by qiangchen@chromium.org
The CQ bit was checked by qiangchen@chromium.org
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")
The CQ bit was unchecked by qiangchen@chromium.org
The CQ bit was checked by qiangchen@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4af22d6bdd6712738296923c5d04da4976f555c3 Cr-Commit-Position: refs/heads/master@{#427797} |