Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issue 2961193002: Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice (Closed)

Created:
3 years, 5 months ago by Hzj_jie
Modified:
3 years, 5 months ago
CC:
chromium-reviews, extensions-reviews_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureOptions impacts which underlying DesktopCapturer implementation is used. But there is no guarantee that different DesktopCapturer implementations would eventually return the same SourceList, or a Source in the SourceList represents the same output content on the system. So DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 Review-Url: https://codereview.chromium.org/2961193002 Cr-Commit-Position: refs/heads/master@{#485097} Committed: https://chromium.googlesource.com/chromium/src/+/56efe642b5d1fc400ac7f34451d9a540f9d30fbb

Patch Set 1 #

Total comments: 8

Patch Set 2 : Resolve review comments #

Patch Set 3 : Move CreateDesktopCaptureOptions into content/public/browser #

Patch Set 4 : CONTENT_EXPORT #

Patch Set 5 : Sync latest changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -30 lines) Patch
M chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc View 1 2 3 chunks +7 lines, -10 lines 0 comments Download
M content/browser/media/capture/desktop_capture_device.cc View 1 2 3 4 4 chunks +2 lines, -20 lines 0 comments Download
M content/public/browser/BUILD.gn View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M content/public/browser/DEPS View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
A content/public/browser/desktop_capture.h View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
A content/public/browser/desktop_capture.cc View 1 2 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 75 (59 generated)
Hzj_jie
3 years, 5 months ago (2017-06-29 00:50:27 UTC) #10
Niklas Enbom
Conceptually this looks good to me, we should definitely use the same experiment in both ...
3 years, 5 months ago (2017-06-29 22:32:28 UTC) #13
Hzj_jie
On 2017/06/29 22:32:28, Niklas Enbom wrote: > Conceptually this looks good to me, we should ...
3 years, 5 months ago (2017-06-29 23:10:53 UTC) #14
Hzj_jie
Sergeyu, this change ensures a same DesktopCaptureOptions is used in both picker and capturer. Please ...
3 years, 5 months ago (2017-06-29 23:12:17 UTC) #16
Sergey Ulanov
Is the issue that different capturers identify multiple monitors differently? It's not clear how this ...
3 years, 5 months ago (2017-06-29 23:49:01 UTC) #17
Hzj_jie
https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc#newcode50 chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_capture_device.cc On 2017/06/29 23:49:01, Sergey Ulanov wrote: > ...
3 years, 5 months ago (2017-06-29 23:56:28 UTC) #21
Hzj_jie
Any other comments?
3 years, 5 months ago (2017-07-05 19:07:51 UTC) #24
Sergey Ulanov
https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc#newcode50 chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_capture_device.cc On 2017/06/29 23:56:27, Hzj_jie wrote: > On ...
3 years, 5 months ago (2017-07-05 19:31:07 UTC) #25
Hzj_jie
The failures do not relate to my change. https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc#newcode50 chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // ...
3 years, 5 months ago (2017-07-07 01:45:08 UTC) #56
Sergey Ulanov
This change LGTM, but I'm not content/public OWNER. On 2017/07/07 01:45:08, Hzj_jie wrote: > The ...
3 years, 5 months ago (2017-07-07 17:35:12 UTC) #59
Hzj_jie
On 2017/07/07 17:35:12, Sergey Ulanov wrote: > This change LGTM, but I'm not content/public OWNER. ...
3 years, 5 months ago (2017-07-07 17:39:28 UTC) #60
Avi (use Gerrit)
On 2017/07/07 17:39:28, Hzj_jie wrote: > On 2017/07/07 17:35:12, Sergey Ulanov wrote: > > This ...
3 years, 5 months ago (2017-07-07 17:48:16 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2961193002/160001
3 years, 5 months ago (2017-07-07 17:49:53 UTC) #63
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/483419)
3 years, 5 months ago (2017-07-07 17:58:53 UTC) #65
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2961193002/180001
3 years, 5 months ago (2017-07-07 23:21:27 UTC) #72
commit-bot: I haz the power
3 years, 5 months ago (2017-07-07 23:26:33 UTC) #75
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/56efe642b5d1fc400ac7f34451d9...

Powered by Google App Engine
This is Rietveld 408576698