[Typed OM] Add CSSValue -> CSSNumberValue conversion

third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/cssom/InlineStylePropertyMap.h"
 
 #include "bindings/core/v8/Iterable.h"
 #include "core/CSSPropertyNames.h"
 #include "core/css/CSSPrimitiveValue.h"
 #include "core/css/CSSPropertyMetadata.h"
 #include "core/css/CSSValueList.h"
 #include "core/css/StylePropertySet.h"
 #include "core/css/cssom/CSSOMTypes.h"
 #include "core/css/cssom/CSSSimpleLength.h"
 #include "core/css/cssom/StyleValueFactory.h"
 
 namespace blink {
 
 namespace {
 
 CSSValue* styleValueToCSSValue(CSSPropertyID propertyID, const CSSStyleValue& styleValue)
 {
     if (!CSSOMTypes::propertyCanTake(propertyID, styleValue))
         return nullptr;
     return styleValue.toCSSValueWithProperty(propertyID);
 }
 
+CSSValue* singleStyleValueAsCSSValue(CSSPropertyID propertyID, const CSSStyleValue& styleValue)
+{
+    if (!CSSPropertyMetadata::propertySupportsMultiple(propertyID))
+        return styleValueToCSSValue(propertyID, styleValue);
+
+    CSSValue* cssValue = styleValueToCSSValue(propertyID, styleValue);
+    if (!cssValue)
+        return nullptr;
+
+    // TODO(meade): This won't always work. Figure out what kind of CSSValueList to create properly.
+    CSSValueList* valueList = CSSValueList::createSpaceSeparated();
+    valueList->append(*cssValue);
+    return valueList;
+}
+
+CSSValueList* asCSSValueList(CSSPropertyID propertyID, const CSSStyleValueVector& styleValueVector)
+{
+    // TODO(meade): This won't always work. Figure out what kind of CSSValueList to create properly.
+    CSSValueList* valueList = CSSValueList::createSpaceSeparated();
+    for (const CSSStyleValue* value : styleValueVector) {
+        CSSValue* cssValue = styleValueToCSSValue(propertyID, *value);
+        if (!cssValue) {
+            return nullptr;
+        }
+        valueList->append(*cssValue);
+    }
+    return valueList;
+}
+
 } // namespace
 
 CSSStyleValueVector InlineStylePropertyMap::getAllInternal(CSSPropertyID propertyID)
 {
     CSSValue* cssValue = m_ownerElement->ensureMutableInlineStyle().getPropertyCSSValue(propertyID);
     if (!cssValue)
         return CSSStyleValueVector();
 
     return StyleValueFactory::cssValueToStyleValueVector(propertyID, *cssValue);
 }
@@ skipping 13 matching lines @@
     StylePropertySet& inlineStyleSet = m_ownerElement->ensureMutableInlineStyle();
     for (unsigned i = 0; i < inlineStyleSet.propertyCount(); i++) {
         CSSPropertyID propertyID = inlineStyleSet.propertyAt(i).id();
         result.append(getPropertyNameString(propertyID));
     }
     return result;
 }
 
 void InlineStylePropertyMap::set(CSSPropertyID propertyID, CSSStyleValueOrCSSStyleValueSequenceOrString& item, ExceptionState& exceptionState)
 {
+    CSSValue* cssValue = nullptr;
     if (item.isCSSStyleValue()) {
-        CSSValue* cssValue = styleValueToCSSValue(propertyID, *item.getAsCSSStyleValue());
-        if (!cssValue) {
-            exceptionState.throwTypeError("Invalid type for property");
-            return;
-        }
-        m_ownerElement->setInlineStyleProperty(propertyID, cssValue);
+        cssValue = singleStyleValueAsCSSValue(propertyID, *item.getAsCSSStyleValue());
     } else if (item.isCSSStyleValueSequence()) {
         if (!CSSPropertyMetadata::propertySupportsMultiple(propertyID)) {
             exceptionState.throwTypeError("Property does not support multiple values");
             return;
         }
-
-        // TODO(meade): This won't always work. Figure out what kind of CSSValueList to create properly.
-        CSSValueList* valueList = CSSValueList::createSpaceSeparated();
-        CSSStyleValueVector styleValueVector = item.getAsCSSStyleValueSequence();
-        for (const Member<CSSStyleValue> value : styleValueVector) {
-            CSSValue* cssValue = styleValueToCSSValue(propertyID, *value);
-            if (!cssValue) {
-                exceptionState.throwTypeError("Invalid type for property");
-                return;
-            }
-            valueList->append(*cssValue);
-        }
-
-        m_ownerElement->setInlineStyleProperty(propertyID, valueList);
+        cssValue = asCSSValueList(propertyID, item.getAsCSSStyleValueSequence());
     } else {
         // Parse it.
         DCHECK(item.isString());
         // TODO(meade): Implement this.
         exceptionState.throwTypeError("Not implemented yet");
+        return;
     }
+    if (!cssValue) {
+        exceptionState.throwTypeError("Invalid type for property");
+        return;
+    }
+    m_ownerElement->setInlineStyleProperty(propertyID, cssValue);
 }
 
 void InlineStylePropertyMap::append(CSSPropertyID propertyID, CSSStyleValueOrCSSStyleValueSequenceOrString& item, ExceptionState& exceptionState)
 {
     if (!CSSPropertyMetadata::propertySupportsMultiple(propertyID)) {
         exceptionState.throwTypeError("Property does not support multiple values");
         return;
     }
 
     const CSSValue* cssValue = m_ownerElement->ensureMutableInlineStyle().getPropertyCSSValue(propertyID);
     CSSValueList* cssValueList = nullptr;
-    if (cssValue->isValueList()) {
+    if (!cssValue) {
+        // TODO(meade): Figure out the correct separator.

[sashab, 2016/07/11 23:47:22]

There's a lot of these TODOs to figure out the correct behavior... Should you
have more ASSERTs or should we be erroring out in these cases? Is there an
alternative way we could implement this so more info is passed through?

[meade_UTC10, 2016/07/27 07:14:29]

So these are interesting - we don't check the separator when this is used
internally for calculating styles (so there is no visual effect here), but the
generated text is wrong. We definitely shouldn't ASSERT or error out here, it's
just not-quite-right yet.
I have a plan to fix this, but it involves changing CSSProperties.in to store
which separator is used. It's a decent sized change. Though, you might find that
change useful for your stuff eventually too :)

[sashab, 2016/07/27 23:23:57]

Ok, that sounds good, but I would make the TODO more descriptive then, like:
// TODO(meade): Store the correct separator for each property in
CSSProperties.in and use it to choose the list type.

[meade_UTC10, 2016/08/03 03:26:44]

Done.

+        cssValueList = CSSValueList::createSpaceSeparated();
+    } else if (cssValue->isValueList()) {
         cssValueList = toCSSValueList(cssValue)->copy();
     } else {
         // TODO(meade): Figure out what the correct behaviour here is.
         exceptionState.throwTypeError("Property is not already list valued");
         return;
     }
 
     if (item.isCSSStyleValue()) {
         CSSValue* cssValue = styleValueToCSSValue(propertyID, *item.getAsCSSStyleValue());
         if (!cssValue) {
@@ skipping 39 matching lines @@
             value.setCSSStyleValue(styleValueVector[0]);
         else
             value.setCSSStyleValueSequence(styleValueVector);
         result.append(std::make_pair(getPropertyNameString(propertyID), value));
     }
     return result;
 }
 
 } // namespace blink