Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108293002:
VideoCaptureManager: remove ScopedVector and other smaller cleanups (Closed)
|
Created:
4 years, 5 months ago by mcasas Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVideoCaptureManager: remove ScopedVector and other smaller cleanups
because is deprecated. Also using |using|.
While I was doing that, I saw that some methods
related to Get_X_By_Y_() had inconsistent names,
signatures and/or locations in the header/implementation
files, hence:
- GetDeviceEntryForMediaStreamDevice() renamed to
GetDeviceEntryByTypeAndId() and reducing the
parameters to the used information.
- GetDeviceEntryForController() renamed to
GetDeviceEntryByController()
- FindDeviceInfoById() renamed to GetDeviceInfoById()
- GetVideoCaptureDeviceFromSessionId() renamed to
GetVideoCaptureDeviceBySessionId()
- Added GetDeviceEntryBySerialId().
And bundled together in the .h file. Also I shoved
around some methods in the .h file to match the .cc
but I'm planning on another subsequent CL just
aligning both.
BUG=554289
TEST=no new code introduced (this is a refactoring)
so all unittests, content_browsertests and browser_tests
related to video capture (there a loads of them)
passing should be enough verification.
Committed: https://crrev.com/8b9d517450cbf16332bc4c31aa855b40bd1ee922
Cr-Commit-Position: refs/heads/master@{#403299}
Patch Set 1 #
Total comments: 7
Patch Set 2 : chfremer@ comments #
Total comments: 16
Patch Set 3 : emircan@s nits #
Messages
Total messages: 20 (11 generated)
Description was changed from ========== VideoCaptureManager: remove ScopedVector because is deprecated BUG=554289 ========== to ========== VideoCaptureManager: remove ScopedVector because is deprecated. Also using |using|. WIP !! BUG=554289 ==========
Description was changed from ========== VideoCaptureManager: remove ScopedVector because is deprecated. Also using |using|. WIP !! BUG=554289 ========== to ========== VideoCaptureManager: remove ScopedVector and other smaller cleanups because is deprecated. Also using |using|. While I was doing that, I saw that some methods related to Get_X_By_Y_() had inconsistent names, signatures and/or locations in the header/implementation files, hence: - GetDeviceEntryForMediaStreamDevice() renamed to GetDeviceEntryByTypeAndId() and reducing the parameters to the used information. - GetDeviceEntryForController() renamed to GetDeviceEntryByController() - FindDeviceInfoById() renamed to GetDeviceInfoById() - GetVideoCaptureDeviceFromSessionId() renamed to GetVideoCaptureDeviceBySessionId() - Added GetDeviceEntryBySerialId(). And bundled together in the .h file. Also I shoved around some methods in the .h file to match the .cc but I'm planning on another subsequent CL just aligning both. BUG=554289 TEST=no new code introduced (this is a refactoring) so all unittests, content_browsertests and browser_tests related to video capture (there a loads of them) passing should be enough verification. ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ PTAL
chfremer@chromium.org changed reviewers: + chfremer@chromium.org
https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:370: }) != devices_.end()); Is there any reusable short-hand notation for finding a raw pointer in a container storing unique_ptr<>? This seems like a fairly common need. If nothing exists, can we create something and put it into ::base? https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:977: }); See the other comment. Would be nice to have reusable short-hand form. https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:314: // Currently opened devices and associate controller. The device may or may Is there only one controller associated with all devices? If not, this should read "... and associated controllers".
PTAL https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:370: }) != devices_.end()); On 2016/06/30 17:08:52, chfremer wrote: > Is there any reusable short-hand notation for finding a raw pointer in a > container storing unique_ptr<>? This seems like a fairly common need. If nothing > exists, can we create something and put it into ::base? Yeah, see my reply below. https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:977: }); On 2016/06/30 17:08:52, chfremer wrote: > See the other comment. Would be nice to have reusable short-hand form. I searched in stl_util.h and base/containers and didn't find anything similar. Perhaps we could extend the logic in ContainsKey()/ContainsValue() [1] to detect that the Collection is a std::vector<std::unique_ptr<>> and the Value would be a pointer, and search like we are doing here, WDYT? I'll make a bug and leave it for another CL though :) [1] https://cs.chromium.org/chromium/src/base/stl_util.h?sq=package:chromium&dr=C... https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:314: // Currently opened devices and associate controller. The device may or may On 2016/06/30 17:08:52, chfremer wrote: > Is there only one controller associated with all devices? > If not, this should read "... and associated controllers". Yeah, there is a one-to-one relationship between a VideoCaptureDevice and a VideoCaptureController, both are kept together in the DeviceEntry, l.128 of this file https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide... Will rewrite though.
lgtm https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2108293002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:977: }); Thanks for creating the bug entry. I think a new function ContainsPointer() sounds like a reasonable solution. I am not a fan of overloading one of the existing functions, because the name would not reveal the intent very well.
lgtm % nits. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:411: DeviceEntry* entry = GetDeviceEntryBySerialId(serial_id); DeviceEntry* const entry https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:508: DeviceEntry* entry = GetDeviceEntryBySerialId(serial_id); Ditto https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:1035: return &(it); Remove extra () https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:212: // Finds a DeviceEntry in different ways: by its device ID and type (if it is For consistency; s/device ID/|device_id| (also change the function parameter) and s/type|type|. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:225: // if necessary. Returns NULL if the session id is invalid. s/session id/|capture_session_id| https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:312: SessionMap sessions_; Using-declarations should be in the beginning of the private section. https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:314: // Currently opened device-controller couples. The device may or may not be It sounds like this is a vector of pairs which is misleading. Can we say "Currently opened DeviceEntry instances ..."? https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:319: using DeviceStartQueue = std::list<CaptureDeviceStartRequest>; Ditto.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, chfremer@chromium.org Link to the patchset: https://codereview.chromium.org/2108293002/#ps100001 (title: "emircan@s nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== VideoCaptureManager: remove ScopedVector and other smaller cleanups because is deprecated. Also using |using|. While I was doing that, I saw that some methods related to Get_X_By_Y_() had inconsistent names, signatures and/or locations in the header/implementation files, hence: - GetDeviceEntryForMediaStreamDevice() renamed to GetDeviceEntryByTypeAndId() and reducing the parameters to the used information. - GetDeviceEntryForController() renamed to GetDeviceEntryByController() - FindDeviceInfoById() renamed to GetDeviceInfoById() - GetVideoCaptureDeviceFromSessionId() renamed to GetVideoCaptureDeviceBySessionId() - Added GetDeviceEntryBySerialId(). And bundled together in the .h file. Also I shoved around some methods in the .h file to match the .cc but I'm planning on another subsequent CL just aligning both. BUG=554289 TEST=no new code introduced (this is a refactoring) so all unittests, content_browsertests and browser_tests related to video capture (there a loads of them) passing should be enough verification. ========== to ========== VideoCaptureManager: remove ScopedVector and other smaller cleanups because is deprecated. Also using |using|. While I was doing that, I saw that some methods related to Get_X_By_Y_() had inconsistent names, signatures and/or locations in the header/implementation files, hence: - GetDeviceEntryForMediaStreamDevice() renamed to GetDeviceEntryByTypeAndId() and reducing the parameters to the used information. - GetDeviceEntryForController() renamed to GetDeviceEntryByController() - FindDeviceInfoById() renamed to GetDeviceInfoById() - GetVideoCaptureDeviceFromSessionId() renamed to GetVideoCaptureDeviceBySessionId() - Added GetDeviceEntryBySerialId(). And bundled together in the .h file. Also I shoved around some methods in the .h file to match the .cc but I'm planning on another subsequent CL just aligning both. BUG=554289 TEST=no new code introduced (this is a refactoring) so all unittests, content_browsertests and browser_tests related to video capture (there a loads of them) passing should be enough verification. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== VideoCaptureManager: remove ScopedVector and other smaller cleanups because is deprecated. Also using |using|. While I was doing that, I saw that some methods related to Get_X_By_Y_() had inconsistent names, signatures and/or locations in the header/implementation files, hence: - GetDeviceEntryForMediaStreamDevice() renamed to GetDeviceEntryByTypeAndId() and reducing the parameters to the used information. - GetDeviceEntryForController() renamed to GetDeviceEntryByController() - FindDeviceInfoById() renamed to GetDeviceInfoById() - GetVideoCaptureDeviceFromSessionId() renamed to GetVideoCaptureDeviceBySessionId() - Added GetDeviceEntryBySerialId(). And bundled together in the .h file. Also I shoved around some methods in the .h file to match the .cc but I'm planning on another subsequent CL just aligning both. BUG=554289 TEST=no new code introduced (this is a refactoring) so all unittests, content_browsertests and browser_tests related to video capture (there a loads of them) passing should be enough verification. ========== to ========== VideoCaptureManager: remove ScopedVector and other smaller cleanups because is deprecated. Also using |using|. While I was doing that, I saw that some methods related to Get_X_By_Y_() had inconsistent names, signatures and/or locations in the header/implementation files, hence: - GetDeviceEntryForMediaStreamDevice() renamed to GetDeviceEntryByTypeAndId() and reducing the parameters to the used information. - GetDeviceEntryForController() renamed to GetDeviceEntryByController() - FindDeviceInfoById() renamed to GetDeviceInfoById() - GetVideoCaptureDeviceFromSessionId() renamed to GetVideoCaptureDeviceBySessionId() - Added GetDeviceEntryBySerialId(). And bundled together in the .h file. Also I shoved around some methods in the .h file to match the .cc but I'm planning on another subsequent CL just aligning both. BUG=554289 TEST=no new code introduced (this is a refactoring) so all unittests, content_browsertests and browser_tests related to video capture (there a loads of them) passing should be enough verification. Committed: https://crrev.com/8b9d517450cbf16332bc4c31aa855b40bd1ee922 Cr-Commit-Position: refs/heads/master@{#403299} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8b9d517450cbf16332bc4c31aa855b40bd1ee922 Cr-Commit-Position: refs/heads/master@{#403299}
Message was sent while issue was closed.
Old comments. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:411: DeviceEntry* entry = GetDeviceEntryBySerialId(serial_id); On 2016/06/30 18:24:04, emircan wrote: > DeviceEntry* const entry Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:508: DeviceEntry* entry = GetDeviceEntryBySerialId(serial_id); On 2016/06/30 18:24:04, emircan wrote: > Ditto Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:1035: return &(it); On 2016/06/30 18:24:04, emircan wrote: > Remove extra () Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:212: // Finds a DeviceEntry in different ways: by its device ID and type (if it is On 2016/06/30 18:24:05, emircan wrote: > For consistency; s/device ID/|device_id| (also change the function parameter) > and s/type|type|. Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:225: // if necessary. Returns NULL if the session id is invalid. On 2016/06/30 18:24:04, emircan wrote: > s/session id/|capture_session_id| Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:312: SessionMap sessions_; On 2016/06/30 18:24:05, emircan wrote: > Using-declarations should be in the beginning of the private section. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:314: // Currently opened device-controller couples. The device may or may not be On 2016/06/30 18:24:04, emircan wrote: > It sounds like this is a vector of pairs which is misleading. Can we say > "Currently opened DeviceEntry instances ..."? Done. https://codereview.chromium.org/2108293002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:319: using DeviceStartQueue = std::list<CaptureDeviceStartRequest>; On 2016/06/30 18:24:04, emircan wrote: > Ditto. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
