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)

[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");
 
     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)
+{
+    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;
+}
+
 
 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);

[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;
     constraints.initialize();
     return constraints;
 }
 
 } // namespace MediaConstraintsImpl
 } // namespace blink