Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1581103002:
Use new-style constraints for device selection. (Closed)
|
Created:
4 years, 11 months ago by hta - Chromium Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, jam, kinuko+watch, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, philipj_slow, posciak+watch_chromium.org, tommyw+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@constraints-errors Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse new-style constraints for device selection.
This CL connects the new-style constraints to the data that is
sent to the browser process to select a capture device.
This CL validates the info-passing functions; later CLs will
remove all usage by Chrome of the old-style constraint values
(but these are kept for now to allow us to do incremental
introduction of the new functions.)
It also adds a set of constraints whose names were found in libjingle source.
BUG=543997
Committed: https://crrev.com/2e01bbd052f7938ad88f8f4d51aa18e1394e6e69
Cr-Commit-Position: refs/heads/master@{#370008}
Patch Set 1 #Patch Set 2 : More constraints from Libjingle #
Total comments: 18
Patch Set 3 : Review comments #
Total comments: 2
Messages
Total messages: 28 (13 generated)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581103002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581103002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581103002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581103002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) BUG=543997 ========== to ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) BUG=543997 ==========
hta@chromium.org changed reviewers: + guidou@chromium.org, jochen@chromium.org, tommi@chromium.org
This is a follow-on CL to 1570813002.
Description was changed from ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) BUG=543997 ========== to ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) It also adds a set of constraints whose names were found in libjingle source. BUG=543997 ==========
lgtm
https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:43: void CopyVector(const blink::WebVector<blink::WebString> source, const &? https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:50: void CopyFirstString(const blink::WebVector<blink::WebString> source, const & https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:63: DLOG(WARNING) << "Copying audio constraints"; clean up? (and below) https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:72: // TODO: Get alternatives only mentioned in advanced array. assign todo to someone (did git cl presubmit complain?) https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:74: &(controls->audio.alternate_device_ids)); nit: no need for () https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:75: if (!audio_basic.hotwordEnabled.matches(false)) if not matches false... is that right? https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:87: if (controls->video.device_ids.size() > 0) I guess you'll clean these up too? https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:194: .renderToAssociatedSink.matches(false)) { indent 4 spaces.... actually, I'd be curious to see what git cl format does in this case? https://codereview.chromium.org/1581103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1581103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:274: WTF_LOG(Media, "Copying a device ID"); remove?
lgtm once tommi is happy
lgtm. I didn't actually get a ping back for reviewing again? Maybe coments sitting in draft? https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:75: if (!audio_basic.hotwordEnabled.matches(false)) On 2016/01/14 15:31:27, tommi-chromium wrote: > if not matches false... is that right? ah, nevermind, makes sense now. To make sure hotword_enabled is always set to the right value, you could do: controls->hotword_enabled = !audio_basic.hotwordEnabled.matches(false); or (I'm assuming) controls->hotword_enabled = audio_basic.hotwordEnabled.matches(true); https://codereview.chromium.org/1581103002/diff/40001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/1581103002/diff/40001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:65: &(controls->audio.stream_source)); nit: skip the extra parenthesis (like in line 70) https://codereview.chromium.org/1581103002/diff/40001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:66: CopyVector(audio_basic.deviceId.exact(), &(controls->audio.device_ids)); and here
Now I've addressed the comments, and also rebased and tested that it still runs.... took a little work. Should be good to go now. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:43: void CopyVector(const blink::WebVector<blink::WebString> source, On 2016/01/14 15:31:27, tommi-chromium wrote: > const &? Done. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:50: void CopyFirstString(const blink::WebVector<blink::WebString> source, On 2016/01/14 15:31:27, tommi-chromium wrote: > const & Done. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:63: DLOG(WARNING) << "Copying audio constraints"; On 2016/01/14 15:31:27, tommi-chromium wrote: > clean up? (and below) Done. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:74: &(controls->audio.alternate_device_ids)); On 2016/01/14 15:31:27, tommi-chromium wrote: > nit: no need for () Done. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:75: if (!audio_basic.hotwordEnabled.matches(false)) On 2016/01/14 15:31:27, tommi-chromium wrote: > if not matches false... is that right? It seemed the simplest way to say "if I'm allowed to make it false, I will; if I'm forced to make it true, I will". If it's empty, both false and true will match. So this is where the default is generated. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:87: if (controls->video.device_ids.size() > 0) On 2016/01/14 15:31:27, tommi-chromium wrote: > I guess you'll clean these up too? Done. https://codereview.chromium.org/1581103002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:194: .renderToAssociatedSink.matches(false)) { On 2016/01/14 15:31:27, tommi-chromium wrote: > indent 4 spaces.... actually, I'd be curious to see what git cl format does in > this case? It said + if (!user_media_request.audioConstraints() + .basic() + .renderToAssociatedSink.matches(false)) { It also changed } else if { to } else if { which directly contravenes Webkit style guide.... not nice for style-crossing CLs. https://codereview.chromium.org/1581103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1581103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:274: WTF_LOG(Media, "Copying a device ID"); On 2016/01/14 15:31:27, tommi-chromium wrote: > remove? Done.
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581103002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org Link to the patchset: https://codereview.chromium.org/1581103002/#ps40001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581103002/40001
Message was sent while issue was closed.
Description was changed from ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) It also adds a set of constraints whose names were found in libjingle source. BUG=543997 ========== to ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) It also adds a set of constraints whose names were found in libjingle source. BUG=543997 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) It also adds a set of constraints whose names were found in libjingle source. BUG=543997 ========== to ========== Use new-style constraints for device selection. This CL connects the new-style constraints to the data that is sent to the browser process to select a capture device. This CL validates the info-passing functions; later CLs will remove all usage by Chrome of the old-style constraint values (but these are kept for now to allow us to do incremental introduction of the new functions.) It also adds a set of constraints whose names were found in libjingle source. BUG=543997 Committed: https://crrev.com/2e01bbd052f7938ad88f8f4d51aa18e1394e6e69 Cr-Commit-Position: refs/heads/master@{#370008} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2e01bbd052f7938ad88f8f4d51aa18e1394e6e69 Cr-Commit-Position: refs/heads/master@{#370008} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
