Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721113002:
getUserMedia: handle the device starting status report. (Closed)
|
Created:
3 years, 9 months ago by braveyao Modified:
3 years, 9 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, avayvod+watch_chromium.org, imcheng+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, jasonroberts+watch_google.com, chromium-apps-reviews_chromium.org, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptiongetUserMedia: handle the device starting status report.
Chromium used to report device started immediately at requesting user media,
despite the device operation results. This causes several kinds of problems.
- Notification of capture starting is shown before device starts.
- gUM gets successful callback even if the device fails to start.
So we decide to make gUM return the real device starting states
(here we focus on the video capturing).
This is the second part of the complete modification.
The design doc is here, https://goo.gl/WTZkEl
The previous CL is here, https://codereview.chromium.org/2673373003/
This CL does severals things when a OnStarted event from device arrives
- VideoCaptureImpl calls the state_update_cb, informing the track is completed.
- When all tracks are completed, return gUM with successful callback and send
a message to browser process to show the notification in UI.
- Other related changes due to the asynchronous OnStarted event.
- Remove some test cases of desktop_capture, which tests nothing before
(due to the always-successful gUM callback) and fails after this cl
(gUM will get errorcallback if stream can't be started successfully).
BUG=674959, 651910
Review-Url: https://codereview.chromium.org/2721113002
Cr-Commit-Position: refs/heads/master@{#456895}
Committed: https://chromium.googlesource.com/chromium/src/+/1941549656951c9dbd61984b3fd7f8414a3536a6
Patch Set 1 #
Total comments: 28
Patch Set 2 : rebase to Mar2 #Patch Set 3 : address comments on PS#1 #
Total comments: 6
Patch Set 4 : address comments on PS#3 #
Total comments: 18
Patch Set 5 : address comments on PS#4 #
Total comments: 2
Patch Set 6 : address nits on PS#5 #
Total comments: 6
Patch Set 7 : address nits on PS#6 #
Total comments: 5
Patch Set 8 : address nits #Messages
Total messages: 91 (73 generated)
The CQ bit was checked by braveyao@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 checked by braveyao@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by braveyao@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.
braveyao@chromium.org changed reviewers: + chfremer@chromium.org, emircan@chromium.org, miu@chromium.org
Hi, this is the second part of changes to getUserMedia. miu@, please help to take a look at content/renderer/media/ emircan@, please help to take a look at content/browser/renderer_host/media/ +cc: chfremer@, in case you're interested.
A few questions and recommendations. https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/desktop_capture/test.js (right): https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/desktop_capture/test.js:127: // fake web-content/window source id. I don't immediately see why the changes in this CL make these existing test cases impossible/obsolete. https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:807: // job of VideoCaptureController to report OnStarted event to all the clients. I like the test! For the description comment here I recommend using a sentence that starts with "Tests that ...". The goal of the description comment is to allow the reader quickly understand what the test does without having to read the actual code. In this case "Tests that the VideoCaptureController reports OnStarted() to all clients, even if they connect after VideoCaptureController::OnStarted() has been invoked." https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:370: EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _)); Why are we adding these expectations? It appears that the original purpose of the test was to verify the VideoCaptureObserver calls, and for that, the other calls are uninteresting, and therefore should not be explicitly expected. https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:746: EXPECT_CALL(*frame_observer_, OnStarted(_)); Hmm. The purpose of this test is somewhat unclear to me. The name suggests that the purpose of the test is to verify that the WillSuspendDevice() and WillResumeDevice() calls arrive as expected. The callback that may arrive to |listener_->Opened()|, |frame_observer_|->OnStarted() and |listener_->Closed()| are not relevant for testing that, and therefore the EXPECT_CALLs for those are diluting the focus from what actually matters. Ideally those callbacks should be covered by a separate test (which they already might be), and then we should remove the EXPECT_CALLs from this one. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... File content/renderer/media/video_capture_impl.h (right): https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl.h:158: bool maybe_refresh_frame_; The "maybe_" prefix seems redundant, since it is clear that the bool may be true or false. I recommend naming this "should_request_refresh_frame_on_started_". The comment "This only has effect on screen capture." is a dangerous claim to make here, because it assumes knowledge of what happens outside of this class. I understand that you want to document the effect on the larger context, but this is not a good location for putting this information, because when the larger context changes, who will know that there is documentation hiding in here that needs to be updated? https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... File content/renderer/media/video_capture_impl_manager_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_manager_unittest.cc:144: .RetiresOnSaturation(); If this worked before, it will probably still work after the change. While reading, I stumbled over the two EXPECT_CALL statements for OnStarted(). I understand that the second EXPECT_CALL is separate from the first, because it is supposed to call |quit_closure|. However, it is not clear to me whether or not gmock guarantees that it will match the first |kNumClient - 1| invocations to the first (and not the second) EXPECT_CALL unless we use InSequence, see the section on RetiresOnSaturation in [1]. [1] https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... File content/renderer/media/video_capture_impl_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:182: // |MockMojoVideoCaptureHost::Start()| by far. I understand the added code in the EXPECT_CALL, and I also understand why it is needed. But I do not understand the explanation given in this comment. I would be okay with just removing it. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:186: })); Instead of adding this to all the test cases, you may be able to do an ON_CALL(...).WillByDefault(...) in the constructor or SetUp() method of VideoCaptureImplTest. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:375: TEST_F(VideoCaptureImplTest, BufferReceivedBeforeOnStarted) { Great to see a test case for this! I can think of at least two more test cases worth testing. 1.) We want to verify that no RequestRefreshFrame() is sent in case that no frames arrived before the OnStateChanged(STARTED) call. 2.) Assuming it is possible that additional OnStateChanged(STARTED) calls arrive after the first one, we want to verify what happens in such cases. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:385: EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, kBufferId, _)); Are these two EXPECT_CALL relevant for this test case? If not, please remove. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:399: EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 1); Is all of the shutdown logic starting at line 394 relevant for this test case? If not, please remove. https://codereview.chromium.org/2721113002/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:289: std::unique_ptr<VideoCaptureDevice> device = Thanks for these cleanups!
The CQ bit was checked by braveyao@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by braveyao@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by braveyao@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...
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #2 (id:120001) has been deleted
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 checked by braveyao@chromium.org to run a CQ dry run
Patchset #3 (id:160001) has been deleted
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.
Thanks for the comments! All addressed. PTAL! https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/desktop_capture/test.js (right): https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/desktop_capture/test.js:127: // fake web-content/window source id. On 2017/03/02 17:56:10, chfremer wrote: > I don't immediately see why the changes in this CL make these existing test > cases impossible/obsolete. Revised the comments a bit. Is it better? https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:807: // job of VideoCaptureController to report OnStarted event to all the clients. On 2017/03/02 17:56:10, chfremer wrote: > I like the test! > For the description comment here I recommend using a sentence that starts with > "Tests that ...". > The goal of the description comment is to allow the reader quickly understand > what the test does without having to read the actual code. > > In this case "Tests that the VideoCaptureController reports OnStarted() to all > clients, even if they connect after VideoCaptureController::OnStarted() has been > invoked." Done. https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:370: EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _)); On 2017/03/02 17:56:10, chfremer wrote: > Why are we adding these expectations? > > It appears that the original purpose of the test was to verify the > VideoCaptureObserver calls, and for that, the other calls are uninteresting, and > therefore should not be explicitly expected. Done. No particular reason. Just to remove the GMOCK Warning that these mocked mehtods are called. https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:746: EXPECT_CALL(*frame_observer_, OnStarted(_)); On 2017/03/02 17:56:10, chfremer wrote: > Hmm. The purpose of this test is somewhat unclear to me. > The name suggests that the purpose of the test is to verify that the > WillSuspendDevice() and WillResumeDevice() calls arrive as expected. > > The callback that may arrive to |listener_->Opened()|, > |frame_observer_|->OnStarted() and |listener_->Closed()| are not relevant for > testing that, and therefore the EXPECT_CALLs for those are diluting the focus > from what actually matters. > > Ideally those callbacks should be covered by a separate test (which they already > might be), and then we should remove the EXPECT_CALLs from this one. This is needed because StartClient() are called twice, due to the modification in VideoCaptureConthroller. So either we don't do EXPECT_CALL(OnStarted) or expect it twice. Here I choose to expect it twice. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... File content/renderer/media/video_capture_impl.h (right): https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl.h:158: bool maybe_refresh_frame_; On 2017/03/02 17:56:10, chfremer wrote: > The "maybe_" prefix seems redundant, since it is clear that the bool may be true > or false. I recommend naming this "should_request_refresh_frame_on_started_". > > The comment "This only has effect on screen capture." is a dangerous claim to > make here, because it assumes knowledge of what happens outside of this class. I > understand that you want to document the effect on the larger context, but this > is not a good location for putting this information, because when the larger > context changes, who will know that there is documentation hiding in here that > needs to be updated? > Done. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... File content/renderer/media/video_capture_impl_manager_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_manager_unittest.cc:144: .RetiresOnSaturation(); On 2017/03/02 17:56:10, chfremer wrote: > If this worked before, it will probably still work after the change. > > While reading, I stumbled over the two EXPECT_CALL statements for OnStarted(). I > understand that the second EXPECT_CALL is separate from the first, because it is > supposed to call |quit_closure|. However, it is not clear to me whether or not > gmock guarantees that it will match the first |kNumClient - 1| invocations to > the first (and not the second) EXPECT_CALL unless we use InSequence, see the > section on RetiresOnSaturation in [1]. > > [1] > https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md Done. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... File content/renderer/media/video_capture_impl_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:182: // |MockMojoVideoCaptureHost::Start()| by far. On 2017/03/02 17:56:10, chfremer wrote: > I understand the added code in the EXPECT_CALL, and I also understand why it is > needed. But I do not understand the explanation given in this comment. I would > be okay with just removing it. Done. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:186: })); On 2017/03/02 17:56:10, chfremer wrote: > Instead of adding this to all the test cases, you may be able to do an > ON_CALL(...).WillByDefault(...) in the constructor or SetUp() method of > VideoCaptureImplTest. Done. Thanks a lot for the suggestion! https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:375: TEST_F(VideoCaptureImplTest, BufferReceivedBeforeOnStarted) { On 2017/03/02 17:56:10, chfremer wrote: > Great to see a test case for this! > > I can think of at least two more test cases worth testing. > 1.) We want to verify that no RequestRefreshFrame() is sent in case that no > frames arrived before the OnStateChanged(STARTED) call. > 2.) Assuming it is possible that additional OnStateChanged(STARTED) calls arrive > after the first one, we want to verify what happens in such cases. Done. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:385: EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, kBufferId, _)); On 2017/03/02 17:56:10, chfremer wrote: > Are these two EXPECT_CALL relevant for this test case? If not, please remove. Kinda. I want to verify that the frame is received and released correctly. https://codereview.chromium.org/2721113002/diff/60001/content/renderer/media/... content/renderer/media/video_capture_impl_unittest.cc:399: EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 1); On 2017/03/02 17:56:10, chfremer wrote: > Is all of the shutdown logic starting at line 394 relevant for this test case? > If not, please remove. StopCapture and the above two lines are needed for this unittest. Remove others. https://codereview.chromium.org/2721113002/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:289: std::unique_ptr<VideoCaptureDevice> device = On 2017/03/02 17:56:10, chfremer wrote: > Thanks for these cleanups! Acknowledged.
https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/desktop_capture/test.js (right): https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/desktop_capture/test.js:127: // fake web-content/window source id. On 2017/03/03 17:53:12, braveyao wrote: > On 2017/03/02 17:56:10, chfremer wrote: > > I don't immediately see why the changes in this CL make these existing test > > cases impossible/obsolete. > > Revised the comments a bit. Is it better? The extra information in the new comment seems helpful, so definitely better. But I still don't understand why the change in this CL causes tests that worked before to no longer work. Isn't the only change that the response to getUserMedia and chooseDesktopMedia now arrives a bit later than before? https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:370: EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _)); On 2017/03/03 17:53:12, braveyao wrote: > On 2017/03/02 17:56:10, chfremer wrote: > > Why are we adding these expectations? > > > > It appears that the original purpose of the test was to verify the > > VideoCaptureObserver calls, and for that, the other calls are uninteresting, > and > > therefore should not be explicitly expected. > > Done. > No particular reason. Just to remove the GMOCK Warning that these mocked mehtods > are called. Ah, I understand. I think the GMOCK Warning also recommends to not add EXPECT_CALL just to get rid of the warning, so we are probably doing the right thing by removing them. https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... File content/renderer/media/video_capture_impl_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:321: }), In order to remove the duplication with the WillByDefault setup above, here you could probably use DoAll(DoDefault(), SaveArg<2>(¶ms)) see https://github.com/google/googlemock/blob/master/googlemock/docs/CheatSheet.md https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:366: .WillOnce(InvokeWithoutArgs(NotSendOnStarted)); You may be able to avoid "hijacking" the InvokeWithoutArgs Action by defining a custom Action ACTION(DoNothing) { } at the top of the file and here doing a .WillOnce(DoNothing()) See section "Defining Actions" in https://github.com/google/googlemock/blob/master/googlemock/docs/CheatSheet.md https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:389: .WillOnce(InvokeWithoutArgs(NotSendOnStarted)); Why do we override the default action here just to manually perform it in line 394?
Description was changed from ========== getUserMedia: handle the device starting status report. Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the second part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The previous CL is here, https://codereview.chromium.org/2673373003/ This CL does severals things when a OnStarted event from device arrives - VideoCaptureImpl calls the state_update_cb, informing the track is completed. - When all tracks are completed, return gUM with successful callback and send a message to browser process to show the notification in UI. - Other related changes due to the asynchronous OnStarted event. BUG=674959, 651910 ========== to ========== getUserMedia: handle the device starting status report. Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the second part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The previous CL is here, https://codereview.chromium.org/2673373003/ This CL does severals things when a OnStarted event from device arrives - VideoCaptureImpl calls the state_update_cb, informing the track is completed. - When all tracks are completed, return gUM with successful callback and send a message to browser process to show the notification in UI. - Other related changes due to the asynchronous OnStarted event. - Remove some test cases of desktop_capture, which tests nothing before (due to the always-successful gUM callback) and fails after this cl (gUM will get errorcallback if stream can't be started successfully). BUG=674959, 651910 ==========
The CQ bit was checked by braveyao@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.
Thanks a lot! All addressed. PTAL! https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/desktop_capture/test.js (right): https://codereview.chromium.org/2721113002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/desktop_capture/test.js:127: // fake web-content/window source id. On 2017/03/03 19:08:33, chfremer wrote: > On 2017/03/03 17:53:12, braveyao wrote: > > On 2017/03/02 17:56:10, chfremer wrote: > > > I don't immediately see why the changes in this CL make these existing test > > > cases impossible/obsolete. > > > > Revised the comments a bit. Is it better? > > The extra information in the new comment seems helpful, so definitely better. > But I still don't understand why the change in this CL causes tests that worked > before to no longer work. Isn't the only change that the response to > getUserMedia and chooseDesktopMedia now arrives a bit later than before? Before this cl, gUM always get successful callback, no matter device-start succeeds or fails. After this cl, gUM will get real device-start result through success-callback or error-callback. Because of the fake source id, we can't successfully create stream of tab/windwo capture. So the removed cases will fail. And updated the cl description about this. https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:370: EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _)); On 2017/03/03 19:08:33, chfremer wrote: > On 2017/03/03 17:53:12, braveyao wrote: > > On 2017/03/02 17:56:10, chfremer wrote: > > > Why are we adding these expectations? > > > > > > It appears that the original purpose of the test was to verify the > > > VideoCaptureObserver calls, and for that, the other calls are uninteresting, > > and > > > therefore should not be explicitly expected. > > > > Done. > > No particular reason. Just to remove the GMOCK Warning that these mocked > mehtods > > are called. > > Ah, I understand. > I think the GMOCK Warning also recommends to not add EXPECT_CALL just to get rid > of the warning, so we are probably doing the right thing by removing them. Acknowledged. https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... File content/renderer/media/video_capture_impl_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:321: }), On 2017/03/03 19:08:34, chfremer wrote: > In order to remove the duplication with the WillByDefault setup above, here you > could probably use DoAll(DoDefault(), SaveArg<2>(¶ms)) see > https://github.com/google/googlemock/blob/master/googlemock/docs/CheatSheet.md According to the doc, "DoDefault() cannot be used inside a composite action". And the test result shows there is runtime error as expected. https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:366: .WillOnce(InvokeWithoutArgs(NotSendOnStarted)); On 2017/03/03 19:08:33, chfremer wrote: > You may be able to avoid "hijacking" the InvokeWithoutArgs Action by defining a > custom Action > > ACTION(DoNothing) { } > > at the top of the file and here doing a > > .WillOnce(DoNothing()) > > See section "Defining Actions" in > https://github.com/google/googlemock/blob/master/googlemock/docs/CheatSheet.md Done. Thanks! https://codereview.chromium.org/2721113002/diff/180001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:389: .WillOnce(InvokeWithoutArgs(NotSendOnStarted)); On 2017/03/03 19:08:33, chfremer wrote: > Why do we override the default action here just to manually perform it in line > 394? Done.
lgtm
https://codereview.chromium.org/2721113002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc (left): https://codereview.chromium.org/2721113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:220: // tabShareWithoutAudioGetStream() I'm worried about these deletions. Instead, can we comment them out; and add a TODO comment and file a crbug to get them working again? FWICT, either test hooks are needed to fake-out the chooser dialog UI, or we need to create interactive_ui_tests for these. https://codereview.chromium.org/2721113002/diff/200001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2721113002/diff/200001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:195: bool has_received_onstarted_; Instead of adding this separate boolean, how about adding a VideoCaptureState called VIDEO_CAPTURE_STATE_STARTING? This seems to be an actual concept that is known and used in both the browser and renderer code. https://cs.chromium.org/chromium/src/content/common/media/video_capture.h?rcl... https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (left): https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:118: state_update_cb.Run(VIDEO_CAPTURE_STATE_STARTED); Per my earlier comment, how about keeping this, but calling it with the new VIDEO_CAPTURE_STATE_STARTING state? https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:385: state_ = VIDEO_CAPTURE_STATE_STARTED; Retain this, but it should be changed to: state_ = VIDEO_CAPTURE_STATE_STARTING; https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:111: if (state_ == VIDEO_CAPTURE_STATE_ERROR) { Can we re-organize this complex logic into a flattened switch-statement, to better clarify that this logic is correctly handling things in each possible |state_|? Something like: switch (state_) { case VIDEO_CAPTURE_STATE_STARTING: case VIDEO_CAPTURE_STATE_STARTED: ...L118 to L122 goes here... return; case VIDEO_CAPTURE_STATE_STOPPING: case VIDEO_CAPTURE_STATE_STOPPED: ...L124 to L126 goes here... return; case VIDEO_CAPTURE_STATE_PAUSED: case VIDEO_CAPTURE_STATE_RESUMED: case VIDEO_CAPTURE_STATE_PAUSED: ...L128 to L138 goes here... return; case VIDEO_CAPTURE_STATE_ENDED: // Do nothing because... return; case VIDEO_CAPTURE_STATE_ERROR: state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR); return; } https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:129: if (state_ == VIDEO_CAPTURE_STATE_STARTED) Note: Here's one reason I suggest the switch-statement reorganization: This if-statement is meaningless because |state_| cannot be VIDEO_CAPTURE_STATE_STARTED at this point. https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:192: if (should_request_refresh_frame_on_started_) { Suggestion: Instead of this logic, what if VideoCaptureImpl just unconditionally requests a refresh frame once reaching the STARTED state? IMHO, it makes sense to provide the client with a first frame as soon as possible. Also, it wouldn't be horribly redundant because capture devices can ignore the request if they know they are going to provide a new frame soon anyway.
The CQ bit was checked by braveyao@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by braveyao@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:220001) has been deleted
The CQ bit was checked by braveyao@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.
Thanks miu@ so much for the suggestions! PTAL. https://codereview.chromium.org/2721113002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc (left): https://codereview.chromium.org/2721113002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:220: // tabShareWithoutAudioGetStream() On 2017/03/06 22:37:33, miu wrote: > I'm worried about these deletions. Instead, can we comment them out; and add a > TODO comment and file a crbug to get them working again? FWICT, either test > hooks are needed to fake-out the chooser dialog UI, or we need to create > interactive_ui_tests for these. Done. https://codereview.chromium.org/2721113002/diff/200001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2721113002/diff/200001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:195: bool has_received_onstarted_; On 2017/03/06 22:37:33, miu wrote: > Instead of adding this separate boolean, how about adding a VideoCaptureState > called VIDEO_CAPTURE_STATE_STARTING? This seems to be an actual concept that is > known and used in both the browser and renderer code. > > https://cs.chromium.org/chromium/src/content/common/media/video_capture.h?rcl... Done. https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (left): https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:118: state_update_cb.Run(VIDEO_CAPTURE_STATE_STARTED); On 2017/03/06 22:37:33, miu wrote: > Per my earlier comment, how about keeping this, but calling it with the new > VIDEO_CAPTURE_STATE_STARTING state? This does no good and will cause troubles to the unittests. So I still don't want to keep it. https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:385: state_ = VIDEO_CAPTURE_STATE_STARTED; On 2017/03/06 22:37:33, miu wrote: > Retain this, but it should be changed to: > > state_ = VIDEO_CAPTURE_STATE_STARTING; Done. https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:111: if (state_ == VIDEO_CAPTURE_STATE_ERROR) { On 2017/03/06 22:37:33, miu wrote: > Can we re-organize this complex logic into a flattened switch-statement, to > better clarify that this logic is correctly handling things in each possible > |state_|? > > Something like: > > switch (state_) { > case VIDEO_CAPTURE_STATE_STARTING: > case VIDEO_CAPTURE_STATE_STARTED: > ...L118 to L122 goes here... > return; > case VIDEO_CAPTURE_STATE_STOPPING: > case VIDEO_CAPTURE_STATE_STOPPED: > ...L124 to L126 goes here... > return; > case VIDEO_CAPTURE_STATE_PAUSED: > case VIDEO_CAPTURE_STATE_RESUMED: > case VIDEO_CAPTURE_STATE_PAUSED: > ...L128 to L138 goes here... > return; > case VIDEO_CAPTURE_STATE_ENDED: > // Do nothing because... > return; > case VIDEO_CAPTURE_STATE_ERROR: > state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR); > return; > } Done. By my understanding, the check, "if (clients_pending_on_restart_.count(client_id) || clients_.count(client_id))" is useless here. Please help to double check! https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:129: if (state_ == VIDEO_CAPTURE_STATE_STARTED) On 2017/03/06 22:37:33, miu wrote: > Note: Here's one reason I suggest the switch-statement reorganization: This > if-statement is meaningless because |state_| cannot be > VIDEO_CAPTURE_STATE_STARTED at this point. Done. Also found the STARTING state is necessary for StopDevice(). Thanks! https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:192: if (should_request_refresh_frame_on_started_) { On 2017/03/06 22:37:33, miu wrote: > Suggestion: Instead of this logic, what if VideoCaptureImpl just unconditionally > requests a refresh frame once reaching the STARTED state? IMHO, it makes sense > to provide the client with a first frame as soon as possible. Also, it wouldn't > be horribly redundant because capture devices can ignore the request if they > know they are going to provide a new frame soon anyway. Done. Good to know! But it will cause many GMock Warnings. And most cases don't care the calling to RequestRefreshFrames. So I didn't add EXPECT_CALL there. Is it really good?
PS5 lgtm % nits: https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:111: if (state_ == VIDEO_CAPTURE_STATE_ERROR) { On 2017/03/08 22:02:52, braveyao wrote: > On 2017/03/06 22:37:33, miu wrote: > > Can we re-organize this complex logic into a flattened switch-statement, to > > better clarify that this logic is correctly handling things in each possible > > |state_|? > > > > Something like: > > > > switch (state_) { > > case VIDEO_CAPTURE_STATE_STARTING: > > case VIDEO_CAPTURE_STATE_STARTED: > > ...L118 to L122 goes here... > > return; > > case VIDEO_CAPTURE_STATE_STOPPING: > > case VIDEO_CAPTURE_STATE_STOPPED: > > ...L124 to L126 goes here... > > return; > > case VIDEO_CAPTURE_STATE_PAUSED: > > case VIDEO_CAPTURE_STATE_RESUMED: > > case VIDEO_CAPTURE_STATE_PAUSED: > > ...L128 to L138 goes here... > > return; > > case VIDEO_CAPTURE_STATE_ENDED: > > // Do nothing because... > > return; > > case VIDEO_CAPTURE_STATE_ERROR: > > state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR); > > return; > > } > > Done. > By my understanding, the check, > "if (clients_pending_on_restart_.count(client_id) || > clients_.count(client_id))" > is useless here. Please help to double check! Yes, that's my understanding too. :) https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:192: if (should_request_refresh_frame_on_started_) { On 2017/03/08 22:02:52, braveyao wrote: > On 2017/03/06 22:37:33, miu wrote: > > Suggestion: Instead of this logic, what if VideoCaptureImpl just > unconditionally > > requests a refresh frame once reaching the STARTED state? IMHO, it makes sense > > to provide the client with a first frame as soon as possible. Also, it > wouldn't > > be horribly redundant because capture devices can ignore the request if they > > know they are going to provide a new frame soon anyway. > > Done. > Good to know! > But it will cause many GMock Warnings. And most cases don't care the calling to > RequestRefreshFrames. So I didn't add EXPECT_CALL there. Is it really good? Yes, this should be fine. It's up to you, but it would probably be reasonable to add the EXPECT_CALL() to reduce log spam; and because it's an acceptable behavior change. https://codereview.chromium.org/2721113002/diff/240001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2721113002/diff/240001/content/renderer/media... content/renderer/media/video_capture_impl.cc:143: // Do nothing becase these states aren't taken here. nit: Please add NOT_REACHED() here for safety. The comment should say something like: The internal |state_| is never set to PAUSED/RESUMED since VideoCaptureImpl is not modified by those.
The CQ bit was checked by braveyao@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.
Thanks miu@! emircan@, need your review on content/browser/renderer_host/media/. Thanks! https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:111: if (state_ == VIDEO_CAPTURE_STATE_ERROR) { On 2017/03/10 23:56:56, miu wrote: > On 2017/03/08 22:02:52, braveyao wrote: > > On 2017/03/06 22:37:33, miu wrote: > > > Can we re-organize this complex logic into a flattened switch-statement, to > > > better clarify that this logic is correctly handling things in each possible > > > |state_|? > > > > > > Something like: > > > > > > switch (state_) { > > > case VIDEO_CAPTURE_STATE_STARTING: > > > case VIDEO_CAPTURE_STATE_STARTED: > > > ...L118 to L122 goes here... > > > return; > > > case VIDEO_CAPTURE_STATE_STOPPING: > > > case VIDEO_CAPTURE_STATE_STOPPED: > > > ...L124 to L126 goes here... > > > return; > > > case VIDEO_CAPTURE_STATE_PAUSED: > > > case VIDEO_CAPTURE_STATE_RESUMED: > > > case VIDEO_CAPTURE_STATE_PAUSED: > > > ...L128 to L138 goes here... > > > return; > > > case VIDEO_CAPTURE_STATE_ENDED: > > > // Do nothing because... > > > return; > > > case VIDEO_CAPTURE_STATE_ERROR: > > > state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR); > > > return; > > > } > > > > Done. > > By my understanding, the check, > > "if (clients_pending_on_restart_.count(client_id) || > > clients_.count(client_id))" > > is useless here. Please help to double check! > > Yes, that's my understanding too. :) Acknowledged. https://codereview.chromium.org/2721113002/diff/200001/content/renderer/media... content/renderer/media/video_capture_impl.cc:192: if (should_request_refresh_frame_on_started_) { On 2017/03/10 23:56:56, miu wrote: > On 2017/03/08 22:02:52, braveyao wrote: > > On 2017/03/06 22:37:33, miu wrote: > > > Suggestion: Instead of this logic, what if VideoCaptureImpl just > > unconditionally > > > requests a refresh frame once reaching the STARTED state? IMHO, it makes > sense > > > to provide the client with a first frame as soon as possible. Also, it > > wouldn't > > > be horribly redundant because capture devices can ignore the request if they > > > know they are going to provide a new frame soon anyway. > > > > Done. > > Good to know! > > But it will cause many GMock Warnings. And most cases don't care the calling > to > > RequestRefreshFrames. So I didn't add EXPECT_CALL there. Is it really good? > > Yes, this should be fine. It's up to you, but it would probably be reasonable to > add the EXPECT_CALL() to reduce log spam; and because it's an acceptable > behavior change. Acknowledged. https://codereview.chromium.org/2721113002/diff/240001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2721113002/diff/240001/content/renderer/media... content/renderer/media/video_capture_impl.cc:143: // Do nothing becase these states aren't taken here. On 2017/03/10 23:56:56, miu wrote: > nit: Please add NOT_REACHED() here for safety. The comment should say something > like: The internal |state_| is never set to PAUSED/RESUMED since > VideoCaptureImpl is not modified by those. Done.
lgtm % nits. https://codereview.chromium.org/2721113002/diff/260001/content/browser/media/... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/260001/content/browser/media/... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:581: const std::pair<SkColor, gfx::Size> color_and_size = It is encouraged to use auto here, see the last item on https://google.github.io/styleguide/cppguide.html#auto. Also, can you use |const std::pair<SkColor, gfx::Size>&| to avoid making a copy? https://codereview.chromium.org/2721113002/diff/260001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2721113002/diff/260001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:1768: DeviceRequest* request = FindRequest(label); DeviceRequest* const request https://codereview.chromium.org/2721113002/diff/260001/content/renderer/media... File content/renderer/media/video_capture_impl_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/260001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:366: .WillOnce(DoNothing()); Do you need to call DoNothing() here? You can replace it with Times(1).
The CQ bit was checked by braveyao@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.
braveyao@chromium.org changed reviewers: + dcheng@chromium.org, sergeyu@chromium.org, tommi@chromium.org
Thanks emircan@! Need more OWNER's review: sergeyu@, please take a look at chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc. dcheng@, please take a look at content/common/media/media_stream_messages.h tommi@, please take a look at content/common/media/video_capture.h https://codereview.chromium.org/2721113002/diff/260001/content/browser/media/... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/260001/content/browser/media/... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:581: const std::pair<SkColor, gfx::Size> color_and_size = On 2017/03/13 21:03:35, emircan wrote: > It is encouraged to use auto here, see the last item on > https://google.github.io/styleguide/cppguide.html#auto. Also, can you use |const > std::pair<SkColor, gfx::Size>&| to avoid making a copy? Done. https://codereview.chromium.org/2721113002/diff/260001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2721113002/diff/260001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:1768: DeviceRequest* request = FindRequest(label); On 2017/03/13 21:03:35, emircan wrote: > DeviceRequest* const request Done. https://codereview.chromium.org/2721113002/diff/260001/content/renderer/media... File content/renderer/media/video_capture_impl_unittest.cc (right): https://codereview.chromium.org/2721113002/diff/260001/content/renderer/media... content/renderer/media/video_capture_impl_unittest.cc:366: .WillOnce(DoNothing()); On 2017/03/13 21:03:35, emircan wrote: > Do you need to call DoNothing() here? You can replace it with Times(1). Oh, ".Times(1)" doesn't work here simply. And we want to emphasize the difference of this case that OnStarted will be called later. So keep it as it.
ipc lgtm https://codereview.chromium.org/2721113002/diff/280001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2721113002/diff/280001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:1772: if (request->ui_proxy.get()) { Nit: no .get()
Lgtm. happy to see this fixed. It was recently fixed for audio, in case someone was wondering.
If you link to a (design) doc from CL description, please make sure that the doc is public. LGTM provided that crbug.com/699201 is treated as P2, not P3. https://codereview.chromium.org/2721113002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc (right): https://codereview.chromium.org/2721113002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:220: /** please use C++ style comments (//..) to comment the test. Otherwise it's harder to see in review and in git-diff that the test is commented. Also that would allow to avoid moving any tests https://codereview.chromium.org/2721113002/diff/280001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/desktop_capture/test.js (right): https://codereview.chromium.org/2721113002/diff/280001/chrome/test/data/exten... chrome/test/data/extensions/api_test/desktop_capture/test.js:149: /** use // for comments please And then you also wouldn't have to move any tests.
The CQ bit was checked by braveyao@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.
Thanks all! https://codereview.chromium.org/2721113002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc (right): https://codereview.chromium.org/2721113002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc:220: /** On 2017/03/14 19:43:34, Sergey Ulanov wrote: > please use C++ style comments (//..) to comment the test. > Otherwise it's harder to see in review and in git-diff that the test is > commented. > Also that would allow to avoid moving any tests Done. https://codereview.chromium.org/2721113002/diff/280001/content/browser/render... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/2721113002/diff/280001/content/browser/render... content/browser/renderer_host/media/media_stream_manager.cc:1772: if (request->ui_proxy.get()) { On 2017/03/14 06:28:00, dcheng wrote: > Nit: no .get() Done.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chfremer@chromium.org, miu@chromium.org, emircan@chromium.org, tommi@chromium.org, sergeyu@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2721113002/#ps300001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1489534941196150,
"parent_rev": "9dc27cfc2e8320bd65f4da58debc5569cf8de443", "commit_rev":
"1941549656951c9dbd61984b3fd7f8414a3536a6"}
Message was sent while issue was closed.
Description was changed from ========== getUserMedia: handle the device starting status report. Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the second part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The previous CL is here, https://codereview.chromium.org/2673373003/ This CL does severals things when a OnStarted event from device arrives - VideoCaptureImpl calls the state_update_cb, informing the track is completed. - When all tracks are completed, return gUM with successful callback and send a message to browser process to show the notification in UI. - Other related changes due to the asynchronous OnStarted event. - Remove some test cases of desktop_capture, which tests nothing before (due to the always-successful gUM callback) and fails after this cl (gUM will get errorcallback if stream can't be started successfully). BUG=674959, 651910 ========== to ========== getUserMedia: handle the device starting status report. Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the second part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The previous CL is here, https://codereview.chromium.org/2673373003/ This CL does severals things when a OnStarted event from device arrives - VideoCaptureImpl calls the state_update_cb, informing the track is completed. - When all tracks are completed, return gUM with successful callback and send a message to browser process to show the notification in UI. - Other related changes due to the asynchronous OnStarted event. - Remove some test cases of desktop_capture, which tests nothing before (due to the always-successful gUM callback) and fails after this cl (gUM will get errorcallback if stream can't be started successfully). BUG=674959, 651910 Review-Url: https://codereview.chromium.org/2721113002 Cr-Commit-Position: refs/heads/master@{#456895} Committed: https://chromium.googlesource.com/chromium/src/+/1941549656951c9dbd61984b3fd7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:300001) as https://chromium.googlesource.com/chromium/src/+/1941549656951c9dbd61984b3fd7... |
