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..5d2c8536adfe269f24a91d2c8477af15b5254edb 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,6 +45,36 @@ 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)
+        mandatoryConstraintsVector.append(WebMediaConstraint(iter->key, iter->value));

[Peter Beverloo, 2015/10/08 09:50:25]

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.

[hta - Chromium, 2015/10/09 13:02:28]

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().

+    mandatory.assign(mandatoryConstraintsVector);
+    return true;
+}
+
+static bool parseOptionalConstraintsVectorElement(const Dictionary& constraint, Vector<WebMediaConstraint>& optionalConstraintsVector)
+{
+    Vector<String> localNames;
+    constraint.getPropertyNames(localNames);

[Peter Beverloo, 2015/10/08 09:50:25]

Would it make sense to check for the return value here, as you do for the
function above (using |ok|)?

[hta - Chromium, 2015/10/09 13:02:28]

Done.

+    if (localNames.size() != 1)
+        return false;
+    String key = localNames[0];
+    String value;
+    bool 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())
@@ -60,21 +91,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 +118,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)

[Peter Beverloo, 2015/10/08 09:50:25]

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.

[hta - Chromium, 2015/10/09 13:02:28]

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.

+{
+    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();

[Peter Beverloo, 2015/10/08 09:50:25]

const T&, otherwise you're making a copy.

[hta - Chromium, 2015/10/09 13:02:28]

Done.

+
+        for (size_t i = 0; i < optionalConstraints.size(); ++i) {
+            Dictionary constraint = optionalConstraints[i];
+            if (constraint.isUndefinedOrNull())
+                return false;
+            bool ok = parseOptionalConstraintsVectorElement(constraint, optionalConstraintsVector);
+            if (!ok)
+                return false;
+        }
+        optional.assign(optionalConstraintsVector);
+    }
     return true;
 }
 
@@ -127,6 +169,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);
+    return constraints;
+}
+
 WebMediaConstraints create()
 {
     WebMediaConstraints constraints;