Refactor "track options" to be a dictionary.

third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp

Index: third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
diff --git a/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp b/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
index 66d3a5dbfc49e32ed0e39ee4766b01adefe4abfb..3421aa52a6d39f3af20bae82f5a3fca6bdf10524 100644
--- a/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
+++ b/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
@@ -31,6 +31,7 @@
 #include "config.h"
 
 #include "modules/mediastream/MediaConstraintsImpl.h"
+#include "modules/mediastream/MediaTrackConstraintSet.h"
 
 #include "bindings/core/v8/ArrayValue.h"
 #include "bindings/core/v8/Dictionary.h"
@@ -44,13 +45,47 @@ namespace blink {
 
 namespace MediaConstraintsImpl {
 
+static bool parseMandatoryConstraintsDictionary(const Dictionary& mandatoryConstraintsDictionary, WebVector<WebMediaConstraint>& mandatory)
+{
+    Vector<WebMediaConstraint> mandatoryConstraintsVector;
+    HashMap<String, String> mandatoryConstraintsHashMap;
+    bool ok = mandatoryConstraintsDictionary.getOwnPropertiesAsStringHashMap(mandatoryConstraintsHashMap);
+    if (!ok)
+        return false;
+
+    HashMap<String, String>::const_iterator iter = mandatoryConstraintsHashMap.begin();
+    for (; iter != mandatoryConstraintsHashMap.end(); ++iter)

[tommi (sloooow) - chröme, 2015/10/09 14:49:21]

range based for loop?  (looks like HashMap supports that)

[hta - Chromium, 2015/10/09 16:26:28]

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?

[Guido Urdaneta, 2015/10/12 08:54:56]

Something like this should work:

for (const auto& entry : mandatoryConstraintsHashMap)
  mandatoryConstraintsVector.append(WebMediaConstraint(entry.key, entry.value))

[tommi (sloooow) - chröme, 2015/10/12 09:02:28]

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&|)

[hta - Chromium, 2015/10/12 12:24:40]

This looked so beautiful, I'm adopting it :-)

+        mandatoryConstraintsVector.append(WebMediaConstraint(iter->key, iter->value));
+    mandatory.assign(mandatoryConstraintsVector);
+    return true;
+}
+
+static bool parseOptionalConstraintsVectorElement(const Dictionary& constraint, Vector<WebMediaConstraint>& optionalConstraintsVector)
+{
+    Vector<String> localNames;
+    bool ok = constraint.getPropertyNames(localNames);
+    if (!ok)
+        return false;
+    if (localNames.size() != 1)
+        return false;
+    String key = localNames[0];

[tommi (sloooow) - chröme, 2015/10/09 14:49:21]

is this copy necessary?  (const String&?)

[hta - Chromium, 2015/10/09 16:26:28]

Done.

+    String value;
+    ok = DictionaryHelper::get(constraint, key, value);
+    if (!ok)
+        return false;
+    optionalConstraintsVector.append(WebMediaConstraint(key, value));
+    return true;
+}
+
 static bool parse(const Dictionary& constraintsDictionary, WebVector<WebMediaConstraint>& optional, WebVector<WebMediaConstraint>& mandatory)
 {
     if (constraintsDictionary.isUndefinedOrNull())
         return true;
 
     Vector<String> names;
-    constraintsDictionary.getPropertyNames(names);
+    bool ok = constraintsDictionary.getPropertyNames(names);
+    if (!ok)
+        return false;
 
     String mandatoryName("mandatory");
     String optionalName("optional");
@@ -60,21 +95,14 @@ static bool parse(const Dictionary& constraintsDictionary, WebVector<WebMediaCon
             return false;
     }
 
-    Vector<WebMediaConstraint> mandatoryConstraintsVector;
     if (names.contains(mandatoryName)) {
         Dictionary mandatoryConstraintsDictionary;
         bool ok = constraintsDictionary.get(mandatoryName, mandatoryConstraintsDictionary);
         if (!ok || mandatoryConstraintsDictionary.isUndefinedOrNull())
             return false;
-
-        HashMap<String, String> mandatoryConstraintsHashMap;
-        ok = mandatoryConstraintsDictionary.getOwnPropertiesAsStringHashMap(mandatoryConstraintsHashMap);
+        ok = parseMandatoryConstraintsDictionary(mandatoryConstraintsDictionary, mandatory);
         if (!ok)
             return false;
-
-        HashMap<String, String>::const_iterator iter = mandatoryConstraintsHashMap.begin();
-        for (; iter != mandatoryConstraintsHashMap.end(); ++iter)
-            mandatoryConstraintsVector.append(WebMediaConstraint(iter->key, iter->value));
     }
 
     Vector<WebMediaConstraint> optionalConstraintsVector;
@@ -94,21 +122,39 @@ static bool parse(const Dictionary& constraintsDictionary, WebVector<WebMediaCon
             ok = optionalConstraints.get(i, constraint);
             if (!ok || constraint.isUndefinedOrNull())
                 return false;
-            Vector<String> localNames;
-            constraint.getPropertyNames(localNames);
-            if (localNames.size() != 1)
-                return false;
-            String key = localNames[0];
-            String value;
-            ok = DictionaryHelper::get(constraint, key, value);
+            ok = parseOptionalConstraintsVectorElement(constraint, optionalConstraintsVector);
             if (!ok)
                 return false;
-            optionalConstraintsVector.append(WebMediaConstraint(key, value));
         }
+        optional.assign(optionalConstraintsVector);
     }
 
-    optional.assign(optionalConstraintsVector);
-    mandatory.assign(mandatoryConstraintsVector);
+    return true;
+}
+
+static bool parse(const MediaTrackConstraintSet& constraintsIn, WebVector<WebMediaConstraint>& optional, WebVector<WebMediaConstraint>& mandatory)
+{
+    Vector<WebMediaConstraint> mandatoryConstraintsVector;
+    if (constraintsIn.hasMandatory()) {
+        bool ok = parseMandatoryConstraintsDictionary(constraintsIn.mandatory(), mandatory);
+        if (!ok)
+            return false;
+    }
+
+    Vector<WebMediaConstraint> optionalConstraintsVector;
+    if (constraintsIn.hasOptional()) {
+        const Vector<Dictionary>& optionalConstraints = constraintsIn.optional();
+
+        for (size_t i = 0; i < optionalConstraints.size(); ++i) {
+            Dictionary constraint = optionalConstraints[i];

[tommi (sloooow) - chröme, 2015/10/09 14:49:21]

can we avoid creating a copy or is this not really a copy?

[hta - Chromium, 2015/10/09 16:26:28]

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.

+            if (constraint.isUndefinedOrNull())
+                return false;
+            bool ok = parseOptionalConstraintsVectorElement(constraint, optionalConstraintsVector);
+            if (!ok)
+                return false;
+        }
+        optional.assign(optionalConstraintsVector);
+    }
     return true;
 }
 
@@ -127,6 +173,19 @@ WebMediaConstraints create(const Dictionary& constraintsDictionary, ExceptionSta
     return constraints;
 }
 
+WebMediaConstraints create(const MediaTrackConstraintSet& constraintsIn, ExceptionState& exceptionState)
+{
+    WebVector<WebMediaConstraint> optional;
+    WebVector<WebMediaConstraint> mandatory;
+    if (!parse(constraintsIn, optional, mandatory)) {
+        exceptionState.throwTypeError("Malformed constraints object.");
+        return WebMediaConstraints();
+    }
+    WebMediaConstraints constraints;
+    constraints.initialize(optional, mandatory);

[tommi (sloooow) - chröme, 2015/10/09 14:49:21]

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.

[hta - Chromium, 2015/10/09 16:26:28]

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.

[tommi (sloooow) - chröme, 2015/10/12 09:02:28]

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.

[hta - Chromium, 2015/10/12 12:24:40]

Acknowledged.

+    return constraints;
+}
+
 WebMediaConstraints create()
 {
     WebMediaConstraints constraints;