Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1808273002:
Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux (Closed)
|
Created:
4 years, 9 months ago by GeorgeZ Modified:
4 years, 9 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse DesktopCaptureDeviceAura for all aura windows in
Windows and Linux OSs in desktop window sharing.
DesktopCaptureDeviceAura was used for chrome browser
windows only. For Chrome extension windows may not be
captured correctly.
Chrome doesn't keep tack of a list of all aura windows. It
asks OS with native IDs to get aura windows
BUG=581790, 289779, 590915
Committed: https://crrev.com/0b2c1c16e25d7bf6296aebc20ac9354d12ba6b3d
Cr-Commit-Position: refs/heads/master@{#382383}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 3
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux OSs in desktop window sharing. DesktopCaptureDeviceAura was used for chrome browser windows only. Chrome doesn't keep tack of a list of all aura windows. It askes OS with native IDs to get aura windows BUG= 581790, 289779, 590915 ========== to ========== Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux OSs in desktop window sharing. DesktopCaptureDeviceAura was used for chrome browser windows only. For Chrome extension windows may not be captured correctly. Chrome doesn't keep tack of a list of all aura windows. It asks OS with native IDs to get aura windows BUG= 581790, 289779, 590915 ==========
gyzhou@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, Could you have a look of this CL. Thanks
On 2016/03/17 17:38:38, GeorgeZ wrote: > Sergey, > > Could you have a look of this CL. > > Thanks Unit tests will be in next cl.
https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:181: void NativeDesktopMediaList::Worker::UpdateSources( Call this UpdateThumbnails()? https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:186: base::Bind(&NativeDesktopMediaList::UpdateSourcesList, UpdateSources() has been posted from the UI thread and here you post a task back to UI thread. This makes the refresh process complicated: Refresh() -> Worker::Refresh ()-> AddAuraIdToMediaId() -> Worker:UpdateSources()-> UpdateSourcesList()-> FinishRefreshOnUiThreadAndScheduleNext() It may look as follows: Refresh() -> Worker::Refresh ()-> UpdateSourcesList() -> Worker::UpdateThumbnails()-> UpdateThumbnailsFinished() https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:293: #else // defined(OS_WIN) https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:297: #endif // !defined(OS_WIN) https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:302: source.id.aura_id = aura_id.aura_id; Also initiate thumbnail capture here instead of in FinishRefreshOnUiThreadAndScheduleNext()? https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:305: #endif // defined(USE_AURA)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Sergey, I updated a patch based on your comments. Thanks https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:181: void NativeDesktopMediaList::Worker::UpdateSources( On 2016/03/17 21:59:18, Sergey Ulanov wrote: > Call this UpdateThumbnails()? Done. https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:186: base::Bind(&NativeDesktopMediaList::UpdateSourcesList, On 2016/03/17 21:59:18, Sergey Ulanov wrote: > UpdateSources() has been posted from the UI thread and here you post a task back > to UI thread. This makes the refresh process complicated: > > Refresh() -> Worker::Refresh ()-> AddAuraIdToMediaId() -> > Worker:UpdateSources()-> UpdateSourcesList()-> > FinishRefreshOnUiThreadAndScheduleNext() > > It may look as follows: > > Refresh() -> Worker::Refresh ()-> UpdateSourcesList() -> > Worker::UpdateThumbnails()-> UpdateThumbnailsFinished() Good suggestion and make code and logic concise. Code work flow is changed to Refresh() -> Worker::Refresh () -> RefreshForAuraWindows() -> Worker::UpdateNativeThumbnails() -> UpdateNativeThumbnailsFinished() https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:293: #else On 2016/03/17 21:59:18, Sergey Ulanov wrote: > // defined(OS_WIN) Done. https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:297: #endif On 2016/03/17 21:59:18, Sergey Ulanov wrote: > // !defined(OS_WIN) Done. https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:302: source.id.aura_id = aura_id.aura_id; On 2016/03/17 21:59:18, Sergey Ulanov wrote: > Also initiate thumbnail capture here instead of in > FinishRefreshOnUiThreadAndScheduleNext()? I have to schedule next refresh after both aura window and native window captures are done. https://codereview.chromium.org/1808273002/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:305: #endif On 2016/03/17 21:59:18, Sergey Ulanov wrote: > // defined(USE_AURA) Done.
https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:13: #include "chrome/browser/ui/browser_window.h" Don't need to include these 3 headers anymore https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:97: void UpdateNativeThumbnails(const std::vector<SourceDescription>& sources, RefreshThumbnails. https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:281: #if defined(USE_X11) && !defined(OS_CHROMEOS) #elif https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:298: if (source.id.aura_id > DesktopMediaID::kNullId) Currently both this and Worker::UpdateNativeThumbnails() are responsible for filtering aura windows from everything else. I suggest building a separate list of native windows here and passing them to the worker.
Sergey, I have updated code based on your comments. Thanks, https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:13: #include "chrome/browser/ui/browser_window.h" On 2016/03/18 21:09:36, Sergey Ulanov wrote: > Don't need to include these 3 headers anymore Done. https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:97: void UpdateNativeThumbnails(const std::vector<SourceDescription>& sources, On 2016/03/18 21:09:36, Sergey Ulanov wrote: > RefreshThumbnails. Done. https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:281: #if defined(USE_X11) && !defined(OS_CHROMEOS) On 2016/03/18 21:09:36, Sergey Ulanov wrote: > #elif Done. https://codereview.chromium.org/1808273002/diff/60001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:298: if (source.id.aura_id > DesktopMediaID::kNullId) On 2016/03/18 21:09:36, Sergey Ulanov wrote: > Currently both this and Worker::UpdateNativeThumbnails() are responsible for > filtering aura windows from everything else. I suggest building a separate list > of native windows here and passing them to the worker. Done.
lgtm when my comments are addressed https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:175: const std::vector<SourceDescription>& native_sources, nit: this can be a vector of DesktopMediaId instead of SourceDescrittion. This function doesn't need titles. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:254: native_thumbnail_capture_completed = false; suggest replacing this with pending_native_thumbnail_capture_, i.e. invert the meaning of this flag. This would make it consistent with pending_aura_capture_requests_ and less ambiguous after initialization before the list is refreshed the first time. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:273: #endif // defined(OS_WIN) this should be // defined(USE_X11) && !defined(OS_CHROMEOS) https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:305: native_thumbnail_capture_completed = true; DCHECK(!native_thumbnail_capture_completed_) https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:67: int pending_aura_capture_requests_; initialize this value. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:68: bool native_thumbnail_capture_completed; add _ at the end of the name https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:68: bool native_thumbnail_capture_completed; initialize this value.
Sergey, I addressed all your comments, Thanks, https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:175: const std::vector<SourceDescription>& native_sources, On 2016/03/21 19:18:01, Sergey Ulanov wrote: > nit: this can be a vector of DesktopMediaId instead of SourceDescrittion. This > function doesn't need titles. Done. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:254: native_thumbnail_capture_completed = false; On 2016/03/21 19:18:01, Sergey Ulanov wrote: > suggest replacing this with pending_native_thumbnail_capture_, i.e. invert the > meaning of this flag. This would make it consistent with > pending_aura_capture_requests_ and less ambiguous after initialization before > the list is refreshed the first time. Done. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:273: #endif // defined(OS_WIN) On 2016/03/21 19:18:01, Sergey Ulanov wrote: > this should be // defined(USE_X11) && !defined(OS_CHROMEOS) Done. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:305: native_thumbnail_capture_completed = true; On 2016/03/21 19:18:01, Sergey Ulanov wrote: > DCHECK(!native_thumbnail_capture_completed_) Done. https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1808273002/diff/80001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:67: int pending_aura_capture_requests_; On 2016/03/21 19:18:01, Sergey Ulanov wrote: > initialize this value. Done.
The CQ bit was checked by gyzhou@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/1808273002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808273002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808273002/100001
Message was sent while issue was closed.
Description was changed from ========== Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux OSs in desktop window sharing. DesktopCaptureDeviceAura was used for chrome browser windows only. For Chrome extension windows may not be captured correctly. Chrome doesn't keep tack of a list of all aura windows. It asks OS with native IDs to get aura windows BUG= 581790, 289779, 590915 ========== to ========== Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux OSs in desktop window sharing. DesktopCaptureDeviceAura was used for chrome browser windows only. For Chrome extension windows may not be captured correctly. Chrome doesn't keep tack of a list of all aura windows. It asks OS with native IDs to get aura windows BUG= 581790, 289779, 590915 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux OSs in desktop window sharing. DesktopCaptureDeviceAura was used for chrome browser windows only. For Chrome extension windows may not be captured correctly. Chrome doesn't keep tack of a list of all aura windows. It asks OS with native IDs to get aura windows BUG= 581790, 289779, 590915 ========== to ========== Use DesktopCaptureDeviceAura for all aura windows in Windows and Linux OSs in desktop window sharing. DesktopCaptureDeviceAura was used for chrome browser windows only. For Chrome extension windows may not be captured correctly. Chrome doesn't keep tack of a list of all aura windows. It asks OS with native IDs to get aura windows BUG= 581790, 289779, 590915 Committed: https://crrev.com/0b2c1c16e25d7bf6296aebc20ac9354d12ba6b3d Cr-Commit-Position: refs/heads/master@{#382383} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b2c1c16e25d7bf6296aebc20ac9354d12ba6b3d Cr-Commit-Position: refs/heads/master@{#382383}
Message was sent while issue was closed.
couple more comments https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:252: pending_aura_capture_requests_ = 0; I think this can be replaced with a DCHECK(): DCHECK_EQ(pending_aura_capture_requests_, 0); https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:254: pending_native_thumbnail_capture_ = true; Please add. DCHECK(!pending_native_thumbnail_capture_) https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.h:68: bool pending_native_thumbnail_capture_ = true; I think you want to initialize it to false. It's not pending initially.
Message was sent while issue was closed.
Sergey, The code just committed. I will address these comments in CL for unit tests. Thanks, George On Mon, Mar 21, 2016 at 1:54 PM, <sergeyu@chromium.org> wrote: > couple more comments > > > > https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... > File chrome/browser/media/native_desktop_media_list.cc (right): > > > https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... > chrome/browser/media/native_desktop_media_list.cc:252: > pending_aura_capture_requests_ = 0; > I think this can be replaced with a DCHECK(): > DCHECK_EQ(pending_aura_capture_requests_, 0); > > > https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... > chrome/browser/media/native_desktop_media_list.cc:254: > pending_native_thumbnail_capture_ = true; > Please add. > DCHECK(!pending_native_thumbnail_capture_) > > > https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... > File chrome/browser/media/native_desktop_media_list.h (right): > > > https://codereview.chromium.org/1808273002/diff/100001/chrome/browser/media/n... > chrome/browser/media/native_desktop_media_list.h:68: bool > pending_native_thumbnail_capture_ = true; > I think you want to initialize it to false. It's not pending initially. > > https://codereview.chromium.org/1808273002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
