Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977513004:
Adds multiple forms of MediaStreamTrack constraints (Closed)
|
Created:
4 years, 7 months ago by hta - Chromium Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, haraken, tommyw+watchlist_chromium.org, blink-reviews-bindings_chromium.org, mcasas+watch+mediastream_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
Spec: http://w3c.github.io/mediacapture-main/getusermedia.html#types-for-constrainable-properties
BUG=webrtc:4906
Committed: https://crrev.com/7c0965c6e6012f9bc3fc596ba5c26866f7b9ddea
Cr-Commit-Position: refs/heads/master@{#395857}
Patch Set 1 #Patch Set 2 : Added GYP file for mac builds #Patch Set 3 : Changed tests to test things that work #
Total comments: 2
Patch Set 4 : Cleaned up for review #
Total comments: 7
Patch Set 5 : Review comments addressed #
Total comments: 6
Patch Set 6 : Removed conversions that seem to be automatic #
Total comments: 4
Patch Set 7 : Removed WTF:: prefixes #Patch Set 8 : Removing blink::, using explicit conversions in test #
Total comments: 6
Patch Set 9 : Addressed Tommi's comments #Messages
Total messages: 55 (22 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/1977513004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977513004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/1977513004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977513004/1
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 tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/1977513004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977513004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: There is a layout test that doesn't work.
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: There is a layout test that doesn't work.
==========
hta@chromium.org changed reviewers: + bashi@chromium.org
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: There is a layout test that doesn't work.
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: Tests pass, but syntax [ "a", "b" ] for sequences of strings fail to
work in some circumstances.
==========
@bashi would appreciate it if you could look at the tests in MediaStreamTrackConstraint-getConstraints and see if the working vs non-working forms are as expected.
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/1977513004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977513004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/1977513004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl (right): https://codereview.chromium.org/1977513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:7: // typedef ConstrainLongRange ConstrainLong; Can we remove these old definitions? Or, could you add a comment for what these old definitions are for? (Why we need a list of these typedefs in comment.)
sorry, shouldn't have sent messages that get CCed to so many people while still rooting out the bugs. https://codereview.chromium.org/1977513004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl (right): https://codereview.chromium.org/1977513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:7: // typedef ConstrainLongRange ConstrainLong; On 2016/05/19 06:17:53, Yuki wrote: > Can we remove these old definitions? > > Or, could you add a comment for what these old definitions are for? (Why we > need a list of these typedefs in comment.) these are memory aids so that I can remember what worked previous to this CL. As you can see from "status" this was sent for review in order to share exactly what did not work - I'll remove them before asking for wider review. (Didn't think about all the others who get CCed on reviews, sorry!)
hta@chromium.org changed reviewers: + tommi@chromium.org
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: Tests pass, but syntax [ "a", "b" ] for sequences of strings fail to
work in some circumstances.
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: Depends on https://codereview.chromium.org/1994823002/ to make tests
pass.
==========
This should now be ready for normal review.
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
STATUS: Depends on https://codereview.chromium.org/1994823002/ to make tests
pass.
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
==========
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/1977513004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977513004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:462: const ConstrainLongRange& blinkForm = blinkUnionForm.getAsConstrainLongRange(); nit: You can use |const auto&| for a short hand. https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: } else if (input.hasIdeal()) { To bashi@, We may want to support a conversion from AorB to AorBorC. This else-clause should use |convertStringSequence| defined above. https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.h (right): https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.h:42: class ExceptionState; nit: ExceptionState seems not used. https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.h:44: class MediaTrackConstraintSet; nit: MediaTrackConstraintSet seems not used.
Thanks Yuki - comments addressed! Tommi and bashi, it's on you. https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:462: const ConstrainLongRange& blinkForm = blinkUnionForm.getAsConstrainLongRange(); On 2016/05/24 10:01:03, Yuki wrote: > nit: You can use |const auto&| for a short hand. Done. https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: } else if (input.hasIdeal()) { On 2016/05/24 10:01:03, Yuki wrote: > To bashi@, > > We may want to support a conversion from AorB to AorBorC. This else-clause > should use |convertStringSequence| defined above. would be convenient, but note that then you need to support conversion from AorB, AorC and BorC to AorBorC (and a similar converter explosion for 4-alternative unions).
https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: } else if (input.hasIdeal()) { On 2016/05/24 10:44:31, hta - Chromium wrote: > On 2016/05/24 10:01:03, Yuki wrote: > > To bashi@, > > > > We may want to support a conversion from AorB to AorBorC. This else-clause > > should use |convertStringSequence| defined above. > > would be convenient, but note that then you need to support conversion from > AorB, AorC and BorC to AorBorC (and a similar converter explosion for > 4-alternative unions). Yeah, it would also increase binary size. Switching to use overloading would be a viable option to reduce this kind of pain.
On 2016/05/24 10:54:18, bashi1 wrote: > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp > (right): > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: } > else if (input.hasIdeal()) { > On 2016/05/24 10:44:31, hta - Chromium wrote: > > On 2016/05/24 10:01:03, Yuki wrote: > > > To bashi@, > > > > > > We may want to support a conversion from AorB to AorBorC. This else-clause > > > should use |convertStringSequence| defined above. > > > > would be convenient, but note that then you need to support conversion from > > AorB, AorC and BorC to AorBorC (and a similar converter explosion for > > 4-alternative unions). > > Yeah, it would also increase binary size. Switching to use overloading would be > a viable option to reduce this kind of pain. The new plan in bashi@'s mind should be okay with it, I think. Unless it's actually used, templates never get instantiated. The explosion of the combination doesn't matter, either.
On 2016/05/24 11:00:18, Yuki wrote: > On 2016/05/24 10:54:18, bashi1 wrote: > > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp > > (right): > > > > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: } > > else if (input.hasIdeal()) { > > On 2016/05/24 10:44:31, hta - Chromium wrote: > > > On 2016/05/24 10:01:03, Yuki wrote: > > > > To bashi@, > > > > > > > > We may want to support a conversion from AorB to AorBorC. This > else-clause > > > > should use |convertStringSequence| defined above. > > > > > > would be convenient, but note that then you need to support conversion from > > > AorB, AorC and BorC to AorBorC (and a similar converter explosion for > > > 4-alternative unions). > > > > Yeah, it would also increase binary size. Switching to use overloading would > be > > a viable option to reduce this kind of pain. > > The new plan in bashi@'s mind should be okay with it, I think. Unless it's > actually used, templates never get instantiated. The explosion of the > combination doesn't matter, either. I'm not sure adding such function would have clear benefit at a cost of complexity. Rather, I'd like to switch overloading. It'd be cleaner. (I must admit it will take time so it won't happen soon)
lgtm for union type use. Sorry for the late review...
On 2016/05/24 11:11:59, bashi1 wrote: > On 2016/05/24 11:00:18, Yuki wrote: > > On 2016/05/24 10:54:18, bashi1 wrote: > > > > > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: > } > > > else if (input.hasIdeal()) { > > > On 2016/05/24 10:44:31, hta - Chromium wrote: > > > > On 2016/05/24 10:01:03, Yuki wrote: > > > > > To bashi@, > > > > > > > > > > We may want to support a conversion from AorB to AorBorC. This > > else-clause > > > > > should use |convertStringSequence| defined above. > > > > > > > > would be convenient, but note that then you need to support conversion > from > > > > AorB, AorC and BorC to AorBorC (and a similar converter explosion for > > > > 4-alternative unions). > > > > > > Yeah, it would also increase binary size. Switching to use overloading would > > be > > > a viable option to reduce this kind of pain. > > > > The new plan in bashi@'s mind should be okay with it, I think. Unless it's > > actually used, templates never get instantiated. The explosion of the > > combination doesn't matter, either. > > I'm not sure adding such function would have clear benefit at a cost of > complexity. Rather, I'd like to switch overloading. It'd be cleaner. (I must > admit it will take time so it won't happen soon) Let's discuss offline. It shouldn't be difficult nor complicated. And do not take my comment as "you must implement this feature". I just wanted us to notice that there is a such use case. When designing a new system, it should be good to know many use cases.
On 2016/05/24 11:18:43, Yuki wrote: > On 2016/05/24 11:11:59, bashi1 wrote: > > On 2016/05/24 11:00:18, Yuki wrote: > > > On 2016/05/24 10:54:18, bashi1 wrote: > > > > > > > > > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > > > > File > third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1977513004/diff/60001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:696: > > } > > > > else if (input.hasIdeal()) { > > > > On 2016/05/24 10:44:31, hta - Chromium wrote: > > > > > On 2016/05/24 10:01:03, Yuki wrote: > > > > > > To bashi@, > > > > > > > > > > > > We may want to support a conversion from AorB to AorBorC. This > > > else-clause > > > > > > should use |convertStringSequence| defined above. > > > > > > > > > > would be convenient, but note that then you need to support conversion > > from > > > > > AorB, AorC and BorC to AorBorC (and a similar converter explosion for > > > > > 4-alternative unions). > > > > > > > > Yeah, it would also increase binary size. Switching to use overloading > would > > > be > > > > a viable option to reduce this kind of pain. > > > > > > The new plan in bashi@'s mind should be okay with it, I think. Unless it's > > > actually used, templates never get instantiated. The explosion of the > > > combination doesn't matter, either. > > > > I'm not sure adding such function would have clear benefit at a cost of > > complexity. Rather, I'd like to switch overloading. It'd be cleaner. (I must > > admit it will take time so it won't happen soon) > > Let's discuss offline. It shouldn't be difficult nor complicated. > > And do not take my comment as "you must implement this feature". I just wanted > us to notice that there is a such use case. When designing a new system, it > should be good to know many use cases. My point was it would be better to switch overloading rather than adding a workaround. Just wanted to say my current thought too :)
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
BUG=webrtc:4096
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
Spec:
http://w3c.github.io/mediacapture-main/getusermedia.html#types-for-constraina...
BUG=webrtc:4906
==========
some comments, have to go to a meeting, will continue later https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:505: webForm.setIdeal(WebVector<WebString>(blinkUnionForm.getAsStringSequence())); why do we need the conversion to WebString and WebVector here? https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:511: webForm.setIdeal(WebVector<WebString>(blinkForm.ideal().getAsStringSequence())); same question here and below. it looks like getAsStringSequence already returns the desired type. https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:692: if (input.hasIdeal()) { should hasIdeal() also be gated on hasExact?
Corrected the bug number and added spec link. https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:505: webForm.setIdeal(WebVector<WebString>(blinkUnionForm.getAsStringSequence())); On 2016/05/24 13:07:29, tommi-chrömium wrote: > why do we need the conversion to WebString and WebVector here? The webForm.setIdeal takes that type. I didn't try to compile without it. Since it compiles without it, I'm deleting the conversion; I assume that the WebVector template and the WebString class take care of the conversion for me. https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:511: webForm.setIdeal(WebVector<WebString>(blinkForm.ideal().getAsStringSequence())); On 2016/05/24 13:07:29, tommi-chrömium wrote: > same question here and below. it looks like getAsStringSequence already returns > the desired type. As above.
https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1977513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:505: webForm.setIdeal(WebVector<WebString>(blinkUnionForm.getAsStringSequence())); On 2016/05/24 13:15:29, hta - Chromium wrote: > On 2016/05/24 13:07:29, tommi-chrömium wrote: > > why do we need the conversion to WebString and WebVector here? > > The webForm.setIdeal takes that type. I didn't try to compile without it. Since > it compiles without it, I'm deleting the conversion; I assume that the WebVector > template and the WebString class take care of the conversion for me. > Thanks - yes, the types are compatible, but I think this would otherwise amount to copying the same data twice. https://codereview.chromium.org/1977513004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp (right): https://codereview.chromium.org/1977513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:9: #include "wtf/Vector.h" if we can avoid adding these includes and use the Vector and String types already being used, that would be preferable. https://codereview.chromium.org/1977513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:138: basic.facingMode.setIdeal(WTF::Vector<WTF::String>(1, WTF::String("foo"))); The function prototype expects a WebVector and WebString. Looks like those are the types that the other tests are using. Can we use those instead? (or is there some benefit to using different types?)
Comments addressed. https://codereview.chromium.org/1977513004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp (right): https://codereview.chromium.org/1977513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:9: #include "wtf/Vector.h" On 2016/05/24 13:54:38, tommi-chrömium wrote: > if we can avoid adding these includes and use the Vector and String types > already being used, that would be preferable. These includes are what defines the Vector and String types. IWYU. (and no, nothing in these tests used those types before.) https://codereview.chromium.org/1977513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:138: basic.facingMode.setIdeal(WTF::Vector<WTF::String>(1, WTF::String("foo"))); On 2016/05/24 13:54:38, tommi-chrömium wrote: > The function prototype expects a WebVector and WebString. Looks like those are > the types that the other tests are using. Can we use those instead? (or is > there some benefit to using different types?) After discovering the "using" statements in the header files, I can take the WTF prefixes off. As I discovered in the other source file, Blink seems happy to auto-convert these for me.
Now updated the test file to use explicit conversions where it interfaces to the constraints types, and to embed into the blink:: namespace for brevity. PTAL.
https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp (right): https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:140: basic.facingMode.setIdeal(WebVector<WebString>(Vector<String>(1, String("foo")))); Thanks for making this explicit. It's not exactly what I'm after but it does highlight that we're creating a string to create a vector that we copy into another vector that has a different type of strings (the 3rd type) so that we have the right kind of vector. WebString does support being initialized from static strings. WebVector is a simple template wrapper around std::vector. So, how about this instead: basic.facingMode.setIdeal(WebVector<WebString>(&"foo", 1)); This avoids creating an extra vector and extra string. Also, if you only needed the #includes to be able to use String and Vector, then you shouldn't need those anymore (right?) https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:154: std::vector<WebMediaTrackConstraintSet> advanced; Btw, the general style guide rule for Chromium and Google is to keep variables close to where they're used (ideally RAII). I don't know if it is against the blink convention, but since blink will eventually follow Chromium as far as style goes, I think it doesn't hurt to keep that in mind. https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:159: basic.facingMode.setIdeal(WebVector<WebString>(buffer)); this can be replaced with: WebVector<WebString> buffer(static_cast<size_t>(2u)); buffer[0] = "foo"; buffer[1] = "bar"; basic.facingMode.setIdeal(buffer);
Updated according to comments. Learned something new. https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp (right): https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:140: basic.facingMode.setIdeal(WebVector<WebString>(Vector<String>(1, String("foo")))); On 2016/05/24 21:36:41, tommi-chrömium wrote: > Thanks for making this explicit. It's not exactly what I'm after but it does > highlight that we're creating a string to create a vector that we copy into > another vector that has a different type of strings (the 3rd type) so that we > have the right kind of vector. > > WebString does support being initialized from static strings. WebVector is a > simple template wrapper around std::vector. > So, how about this instead: > > basic.facingMode.setIdeal(WebVector<WebString>(&"foo", 1)); > > This avoids creating an extra vector and extra string. > > Also, if you only needed the #includes to be able to use String and Vector, then > you shouldn't need those anymore (right?) Thanks for finding that constructor - I'd been looking for it, but didn't find it! (BTW, it doesn't work when the argument is a function return value.) https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:154: std::vector<WebMediaTrackConstraintSet> advanced; On 2016/05/24 21:36:41, tommi-chrömium wrote: > Btw, the general style guide rule for Chromium and Google is to keep variables > close to where they're used (ideally RAII). I don't know if it is against the > blink convention, but since blink will eventually follow Chromium as far as > style goes, I think it doesn't hurt to keep that in mind. Moved to where they're used. Since it's two arguments to one function, I kept the declarations of the two together. https://codereview.chromium.org/1977513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp:159: basic.facingMode.setIdeal(WebVector<WebString>(buffer)); On 2016/05/24 21:36:41, tommi-chrömium wrote: > this can be replaced with: > > WebVector<WebString> buffer(static_cast<size_t>(2u)); > buffer[0] = "foo"; > buffer[1] = "bar"; > basic.facingMode.setIdeal(buffer); So WebVector is in fact mutable, not just assignable, unlike a lot of other Web* types... changed.
lgtm. thanks for the patience :)
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/1977513004/#ps160001 (title: "Addressed Tommi's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977513004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977513004/160001
Message was sent while issue was closed.
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
Spec:
http://w3c.github.io/mediacapture-main/getusermedia.html#types-for-constraina...
BUG=webrtc:4906
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
Spec:
http://w3c.github.io/mediacapture-main/getusermedia.html#types-for-constraina...
BUG=webrtc:4906
==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
Spec:
http://w3c.github.io/mediacapture-main/getusermedia.html#types-for-constraina...
BUG=webrtc:4906
==========
to
==========
Adds multiple forms of MediaStreamTrack constraints
This allows one to write "width: 56" with the same meaning as
"width: {ideal: 56}", and similar for other constraints.
Spec:
http://w3c.github.io/mediacapture-main/getusermedia.html#types-for-constraina...
BUG=webrtc:4906
Committed: https://crrev.com/7c0965c6e6012f9bc3fc596ba5c26866f7b9ddea
Cr-Commit-Position: refs/heads/master@{#395857}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7c0965c6e6012f9bc3fc596ba5c26866f7b9ddea Cr-Commit-Position: refs/heads/master@{#395857} |
