Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2224103002:
Package video capture skeleton as Mojo Shell Service (Closed)
|
Created:
4 years, 4 months ago by chfremer Modified:
4 years, 4 months ago Reviewers:
Ken Rockot(use gerrit already), emircan, mcasas, yzshen1, Tom Sepez, Ben Goodger (Google) CC:
chromium-reviews, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@VideoMojoSkeleton2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2].
* Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service.
* Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback.
* Hooked the build into gn_only section of the main BUILD.gn file
BUG=584797
TEST=Build of gn_only succeeds. Test in video_capture_unittests passes.
[1] https://codereview.chromium.org/2155303002/
[2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
Committed: https://crrev.com/db24ccb5ac27c5c19c20980eca528816cbccf809
Cr-Commit-Position: refs/heads/master@{#412286}
Patch Set 1 #Patch Set 2 : Added comments to mojom files. #
Total comments: 10
Patch Set 3 : mcasas' comments #
Total comments: 15
Patch Set 4 : yzshen's comments #
Total comments: 10
Patch Set 5 : mcasas' comments #Patch Set 6 : Fix for bots #
Total comments: 4
Patch Set 7 : ben's comments #Dependent Patchsets: Messages
Total messages: 42 (19 generated)
Description was changed from ========== Clean up first test. Change prefix exe to mojo -> test passes format Implement service test. Currently does not work. Added skeleton for a test. Add service wrapper around Mojo implementation. Steps towards packaging as service. BUG= ========== to ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into the main BUILD.gn file TODO: Move build hook-up point from win/linux only section to something more general. Where should this go? TODO: Where should we hook-up the test target in order to have the test run on the trybots/commit queue? BUG=584797 TEST= [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org
Description was changed from ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into the main BUILD.gn file TODO: Move build hook-up point from win/linux only section to something more general. Where should this go? TODO: Where should we hook-up the test target in order to have the test run on the trybots/commit queue? BUG=584797 TEST= [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into gn_only section of the main BUILD.gn file BUG=584797 TEST=Build of gn_only succeeds. Test in video_capture_unittests passes. [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
Patchset #1 (id:1) has been deleted
chfremer@chromium.org changed reviewers: + rockot@chromium.org
looking good, just some comments rockot@ will know more about the service parts. https://codereview.chromium.org/2224103002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2224103002/diff/40001/BUILD.gn#newcode697 BUILD.gn:697: "//services/video_capture:video_capture_unittests", I'd also add the new video_capture_unittests it some bots; search for "capture_unittests" in e.g. src/testing/buildbot/chromium.{win, mac, linux, android}.json https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... File services/video_capture/BUILD.gn (right): https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/BUILD.gn:68: ":unittest_manifest", I think these one-liners can be collapsed in gn, anyway try gn format --in-place services/video_capture/BUILD.gn and see if it looks better? https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... File services/video_capture/service.h (right): https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/service.h:35: std::unique_ptr<VideoCaptureDeviceFactoryImpl> device_factory_; const? And make it forward declared. https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/service_unittest.cc:48: scoped_refptr<MockClient> client_; In unittests we usually don't bother having this ref-counted, just have a const std::unique_ptr and use base::Unretained() in l.57, since anyway VideoCaptureServiceTest's lifetime will be long enough. https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/service_unittest.cc:58: &wait_loop)); Instead of passing |wait_loop| here, consider what is done in [1]: base::RunLoop run_loop; base::Closure quit_closure = run_loop.QuitClosure(); EXPECT_CALL(...method...).WillOnce(RunClosure(quit_closure)); DoStuff(); run_loop.Run(); with RunClosure defined pervasively in "our" files as ACTION_P(RunClosure, closure) { closure.Run(); } [1] https://cs.chromium.org/chromium/src/content/renderer/media/video_track_recor...
@mcasas PTAL https://codereview.chromium.org/2224103002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2224103002/diff/40001/BUILD.gn#newcode697 BUILD.gn:697: "//services/video_capture:video_capture_unittests", On 2016/08/10 15:50:53, mcasas wrote: > I'd also add the new video_capture_unittests it some bots; > search for "capture_unittests" in e.g. > src/testing/buildbot/chromium.{win, mac, linux, android}.json Excellent suggestion. Definitely want to have this run on the bots. I had a look at the capture_unittests examples and decided that enabling the tests on the bots is complex enough for me to make it an extra CL. I added this as an item (CL1.5) in the design document [1]. [1] https://docs.google.com/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijv... https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... File services/video_capture/BUILD.gn (right): https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/BUILD.gn:68: ":unittest_manifest", On 2016/08/10 15:50:53, mcasas wrote: > I think these one-liners can be collapsed in gn, anyway > try > gn format --in-place services/video_capture/BUILD.gn > and see if it looks better? I applied the command-line and found that it wants it the way it is. It also seems to do its auto-formatting on git cl format, which is probably why it already happened to be what it wanted. https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... File services/video_capture/service.h (right): https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/service.h:35: std::unique_ptr<VideoCaptureDeviceFactoryImpl> device_factory_; On 2016/08/10 15:50:54, mcasas wrote: > const? And make it forward declared. Done. https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/service_unittest.cc:48: scoped_refptr<MockClient> client_; On 2016/08/10 15:50:54, mcasas wrote: > In unittests we usually don't bother having this ref-counted, > just have a const std::unique_ptr and use base::Unretained() > in l.57, since anyway VideoCaptureServiceTest's lifetime > will be long enough. Done. https://codereview.chromium.org/2224103002/diff/40001/services/video_capture/... services/video_capture/service_unittest.cc:58: &wait_loop)); On 2016/08/10 15:50:54, mcasas wrote: > Instead of passing |wait_loop| here, consider what is > done in [1]: > > base::RunLoop run_loop; > base::Closure quit_closure = run_loop.QuitClosure(); > > EXPECT_CALL(...method...).WillOnce(RunClosure(quit_closure)); > DoStuff(); > > run_loop.Run(); > > with RunClosure defined pervasively in "our" files as > > ACTION_P(RunClosure, closure) { > closure.Run(); > } > > [1] > https://cs.chromium.org/chromium/src/content/renderer/media/video_track_recor... Done.
From my side is an el-gee-tee-em, I'll defer to rockot@ since there's plenty of service thingies around I'm not familiar with :)
chfremer@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_client.mojom (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_client.mojom:10: // VideoCaptureDevice style nit: please add a '.' at the end. https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_factory.mojom (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_factory.mojom:48: GetSupportedFormats(VideoCaptureDeviceDescriptor device_descriptor) I wonder why we need to send a VideoCaptureDeviceDescriptor structure. To me some fields in that structure are not necessary to identify the device. WDYT? https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service.cc (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service.cc:11: : device_factory_(new VideoCaptureDeviceFactoryImpl()) {} It seems you could make this member a VideoCaptureDeviceFactoryImpl instance directly, instead of using a unique_ptr. https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service.h (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service.h:5: #ifndef SERVICES_VIDEO_CAPTURE_SERVICE_H_ Please make the file name reflect the class name (i.e., video_capture_service). https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service_manifest.json:7: "all": ["*"] I think we will need to restrict access to this service because video capturing is a privileged service? Are you planning to do that in a future CL? https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service_unittest.cc:47: std::unique_ptr<MockClient> client_; Does it make sense to use: MockClient client_;
yzshen@: PTAL https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_client.mojom (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_client.mojom:10: // VideoCaptureDevice On 2016/08/12 17:46:50, yzshen1 wrote: > style nit: please add a '.' at the end. Done. https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_factory.mojom (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_factory.mojom:48: GetSupportedFormats(VideoCaptureDeviceDescriptor device_descriptor) On 2016/08/12 17:46:50, yzshen1 wrote: > I wonder why we need to send a VideoCaptureDeviceDescriptor structure. To me > some fields in that structure are not necessary to identify the device. WDYT? Good observation. This is a weakness in the design. In the refactoring before this Mojofication, I was about to change the interface such that it would use unique string identifiers instead of the device descriptor structs, but then I ended up not doing it. I think it was because of the extra complexity involved in having to generate ids and build and manage a map from ids to descriptors. How strongly do you feel about this? Would you be okay with leaving it as is or do you prefer a design with unique ids? https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service.cc (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service.cc:11: : device_factory_(new VideoCaptureDeviceFactoryImpl()) {} On 2016/08/12 17:46:50, yzshen1 wrote: > It seems you could make this member a VideoCaptureDeviceFactoryImpl instance > directly, instead of using a unique_ptr. Done. https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service_manifest.json:7: "all": ["*"] On 2016/08/12 17:46:50, yzshen1 wrote: > I think we will need to restrict access to this service because video capturing > is a privileged service? Are you planning to do that in a future CL? By restricting access, do you mean offering different class-type entries in the provided section here or are there other mechanisms beyond this? If the former, then yes, I am going to add different entries for the different types of clients. https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service_unittest.cc:47: std::unique_ptr<MockClient> client_; On 2016/08/12 17:46:50, yzshen1 wrote: > Does it make sense to use: > MockClient client_; Done.
https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_factory.mojom (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_factory.mojom:48: GetSupportedFormats(VideoCaptureDeviceDescriptor device_descriptor) On 2016/08/12 18:32:27, chfremer wrote: > On 2016/08/12 17:46:50, yzshen1 wrote: > > I wonder why we need to send a VideoCaptureDeviceDescriptor structure. To me > > some fields in that structure are not necessary to identify the device. WDYT? > > Good observation. This is a weakness in the design. In the refactoring before > this Mojofication, I was about to change the interface such that it would use > unique string identifiers instead of the device descriptor structs, but then I > ended up not doing it. I think it was because of the extra complexity involved > in having to generate ids and build and manage a map from ids to descriptors. > > How strongly do you feel about this? Would you be okay with leaving it as is or > do you prefer a design with unique ids? It seems nice to do that. But you or other experts of this code may know better whether it is a must or it could be done later. https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service_manifest.json:7: "all": ["*"] On 2016/08/12 18:32:27, chfremer wrote: > On 2016/08/12 17:46:50, yzshen1 wrote: > > I think we will need to restrict access to this service because video > capturing > > is a privileged service? Are you planning to do that in a future CL? > > By restricting access, do you mean offering different class-type entries in the > provided section here or are there other mechanisms beyond this? > If the former, then yes, I am going to add different entries for the different > types of clients. Okay. Thanks!
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/12 20:05:53, yzshen1 wrote: > On 2016/08/12 18:32:27, chfremer wrote: > > On 2016/08/12 17:46:50, yzshen1 wrote: > > > I wonder why we need to send a VideoCaptureDeviceDescriptor structure. To me > > > some fields in that structure are not necessary to identify the device. > WDYT? > > > > Good observation. This is a weakness in the design. In the refactoring before > > this Mojofication, I was about to change the interface such that it would use > > unique string identifiers instead of the device descriptor structs, but then I > > ended up not doing it. I think it was because of the extra complexity involved > > in having to generate ids and build and manage a map from ids to descriptors. > > > > How strongly do you feel about this? Would you be okay with leaving it as is > or > > do you prefer a design with unique ids? > > It seems nice to do that. But you or other experts of this code may know better > whether it is a must or it could be done later. mcasas@: As the expert of this code, could you add your thoughts on this, please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
a few nits, otherwise LGTM https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... File services/video_capture/service_main.cc (right): https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_main.cc:12: } Pro-tip: when uploading a CL, you can set the similarity like, e.g. `git cl upload -t "bla" --similarity=75` and that'll remove this false-diffs, instead correctly indicating that service_main.cc is a new file. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_unittest.cc:30: descriptors)); nit: MOCKs don't need to name the parameters. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_unittest.cc:49: TEST_F(VideoCaptureServiceTest, EnumerateDeviceDescriptorsCallbackArrives) { nit: write a minimal explanation, like: // Tests that calling EnumerateDeviceDescriptors() produces a list of those. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_unittest.cc:54: ; nit: remove ; https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.cc (right): https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:12: std::vector<mojom::VideoCaptureDeviceDescriptorPtr> descriptors; nit: s/descriptors/empty_descriptors/ ?
https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_factory.mojom (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_factory.mojom:48: GetSupportedFormats(VideoCaptureDeviceDescriptor device_descriptor) On 2016/08/12 20:05:53, yzshen1 wrote: > On 2016/08/12 18:32:27, chfremer wrote: > > On 2016/08/12 17:46:50, yzshen1 wrote: > > > I wonder why we need to send a VideoCaptureDeviceDescriptor structure. To me > > > some fields in that structure are not necessary to identify the device. > WDYT? > > > > Good observation. This is a weakness in the design. In the refactoring before > > this Mojofication, I was about to change the interface such that it would use > > unique string identifiers instead of the device descriptor structs, but then I > > ended up not doing it. I think it was because of the extra complexity involved > > in having to generate ids and build and manage a map from ids to descriptors. > > > > How strongly do you feel about this? Would you be okay with leaving it as is > or > > do you prefer a design with unique ids? > > It seems nice to do that. But you or other experts of this code may know better > whether it is a must or it could be done later. On ToT, VideoCaptureManager has to keep all this information between the enumeration and individual VideoCaptureDevices, so it's needed in a first step, to keep the VCDFactory separated from the VCDevice, and to avoid having to reenumerate to pull up those details again, but we can contemplate simplification-refactorings down the line. TODO+bug, I'd say.
Addressed comments. yzshen@: Would you like to take another look, or would you be comfortable signing off? https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... File services/video_capture/service.h (right): https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... services/video_capture/service.h:5: #ifndef SERVICES_VIDEO_CAPTURE_SERVICE_H_ On 2016/08/12 17:46:50, yzshen1 wrote: > Please make the file name reflect the class name (i.e., video_capture_service). Done. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... File services/video_capture/service_main.cc (right): https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_main.cc:12: } On 2016/08/12 21:45:27, mcasas wrote: > Pro-tip: when uploading a CL, you can set the similarity > like, e.g. `git cl upload -t "bla" --similarity=75` and that'll > remove this false-diffs, instead correctly indicating that > service_main.cc is a new file. Acknowledged. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_unittest.cc:30: descriptors)); On 2016/08/12 21:45:27, mcasas wrote: > nit: MOCKs don't need to name the parameters. Done. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_unittest.cc:49: TEST_F(VideoCaptureServiceTest, EnumerateDeviceDescriptorsCallbackArrives) { On 2016/08/12 21:45:27, mcasas wrote: > nit: write a minimal explanation, like: > // Tests that calling EnumerateDeviceDescriptors() produces a list of those. Done. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/service_unittest.cc:54: ; On 2016/08/12 21:45:27, mcasas wrote: > nit: remove ; Done. https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.cc (right): https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:12: std::vector<mojom::VideoCaptureDeviceDescriptorPtr> descriptors; On 2016/08/12 21:45:27, mcasas wrote: > nit: s/descriptors/empty_descriptors/ ? Done.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/12 22:23:24, chfremer wrote: > Addressed comments. > > yzshen@: Would you like to take another look, or would you be comfortable > signing off? > > https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... > File services/video_capture/service.h (right): > > https://codereview.chromium.org/2224103002/diff/60001/services/video_capture/... > services/video_capture/service.h:5: #ifndef SERVICES_VIDEO_CAPTURE_SERVICE_H_ > On 2016/08/12 17:46:50, yzshen1 wrote: > > Please make the file name reflect the class name (i.e., > video_capture_service). > > Done. > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > File services/video_capture/service_main.cc (right): > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > services/video_capture/service_main.cc:12: } > On 2016/08/12 21:45:27, mcasas wrote: > > Pro-tip: when uploading a CL, you can set the similarity > > like, e.g. `git cl upload -t "bla" --similarity=75` and that'll > > remove this false-diffs, instead correctly indicating that > > service_main.cc is a new file. > > Acknowledged. > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > File services/video_capture/service_unittest.cc (right): > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > services/video_capture/service_unittest.cc:30: descriptors)); > On 2016/08/12 21:45:27, mcasas wrote: > > nit: MOCKs don't need to name the parameters. > > Done. > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > services/video_capture/service_unittest.cc:49: TEST_F(VideoCaptureServiceTest, > EnumerateDeviceDescriptorsCallbackArrives) { > On 2016/08/12 21:45:27, mcasas wrote: > > nit: write a minimal explanation, like: > > // Tests that calling EnumerateDeviceDescriptors() produces a list of those. > > Done. > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > services/video_capture/service_unittest.cc:54: ; > On 2016/08/12 21:45:27, mcasas wrote: > > nit: remove ; > > Done. > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > File services/video_capture/video_capture_device_factory_impl.cc (right): > > https://codereview.chromium.org/2224103002/diff/80001/services/video_capture/... > services/video_capture/video_capture_device_factory_impl.cc:12: > std::vector<mojom::VideoCaptureDeviceDescriptorPtr> descriptors; > On 2016/08/12 21:45:27, mcasas wrote: > > nit: s/descriptors/empty_descriptors/ ? > > Done. LGTM
chfremer@chromium.org changed reviewers: + ben@chromium.org, tsepez@chromium.org
tsepez@: PTAL *.mojom files (only change is added comments) ben@: PTAL BUILD.gn (added new target "video_capture_unittests" to "gn_only")
RS LGTM on adding comments to .mojom
https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... File services/video_capture/public/interfaces/video_capture_device.mojom (right): https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... services/video_capture/public/interfaces/video_capture_device.mojom:8: import "services/video_capture/public/interfaces/video_capture_format.mojom"; I don't see this file in the change? https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... services/video_capture/service_manifest.json:7: "all": ["*"] Help me understand the interfaces you expect to bind from tests. Are some unsafe to be seen by renderers? By other specific process types?
ben@: PTAL https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... File services/video_capture/public/interfaces/video_capture_device.mojom (right): https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... services/video_capture/public/interfaces/video_capture_device.mojom:8: import "services/video_capture/public/interfaces/video_capture_format.mojom"; On 2016/08/15 22:56:47, Ben Goodger (Google) wrote: > I don't see this file in the change? That file (video_capture_format.mojom) was not modified in this CL. It was added in the previous CL at https://codereview.chromium.org/2155303002/. https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2224103002/diff/120001/services/video_capture... services/video_capture/service_manifest.json:7: "all": ["*"] On 2016/08/15 22:56:48, Ben Goodger (Google) wrote: > Help me understand the interfaces you expect to bind from tests. > > Are some unsafe to be seen by renderers? By other specific process types? The service is supposed to have two types of clients. The first client is a component called VideoCaptureManager, which lives in the Browser process and whose responsibility it is to distribute access to (and video frames from) the video capture devices to potentially multiple clients. These clients typically live in the renderers. The second type of client is a component test for the service. There may or may not be some interfaces that are exclusive to the testing client in the end. At this stage, we only have the testing client, so I am not (yet) distinguishing different classes of provided-interfaces. Is there anything I need to do to hide this service from renderers?
> On 2016/08/15 22:56:48, Ben Goodger (Google) wrote: > > Help me understand the interfaces you expect to bind from tests. > > > > Are some unsafe to be seen by renderers? By other specific process types? > > The service is supposed to have two types of clients. The first client is a > component called VideoCaptureManager, which lives in the Browser process and > whose responsibility it is to distribute access to (and video frames from) the > video capture devices to potentially multiple clients. These clients typically > live in the renderers. > > The second type of client is a component test for the service. > > There may or may not be some interfaces that are exclusive to the testing client > in the end. At this stage, we only have the testing client, so I am not (yet) > distinguishing different classes of provided-interfaces. > > Is there anything I need to do to hide this service from renderers? If granting access to all interfaces is only ever intended to be possible from tests, rename the class "tests". You don't need to do anything special to hide it from renderers, since the renderer policy would have to specify either a specific class or specific interfaces to bind them. To bind the subset of interfaces the browser requires from exe:content_browser, you'd need to list those in exe:content_browser's manifest.
ben@: PTAL Renamed provided interface class "all" to "tests".
On 2016/08/16 16:11:05, chfremer wrote: > ben@: PTAL > > Renamed provided interface class "all" to "tests". lgtm
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, yzshen@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2224103002/#ps140001 (title: "ben's comments")
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 ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into gn_only section of the main BUILD.gn file BUG=584797 TEST=Build of gn_only succeeds. Test in video_capture_unittests passes. [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into gn_only section of the main BUILD.gn file BUG=584797 TEST=Build of gn_only succeeds. Test in video_capture_unittests passes. [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into gn_only section of the main BUILD.gn file BUG=584797 TEST=Build of gn_only succeeds. Test in video_capture_unittests passes. [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== This is a follow-up to the skeleton CL [1] which is currently under review. For the bigger picture, please refer to the design document at [2]. * Added BUILD.gn for building the video_capture bindings, skeleton implementation, and package it as a Mojo Shell Service. * Added a simple Shell Service Test to confirm that the service can be reached and responds with a callback. * Hooked the build into gn_only section of the main BUILD.gn file BUG=584797 TEST=Build of gn_only succeeds. Test in video_capture_unittests passes. [1] https://codereview.chromium.org/2155303002/ [2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Committed: https://crrev.com/db24ccb5ac27c5c19c20980eca528816cbccf809 Cr-Commit-Position: refs/heads/master@{#412286} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/db24ccb5ac27c5c19c20980eca528816cbccf809 Cr-Commit-Position: refs/heads/master@{#412286} |
