Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2997493002:
desktopCapture: minimized window shouldn't be treated as on-top on Win10 (Closed)
DescriptiondesktopCapture: minimized window shouldn't be treated as on-top on Win10
During window capture on Windows 10, if the selected window is minimized,
ShouldUseScreenCapturer() still thinks it's on top and continue to do a
screencapture which is meaningless.
This cl will set |.is_top_window| with false to minimized window,then we
can skip doing any capture to it.
BUG=chromium:568835
Review-Url: https://codereview.webrtc.org/2997493002
Cr-Commit-Position: refs/heads/master@{#19276}
Committed: https://chromium.googlesource.com/external/webrtc/+/b2b803cb74168d1a94a072534d103cab6d6dd199
Patch Set 1 #
Total comments: 1
Patch Set 2 : address comments #Patch Set 3 : rebase #Messages
Total messages: 23 (15 generated)
The CQ bit was checked by braveyao@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@webrtc.org changed reviewers: + zijiehe@chromium.org
Hi zijie, Sorry for not verifying my previous cl on Win10. Currently we'll do screen capture and cropping to the selected window even if it's minimized during window capture, which is meaningless. So correct the logic here. Please take a look.
https://codereview.webrtc.org/2997493002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/cropping_window_capturer_win.cc (right): https://codereview.webrtc.org/2997493002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/cropping_window_capturer_win.cc:55: if (hwnd == context->selected_window) { After this change, unnecessary checks for other windows will be performed. A simple approach can be adding ==================== if (IsIconic(selected_window_) || !IsWindowVisible(selected_window_)) { return false; } ==================== in ShouldUseScreenCapturer() around line 129. P.S. Several more suggestions: 1. Add a RTC_DCHECK_NE in the contructor of TopWindowVerifierContext to ensure excluded_window != selected_window. 2. Move initialization of selected_window_rect also into the contructor. 3. Remove selected_window_process_id. 4. const selected_window / excluded_window / selected_window_rect. Since you are touching this file, if you are not interested in applying these changes, you can add TODOs in TopWindowVerifierContext. Thank you.
The CQ bit was checked by braveyao@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Apparently I didn't understand the codes thoroughly... Thanks so much for the suggestions! All addressed. PTAL.
The change lgtm. Please verify if it works as expected before submitting. Thank you for the cleanup work.
The CQ bit was checked by braveyao@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19933)
The CQ bit was checked by braveyao@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from zijiehe@chromium.org Link to the patchset: https://codereview.webrtc.org/2997493002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1502220264866140,
"parent_rev": "793fc2affa287adfb561280e0d53fea68e218ec8", "commit_rev":
"b2b803cb74168d1a94a072534d103cab6d6dd199"}
Message was sent while issue was closed.
Description was changed from ========== desktopCapture: minimized window shouldn't be treated as on-top on Win10 During window capture on Windows 10, if the selected window is minimized, ShouldUseScreenCapturer() still thinks it's on top and continue to do a screencapture which is meaningless. This cl will set |.is_top_window| with false to minimized window,then we can skip doing any capture to it. BUG=chromium:568835 ========== to ========== desktopCapture: minimized window shouldn't be treated as on-top on Win10 During window capture on Windows 10, if the selected window is minimized, ShouldUseScreenCapturer() still thinks it's on top and continue to do a screencapture which is meaningless. This cl will set |.is_top_window| with false to minimized window,then we can skip doing any capture to it. BUG=chromium:568835 Review-Url: https://codereview.webrtc.org/2997493002 Cr-Commit-Position: refs/heads/master@{#19276} Committed: https://chromium.googlesource.com/external/webrtc/+/b2b803cb74168d1a94a072534... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/b2b803cb74168d1a94a072534... |
