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

Issue 1763753003: Capture chrome browser windows from internal rendering procedure for windows and linux (Closed)

Created:
4 years, 9 months ago by GeorgeZ
Modified:
4 years, 9 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, posciak+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix NativeDesktopMediaList to set aura_id for chrome Aura windows for Linux and windows. Previously NativeDesktopMediaList was returning only native window ID, now it also sets aura_id for chrome in Linux and windows. This allows to capture browser windows directly from Aura, which is faster and more reliable. BUG=581790, 289779, 590915 Committed: https://crrev.com/a21a35a14f00f2a14681f50454b7eff79fda7765 Cr-Commit-Position: refs/heads/master@{#381711}

Patch Set 1 : #

Total comments: 14

Patch Set 2 : #

Total comments: 24

Patch Set 3 : #

Total comments: 8

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+206 lines, -39 lines) Patch
M chrome/browser/media/desktop_media_list_base.h View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/media/desktop_media_list_base.cc View 1 2 3 5 chunks +17 lines, -5 lines 0 comments Download
M chrome/browser/media/native_desktop_media_list.h View 1 2 3 4 5 chunks +27 lines, -1 line 0 comments Download
M chrome/browser/media/native_desktop_media_list.cc View 1 2 3 4 11 chunks +138 lines, -13 lines 0 comments Download
M chrome/browser/media/tab_desktop_media_list.cc View 1 2 1 chunk +0 lines, -12 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.cc View 1 2 chunks +18 lines, -8 lines 0 comments Download

Messages

Total messages: 40 (17 generated)
GeorgeZ
Sergey and Jiayang, Could you have a look for this CL. Jiayang, You may just ...
4 years, 9 months ago (2016-03-07 17:47:16 UTC) #8
Sergey Ulanov
https://codereview.chromium.org/1763753003/diff/60001/chrome/browser/media/native_desktop_media_list.cc File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1763753003/diff/60001/chrome/browser/media/native_desktop_media_list.cc#newcode192 chrome/browser/media/native_desktop_media_list.cc:192: if (source.id.aura_id > 0) Add a comment to explain ...
4 years, 9 months ago (2016-03-07 21:01:51 UTC) #9
GeorgeZ
Sergey, I have update based on your comments. Thanks, https://codereview.chromium.org/1763753003/diff/60001/chrome/browser/media/native_desktop_media_list.cc File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1763753003/diff/60001/chrome/browser/media/native_desktop_media_list.cc#newcode192 chrome/browser/media/native_desktop_media_list.cc:192: ...
4 years, 9 months ago (2016-03-07 23:29:20 UTC) #10
GeorgeZ
Ping Sergey and JiaYang, https://codereview.chromium.org/1763753003/diff/60001/chrome/browser/media/native_desktop_media_list.h File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1763753003/diff/60001/chrome/browser/media/native_desktop_media_list.h#newcode60 chrome/browser/media/native_desktop_media_list.h:60: ImageHashesMap cur_aura_thumbnail_hashes_; On 2016/03/07 21:01:50, ...
4 years, 9 months ago (2016-03-09 22:38:52 UTC) #11
jiayl
Sergey's review will be sufficient. Let me know if you just need a rubber stamp.
4 years, 9 months ago (2016-03-09 22:48:34 UTC) #12
Sergey Ulanov
https://codereview.chromium.org/1763753003/diff/80001/chrome/browser/media/desktop_media_list_base.h File chrome/browser/media/desktop_media_list_base.h (right): https://codereview.chromium.org/1763753003/diff/80001/chrome/browser/media/desktop_media_list_base.h#newcode12 chrome/browser/media/desktop_media_list_base.h:12: using content::DesktopMediaID; using is not allowed in headers. Please ...
4 years, 9 months ago (2016-03-10 00:46:08 UTC) #13
Sergey Ulanov
Also I think this CL deserves a better description. Specifically it's not clear that it ...
4 years, 9 months ago (2016-03-10 00:50:42 UTC) #14
GeorgeZ
Sergey, I updated code based your comments. Thanks https://codereview.chromium.org/1763753003/diff/80001/chrome/browser/media/desktop_media_list_base.h File chrome/browser/media/desktop_media_list_base.h (right): https://codereview.chromium.org/1763753003/diff/80001/chrome/browser/media/desktop_media_list_base.h#newcode12 chrome/browser/media/desktop_media_list_base.h:12: using ...
4 years, 9 months ago (2016-03-10 21:18:46 UTC) #18
GeorgeZ
dalecurtis@ As owner, could you have a look of content/browser/renderer_host/media/media_stream_manager.cc. I consulted with the auhtor ...
4 years, 9 months ago (2016-03-10 21:23:21 UTC) #20
DaleCurtis
I'll stamp after jiayl@ and sergeyu@ have approved.
4 years, 9 months ago (2016-03-10 23:54:59 UTC) #21
Sergey Ulanov
Is it possible to add unittests for the new code? https://codereview.chromium.org/1763753003/diff/140001/chrome/browser/media/desktop_media_list_base.cc File chrome/browser/media/desktop_media_list_base.cc (right): https://codereview.chromium.org/1763753003/diff/140001/chrome/browser/media/desktop_media_list_base.cc#newcode136 ...
4 years, 9 months ago (2016-03-12 01:01:20 UTC) #22
bonillaglenda0
lgtm
4 years, 9 months ago (2016-03-12 11:10:42 UTC) #24
GeorgeZ
Sergey, I should be able to add unit tests for newly added code. From my ...
4 years, 9 months ago (2016-03-14 16:49:03 UTC) #25
Sergey Ulanov
lgtm
4 years, 9 months ago (2016-03-15 22:34:02 UTC) #26
GeorgeZ
JiaYang and DaleCurtis, Could you approve this CL after Sergey approved. Thanks,
4 years, 9 months ago (2016-03-15 22:40:33 UTC) #27
jiayl
lgtm
4 years, 9 months ago (2016-03-16 00:05:55 UTC) #28
GeorgeZ
DaleCurtis, Could you approve this CL since Sergey and jiayl had approved? Thanks,
4 years, 9 months ago (2016-03-16 18:39:13 UTC) #29
DaleCurtis
https://codereview.chromium.org/1763753003/diff/160001/chrome/browser/media/native_desktop_media_list.cc File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1763753003/diff/160001/chrome/browser/media/native_desktop_media_list.cc#newcode116 chrome/browser/media/native_desktop_media_list.cc:116: NativeAuraIdMap native_aura_id_map); This appears to be passing by copy, ...
4 years, 9 months ago (2016-03-16 21:10:01 UTC) #30
GeorgeZ
Dalecurtis, I uploaded a patch based on your comment. Thanks, https://codereview.chromium.org/1763753003/diff/160001/chrome/browser/media/native_desktop_media_list.cc File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1763753003/diff/160001/chrome/browser/media/native_desktop_media_list.cc#newcode116 ...
4 years, 9 months ago (2016-03-16 21:34:14 UTC) #32
DaleCurtis
lgtm
4 years, 9 months ago (2016-03-17 04:40:35 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763753003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763753003/200001
4 years, 9 months ago (2016-03-17 15:01:07 UTC) #36
commit-bot: I haz the power
Committed patchset #5 (id:200001)
4 years, 9 months ago (2016-03-17 15:14:08 UTC) #38
commit-bot: I haz the power
4 years, 9 months ago (2016-03-17 15:16:42 UTC) #40
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/a21a35a14f00f2a14681f50454b7eff79fda7765
Cr-Commit-Position: refs/heads/master@{#381711}

Powered by Google App Engine
This is Rietveld 408576698