Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(315)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by Hzj_jie
Modified:
2 months, 2 weeks 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 75 (59 generated)
Hzj_jie
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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: > ...
2 months, 3 weeks ago (2017-06-29 23:56:28 UTC) #21
Hzj_jie
Any other comments?
2 months, 2 weeks 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 ...
2 months, 2 weeks 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: // ...
2 months, 2 weeks 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 ...
2 months, 2 weeks 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. ...
2 months, 2 weeks ago (2017-07-07 17:39:28 UTC) #60
Avi (ping after 24h)
On 2017/07/07 17:39:28, Hzj_jie wrote: > On 2017/07/07 17:35:12, Sergey Ulanov wrote: > > This ...
2 months, 2 weeks 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
2 months, 2 weeks 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)
2 months, 2 weeks 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
2 months, 2 weeks ago (2017-07-07 23:21:27 UTC) #72
commit-bot: I haz the power
2 months, 2 weeks 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b