Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1318393002:
Refactor "track options" to be a dictionary. (Closed)
|
Created:
5 years, 3 months ago by hta - Chromium Modified:
5 years, 2 months ago CC:
blink-reviews, tommyw+watchlist_chromium.org, Guido Urdaneta, tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor "track options" to be a dictionary.
This dictionary has members "optional" and "mandatory" for
backwards compatibility; the intent is that constraints using
these members will be treated like old-style constraints, while
constraints without them will be treated like spec-style.
BUG=537590
Committed: https://crrev.com/0e6149ca421d75a825064359ac01b8a71f5c3c13
Cr-Commit-Position: refs/heads/master@{#353519}
Patch Set 1 #Patch Set 2 : Rebase to merged repositories #Patch Set 3 : Used MediaTrackConstraintSet in union types #
Total comments: 18
Patch Set 4 : Review comments #Patch Set 5 : Rebase to HEAD #
Total comments: 14
Patch Set 6 : More review comments addressed #
Total comments: 13
Patch Set 7 : Address more review comments #Patch Set 8 : Another review comment set #
Messages
Total messages: 30 (9 generated)
hta@chromium.org changed reviewers: + guidou@chromium.org, tommi@chromium.org
Time for review. Cannot be landed yet (commit of the union patch failed in queue).
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/1318393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318393002/40001
https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:128: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) Can you use range for here? https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:136: for (size_t i = 0; i < optionalConstraints.size(); ++i) { Range for? https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:59: // Do nothing. Is this possible? Should there be a NOTREACHED here?
I'd like some help on finding "range for". https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:128: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) On 2015/10/05 12:34:12, Guido Urdaneta wrote: > Can you use range for here? can you point to an examle of range for? I think that's a C++ 11 feature I've never used. This is copy/pasted code (I intend to remove the code I pasted from, but it turned out to have one more user). https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:59: // Do nothing. On 2015/10/05 12:34:12, Guido Urdaneta wrote: > Is this possible? > Should there be a NOTREACHED here? It is possible. When you call getUserMedia({video: true}), options will be null for audio (not false). The tests actually caught this one. I find the if/then(nothing)/elsif(something)/else structure awkward, but didn't find a simpler way to write it.
https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:128: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) On 2015/10/05 12:54:01, hta - Chromium wrote: > On 2015/10/05 12:34:12, Guido Urdaneta wrote: > > Can you use range for here? > > can you point to an examle of range for? I think that's a C++ 11 feature I've > never used. > This is copy/pasted code (I intend to remove the code I pasted from, but it > turned out to have one more user). Since this is not an STL container I don't know if it range for actually applies. http://en.cppreference.com/w/cpp/language/range-for
hta@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:120: Dictionary mandatoryConstraintsDictionary = constraintsIn.mandatory(); Parsing of |mandatoryConstraintsDictionary| and |optionalConstraintsVector| in this method are identical to the parse() function above. Rather than duplicating the code, could we perhaps two static (or anonymous) functions for parsing the "mandatory" and "_optional" members, both of which are called directly by the create() methods below? https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:5: micro nit: please link to the spec // https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStream... https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:7: // Dictionary should be MediaTrackConstraintSet. crbug/524424 blocks this. nit: crbug.com/524424 You may want to phrase this as a TODO to resolve when the issue is fixed. The information in lines 8-10 would be of more value in the bug. (As the canonical place for information on the issue.) https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:4: micro nit: Please link to the spec: // https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaTrackC... https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:8: // Next line doesn't compile - crbug/524357 What does "Next line doesn't compile" mean? Try-bots are proving you wrong :) nit: crbug.com/524357 https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:11: DOMString foobar; Please remove this line.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Significant blush for the work-in-progress comments I'd forgotten to remove! See if you like this split into helper functions better - there was less common code than I thought at first. https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:120: Dictionary mandatoryConstraintsDictionary = constraintsIn.mandatory(); On 2015/10/05 13:49:09, Peter Beverloo wrote: > Parsing of |mandatoryConstraintsDictionary| and |optionalConstraintsVector| in > this method are identical to the parse() function above. > > Rather than duplicating the code, could we perhaps two static (or anonymous) > functions for parsing the "mandatory" and "_optional" members, both of which are > called directly by the create() methods below? There are subtle differences in the control flow, but significant chunks can be extracted. Does this look better? https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:5: On 2015/10/05 13:49:09, Peter Beverloo wrote: > micro nit: please link to the spec > > // > https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStream... Done. https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:7: // Dictionary should be MediaTrackConstraintSet. crbug/524424 blocks this. On 2015/10/05 13:49:09, Peter Beverloo wrote: > nit: crbug.com/524424 > > You may want to phrase this as a TODO to resolve when the issue is fixed. The > information in lines 8-10 would be of more value in the bug. (As the canonical > place for information on the issue.) This is embarassing ... 524424 is fixed, and I forgot to delete the comment! https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl (right): https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:4: On 2015/10/05 13:49:09, Peter Beverloo wrote: > micro nit: Please link to the spec: > > // > https://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaTrackC... Done. https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:8: // Next line doesn't compile - crbug/524357 On 2015/10/05 13:49:09, Peter Beverloo wrote: > What does "Next line doesn't compile" mean? Try-bots are proving you wrong :) > > nit: crbug.com/524357 That I'd forgotten to remove the comment :-( (embarraassed) https://codereview.chromium.org/1318393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:11: DOMString foobar; On 2015/10/05 13:49:09, Peter Beverloo wrote: > Please remove this line. Done.
lgtm % few nits https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:58: mandatoryConstraintsVector.append(WebMediaConstraint(iter->key, iter->value)); nit: Why don't use add them to |mandatory| immediately rather than first writing them to a temporary vector, then copying the contents to it? There are no failure clauses after this loop, so that should be safe to do. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:66: constraint.getPropertyNames(localNames); Would it make sense to check for the return value here, as you do for the function above (using |ok|)? https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:131: static bool parse(const MediaTrackConstraintSet& constraintsIn, WebVector<WebMediaConstraint>& optional, WebVector<WebMediaConstraint>& mandatory) I'm still not a huge fan of the overloading, but it matches the style of this file. If you, or anybody, would touch this file in the future, a good thing to consider would be to refactor the interface in a way that would allow you to write unit tests for it. The functions you extracted would actually be perfect for that already. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:142: const Vector<Dictionary> optionalConstraints = constraintsIn.optional(); const T&, otherwise you're making a copy. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl (right): https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6: // http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamC... consistency micro nit: We usually use the following syntax in IDL files: ===== // copyright // https://... [] idl statements ===== (note the blank lines, absence of Specification: line.) https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6: // http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamC... https:// please https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl (right): https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:9: // The "mandatory" and "optional" members are retained for conformance micro nit: optional -> _optional
I think it's OK now... WebVector is a very limited vector type. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:58: mandatoryConstraintsVector.append(WebMediaConstraint(iter->key, iter->value)); On 2015/10/08 09:50:25, Peter Beverloo wrote: > nit: Why don't use add them to |mandatory| immediately rather than first writing > them to a temporary vector, then copying the contents to it? There are no > failure clauses after this loop, so that should be safe to do. It seems that mandatory is a WebVector, and I can't find any append operation in WebVector.h. Seems that WebVectors can only be modified by assign(). https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:66: constraint.getPropertyNames(localNames); On 2015/10/08 09:50:25, Peter Beverloo wrote: > Would it make sense to check for the return value here, as you do for the > function above (using |ok|)? Done. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:131: static bool parse(const MediaTrackConstraintSet& constraintsIn, WebVector<WebMediaConstraint>& optional, WebVector<WebMediaConstraint>& mandatory) On 2015/10/08 09:50:25, Peter Beverloo wrote: > I'm still not a huge fan of the overloading, but it matches the style of this > file. > > If you, or anybody, would touch this file in the future, a good thing to > consider would be to refactor the interface in a way that would allow you to > write unit tests for it. The functions you extracted would actually be perfect > for that already. I thought Blink only believed in JS-level tests... now I see that there's actually a gunit user here too. Will definitely consider writing gunit tests in the future! A next step in the refactoring is to make the other caller of create() use MediaTrackConstraintSet instead of Dictionary; this will delete the overload. I overloaded these functions becaue I think the names of the functions are good, and did not want to make them harder to read; I agree about it generally being a confusing thing. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:142: const Vector<Dictionary> optionalConstraints = constraintsIn.optional(); On 2015/10/08 09:50:25, Peter Beverloo wrote: > const T&, otherwise you're making a copy. Done. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl (right): https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6: // http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamC... On 2015/10/08 09:50:25, Peter Beverloo wrote: > https:// please Done. https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamConstraints.idl:6: // http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamC... On 2015/10/08 09:50:25, Peter Beverloo wrote: > consistency micro nit: We usually use the following syntax in IDL files: > > ===== > // copyright > > // https://... > > [] idl statements > ===== > > (note the blank lines, absence of Specification: line.) Thanks - these are the kind of style issues that are hard to gather from the documentation! https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl (right): https://codereview.chromium.org/1318393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl:9: // The "mandatory" and "optional" members are retained for conformance On 2015/10/08 09:50:25, Peter Beverloo wrote: > micro nit: optional -> _optional Done.
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1318393002/#ps100001 (title: "More review comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1318393002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318393002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) range based for loop? (looks like HashMap supports that) https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:71: String key = localNames[0]; is this copy necessary? (const String&?) https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:149: Dictionary constraint = optionalConstraints[i]; can we avoid creating a copy or is this not really a copy? https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185: constraints.initialize(optional, mandatory); is the ownership of the vectors given to the constraints, or are they copied? In general I worry that the code is parsing and copying strings a lot and that's not a good convention to start.
A few more const & added. Tests still pass, so I guess it works. Asking for more guidance on range loops, and pushing back on refactoring the Web* classes to do less copying at this point. https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) On 2015/10/09 14:49:21, tommi wrote: > range based for loop? (looks like HashMap supports that) Guido suggested that a couple of cycles ago, but I found it a bit confusing to figure out how to write one, so punted. Is it obvious enough to write in a comment how it should look? https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:71: String key = localNames[0]; On 2015/10/09 14:49:21, tommi wrote: > is this copy necessary? (const String&?) Done. https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:149: Dictionary constraint = optionalConstraints[i]; On 2015/10/09 14:49:21, tommi wrote: > can we avoid creating a copy or is this not really a copy? The old code (above) used a getter that returned a Dictionary. Here we're dealing with a Vector<Dictionary>, which should allow for references. Done. https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185: constraints.initialize(optional, mandatory); On 2015/10/09 14:49:21, tommi wrote: > is the ownership of the vectors given to the constraints, or are they copied? > In general I worry that the code is parsing and copying strings a lot and that's > not a good convention to start. It's not a start, it's been that way for a long time. I want to get the design better for the new constraints, but worry more about not introducing changes in behavior for the old constraints. This is still the old stuff. WebMediaConstraints has WebVectors of WebMediaConstraint, which has WebString members. Not a pointer in sight; it seems to be copying all the way down. My experience with the Web* types is that there's a lot of that.
https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) On 2015/10/09 16:26:28, hta - Chromium wrote: > On 2015/10/09 14:49:21, tommi wrote: > > range based for loop? (looks like HashMap supports that) > > Guido suggested that a couple of cycles ago, but I found it a bit confusing to > figure out how to write one, so punted. > > Is it obvious enough to write in a comment how it should look? Something like this should work: for (const auto& entry : mandatoryConstraintsHashMap) mandatoryConstraintsVector.append(WebMediaConstraint(entry.key, entry.value))
https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) On 2015/10/09 16:26:28, hta - Chromium wrote: > On 2015/10/09 14:49:21, tommi wrote: > > range based for loop? (looks like HashMap supports that) > > Guido suggested that a couple of cycles ago, but I found it a bit confusing to > figure out how to write one, so punted. > > Is it obvious enough to write in a comment how it should look? Assuming it works the same way as std maps, it'd look something like this: for (auto& iter : mandatoryConstraintsHashMap) <same as what you have> (possibly |const auto&|) https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185: constraints.initialize(optional, mandatory); On 2015/10/09 16:26:28, hta - Chromium wrote: > On 2015/10/09 14:49:21, tommi wrote: > > is the ownership of the vectors given to the constraints, or are they copied? > > In general I worry that the code is parsing and copying strings a lot and > that's > > not a good convention to start. > > It's not a start, it's been that way for a long time. > > I want to get the design better for the new constraints, but worry more about > not introducing changes in behavior for the old constraints. This is still the > old stuff. > > WebMediaConstraints has WebVectors of WebMediaConstraint, which has WebString > members. Not a pointer in sight; it seems to be copying all the way down. > > My experience with the Web* types is that there's a lot of that. Sorry, my 'to start' remark didn't come out the way it was intended. I know it's not being introduced in this cl. OK to leave this as is.
Adopted the range based for loop. It's beautiful, not just shiny :-) https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:57: for (; iter != mandatoryConstraintsHashMap.end(); ++iter) On 2015/10/12 09:02:28, tommi wrote: > On 2015/10/09 16:26:28, hta - Chromium wrote: > > On 2015/10/09 14:49:21, tommi wrote: > > > range based for loop? (looks like HashMap supports that) > > > > Guido suggested that a couple of cycles ago, but I found it a bit confusing to > > figure out how to write one, so punted. > > > > Is it obvious enough to write in a comment how it should look? > > Assuming it works the same way as std maps, it'd look something like this: > > for (auto& iter : mandatoryConstraintsHashMap) > <same as what you have> > > (possibly |const auto&|) This looked so beautiful, I'm adopting it :-) https://codereview.chromium.org/1318393002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:185: constraints.initialize(optional, mandatory); On 2015/10/12 09:02:28, tommi wrote: > On 2015/10/09 16:26:28, hta - Chromium wrote: > > On 2015/10/09 14:49:21, tommi wrote: > > > is the ownership of the vectors given to the constraints, or are they > copied? > > > In general I worry that the code is parsing and copying strings a lot and > > that's > > > not a good convention to start. > > > > It's not a start, it's been that way for a long time. > > > > I want to get the design better for the new constraints, but worry more about > > not introducing changes in behavior for the old constraints. This is still the > > old stuff. > > > > WebMediaConstraints has WebVectors of WebMediaConstraint, which has WebString > > members. Not a pointer in sight; it seems to be copying all the way down. > > > > My experience with the Web* types is that there's a lot of that. > > Sorry, my 'to start' remark didn't come out the way it was intended. I know it's > not being introduced in this cl. > OK to leave this as is. Acknowledged.
lgtm
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1318393002/#ps140001 (title: "Another review comment set")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1318393002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318393002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0e6149ca421d75a825064359ac01b8a71f5c3c13 Cr-Commit-Position: refs/heads/master@{#353519} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
