Refactor "track options" to be a dictionary.

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #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"
 #include "bindings/core/v8/ExceptionState.h"
 #include "core/dom/ExceptionCode.h"
 #include "wtf/HashMap.h"
 #include "wtf/Vector.h"
 #include "wtf/text/StringHash.h"
 
 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())
         return true;
 
     Vector<String> names;
     constraintsDictionary.getPropertyNames(names);
 
     String mandatoryName("mandatory");
     String optionalName("optional");
 
     for (Vector<String>::iterator it = names.begin(); it != names.end(); ++it) {
         if (*it != mandatoryName && *it != optionalName)
             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;
     if (names.contains(optionalName)) {
         ArrayValue optionalConstraints;
         bool ok = DictionaryHelper::get(constraintsDictionary, optionalName, optionalConstraints);
         if (!ok || optionalConstraints.isUndefinedOrNull())
             return false;
 
         size_t numberOfConstraints;
         ok = optionalConstraints.length(numberOfConstraints);
         if (!ok)
             return false;
 
         for (size_t i = 0; i < numberOfConstraints; ++i) {
             Dictionary constraint;
             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;
+}
+
 
 WebMediaConstraints create(const Dictionary& constraintsDictionary, ExceptionState& exceptionState)
 {
     WebVector<WebMediaConstraint> optional;
     WebVector<WebMediaConstraint> mandatory;
     if (!parse(constraintsDictionary, optional, mandatory)) {
         exceptionState.throwTypeError("Malformed constraints object.");
         return WebMediaConstraints();
     }
 
     WebMediaConstraints constraints;
     constraints.initialize(optional, mandatory);
     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;
     constraints.initialize();
     return constraints;
 }
 
 } // namespace MediaConstraintsImpl
 } // namespace blink