Allow interpolation of background-size values with keywords in CSS Animations

third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp

Index: third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
diff --git a/third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp b/third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..846d4edfcb924f28ba690cf7b9ede2da854ba0bc
--- /dev/null
+++ b/third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.cpp
@@ -0,0 +1,311 @@
+// 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/animation/CSSSizeListInterpolationType.h"
+
+#include "core/animation/CSSLengthInterpolationType.h"
+#include "core/animation/ListInterpolationFunctions.h"
+#include "core/animation/SizeListPropertyFunctions.h"
+#include "core/css/CSSValueList.h"
+#include "core/css/CSSValuePair.h"
+#include "core/css/resolver/StyleResolverState.h"

[suzyh_UTC10 (ex-contributor), 2016/08/26 04:14:34]

Consider also explicitly including the headers were other classes like FillSize,
Length and LengthSize are declared, to make the dependencies explicit rather
than relying on transitive includes.

[alancutter (OOO until 2018), 2016/08/26 13:31:48]

I think the policy is only to not include things that you don't use. I'm less
inclined to strictly include everything that is used as it becomes easy to
accidentally break the first rule as the code changes.

+
+namespace blink {
+
+class CSSSizeNonInterpolableValue : public NonInterpolableValue {
+public:
+    static PassRefPtr<CSSSizeNonInterpolableValue> create(CSSValueID keyword)
+    {
+        return adoptRef(new CSSSizeNonInterpolableValue(keyword));
+    }
+
+    static PassRefPtr<CSSSizeNonInterpolableValue> create(PassRefPtr<NonInterpolableValue> lengthNonInterpolableValue)
+    {
+        return adoptRef(new CSSSizeNonInterpolableValue(lengthNonInterpolableValue));
+    }
+
+    bool isKeyword() const { return m_keyword != CSSValueInvalid; }
+    CSSValueID keyword() const { DCHECK(isKeyword()); return m_keyword; }
+
+    const NonInterpolableValue* lengthNonInterpolableValue() const { DCHECK(!isKeyword()); return m_lengthNonInterpolableValue.get(); }
+    RefPtr<NonInterpolableValue>& lengthNonInterpolableValue() { DCHECK(!isKeyword()); return m_lengthNonInterpolableValue; }
+
+    DECLARE_NON_INTERPOLABLE_VALUE_TYPE();
+
+private:
+    CSSSizeNonInterpolableValue(CSSValueID keyword)

[suzyh_UTC10 (ex-contributor), 2016/08/26 04:14:34]

Is there a reason to have two constructors mirroring the two ::create functions,
or can we have the two ::create functions call one constructor with different
arguments?

[alancutter (OOO until 2018), 2016/08/26 13:31:48]

I think it's better for assertions to be in constructors than create functions.
IMO this is neater WRT the DCHECK but I'm not fussed if you want it changed.

[suzyh_UTC10 (ex-contributor), 2016/08/30 08:01:07]

I think this is a style thing, and I am undecided. My context is a discussion
about what happens in error conditions for constructors, a topic which I am not
yet comfortably familiar with:
https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
I haven't found anything in the Chromium/Blink style guides that express a
position on this. If you can find a precedent in docs or code that would set out
a preferred approach for this, let's go with that. Otherwise, I think it's
clearer and conceptually simpler to have a single constructor, and in that case
would probably put the DCHECK in the create function.

[alancutter (OOO until 2018), 2016/09/05 07:44:27]

There is precedence for checking in the constructor in Blink. See: CSSValuePair,
CSSVariableData, MediaQueryMatcher
I don't think the code here is breaking the linked style guide since the check
is not a virtual method call nor is the failure without a signal (the process
crashes).
Personally it makes more sense to check in the constructor, if you put it in the
create() then there's a possibility the object may be constructed internally
with incorrect parameters. Eliminating these possibilities is a key benefit of
using assertions.

+        : m_keyword(keyword)
+        , m_lengthNonInterpolableValue(nullptr)
+    {
+        DCHECK_NE(keyword, CSSValueInvalid);
+    }
+
+    CSSSizeNonInterpolableValue(PassRefPtr<NonInterpolableValue> lengthNonInterpolableValue)
+        : m_keyword(CSSValueInvalid)
+        , m_lengthNonInterpolableValue(lengthNonInterpolableValue)
+    { }
+
+    CSSValueID m_keyword;
+    RefPtr<NonInterpolableValue> m_lengthNonInterpolableValue;
+};
+
+DEFINE_NON_INTERPOLABLE_VALUE_TYPE(CSSSizeNonInterpolableValue);
+DEFINE_NON_INTERPOLABLE_VALUE_TYPE_CASTS(CSSSizeNonInterpolableValue);
+
+static bool sizesAreCompatible(const NonInterpolableValue* a, const NonInterpolableValue* b)

[suzyh_UTC10 (ex-contributor), 2016/08/26 04:14:34]

Does this function belong in a separate helper library?

[alancutter (OOO until 2018), 2016/08/26 13:31:48]

Moved into SizeInterpolationFunctions.

+{
+    const auto& sizeA = toCSSSizeNonInterpolableValue(*a);
+    const auto& sizeB = toCSSSizeNonInterpolableValue(*b);
+    if (sizeA.isKeyword() != sizeB.isKeyword())
+        return false;
+    if (sizeA.isKeyword())
+        return sizeA.keyword() == sizeB.keyword();
+    return true;
+}
+
+class UnderlyingSizeListChecker : public InterpolationType::ConversionChecker {
+public:
+    ~UnderlyingSizeListChecker() final {}
+
+    static std::unique_ptr<UnderlyingSizeListChecker> create(const NonInterpolableList& underlyingList)
+    {
+        return wrapUnique(new UnderlyingSizeListChecker(underlyingList));
+    }
+
+private:
+    UnderlyingSizeListChecker(const NonInterpolableList& underlyingList)
+        : m_underlyingList(&underlyingList)
+    { }
+
+    bool isValid(const InterpolationEnvironment&, const InterpolationValue& underlying) const final
+    {
+        const auto& underlyingList = toNonInterpolableList(*underlying.nonInterpolableValue);
+        size_t underlyingLength = underlyingList.length();
+        if (underlyingLength != m_underlyingList->length())
+            return false;
+        for (size_t i = 0; i < underlyingLength; i++) {
+            if (!sizesAreCompatible(underlyingList.get(i), m_underlyingList->get(i)))
+                return false;
+        }
+        return true;
+    }
+
+    RefPtr<const NonInterpolableList> m_underlyingList;
+};
+
+class InheritedSizeListChecker : public InterpolationType::ConversionChecker {
+public:
+    ~InheritedSizeListChecker() final {}
+
+    static std::unique_ptr<InheritedSizeListChecker> create(CSSPropertyID property, const SizeList& inheritedSizeList)
+    {
+        return wrapUnique(new InheritedSizeListChecker(property, inheritedSizeList));
+    }
+
+private:
+    InheritedSizeListChecker(CSSPropertyID property, const SizeList& inheritedSizeList)
+        : m_property(property)
+        , m_inheritedSizeList(inheritedSizeList)
+    { }
+
+    bool isValid(const InterpolationEnvironment& environment, const InterpolationValue&) const final
+    {
+        return m_inheritedSizeList == SizeListPropertyFunctions::getSizeList(m_property, *environment.state().parentStyle());
+    }
+
+    CSSPropertyID m_property;
+    SizeList m_inheritedSizeList;
+};
+
+InterpolationValue convertKeyword(CSSValueID keyword)
+{
+    return InterpolationValue(InterpolableList::create(0), CSSSizeNonInterpolableValue::create(keyword));
+}
+
+InterpolationValue wrapConvertedLength(InterpolationValue&& convertedLength)
+{
+    if (!convertedLength)
+        return nullptr;
+    return InterpolationValue(
+        std::move(convertedLength.interpolableValue),
+        CSSSizeNonInterpolableValue::create(convertedLength.nonInterpolableValue.release()));
+}
+
+InterpolationValue convertSizeList(const SizeList& sizeList, float zoom)
+{
+    // Flatten pairs of width/height into individual items, even for contain and cover.
+    return ListInterpolationFunctions::createList(sizeList.size() * 2, [&sizeList, zoom](size_t index) -> InterpolationValue {
+        const FillSize& size = sizeList[index / 2];
+        switch (size.type) {
+        case SizeLength: {
+            const Length& side = index % 2 == 0 ? size.size.width() : size.size.height();
+            if (side.isAuto())
+                return convertKeyword(CSSValueAuto);
+            return wrapConvertedLength(CSSLengthInterpolationType::maybeConvertLength(side, zoom));
+        }
+        case Contain:
+            return convertKeyword(CSSValueContain);
+        case Cover:
+            return convertKeyword(CSSValueCover);
+        case SizeNone:
+        default:
+            NOTREACHED();
+            return nullptr;
+        }
+    });
+}
+
+InterpolationValue CSSSizeListInterpolationType::maybeConvertNeutral(const InterpolationValue& underlying, ConversionCheckers& conversionCheckers) const
+{
+    const auto& underlyingList = toNonInterpolableList(*underlying.nonInterpolableValue);
+    conversionCheckers.append(UnderlyingSizeListChecker::create(underlyingList));
+    return ListInterpolationFunctions::createList(underlyingList.length(), [&underlyingList](size_t index) {
+        // const_cast to take a ref.
+        auto& item = toCSSSizeNonInterpolableValue(const_cast<NonInterpolableValue&>(*underlyingList.get(index)));
+        if (item.isKeyword())
+            return InterpolationValue(InterpolableList::create(0), &item);
+        return InterpolationValue(CSSLengthInterpolationType::createNeutralInterpolableValue(), &item);
+    });
+}
+
+InterpolationValue CSSSizeListInterpolationType::maybeConvertInitial(const StyleResolverState&, ConversionCheckers&) const
+{
+    return convertSizeList(SizeListPropertyFunctions::getInitialSizeList(cssProperty()), 1);
+}
+
+InterpolationValue CSSSizeListInterpolationType::maybeConvertInherit(const StyleResolverState& state, ConversionCheckers& conversionCheckers) const
+{
+    SizeList inheritedSizeList = SizeListPropertyFunctions::getSizeList(cssProperty(), *state.parentStyle());
+    conversionCheckers.append(InheritedSizeListChecker::create(cssProperty(), inheritedSizeList));
+    return convertSizeList(inheritedSizeList, state.style()->effectiveZoom());
+}
+
+InterpolationValue CSSSizeListInterpolationType::maybeConvertValue(const CSSValue& value, const StyleResolverState& state, ConversionCheckers&) const
+{
+    // CSSPropertyParser doesn't put single values in lists so wrap it up in a temporary list.
+    const CSSValueList* list = nullptr;
+    if (!value.isBaseValueList()) {
+        CSSValueList* tempList = CSSValueList::createCommaSeparated();
+        tempList->append(value);
+        list = tempList;
+    } else {
+        list = toCSSValueList(&value);
+    }
+
+    // Flatten pairs of width/height into individual items, even for contain and cover.
+    return ListInterpolationFunctions::createList(list->length() * 2, [list](size_t index) -> InterpolationValue {

[suzyh_UTC10 (ex-contributor), 2016/08/26 04:14:34]

I think I would prefer to see a separate library of helper functions than an
anonymous function defined inline like this.

The code would also be simpler and easier to read with a few parts factored out
into separate functions.

Likewise in convertSizeList above.

[alancutter (OOO until 2018), 2016/08/26 13:31:48]

This is a lambda due to the closure of list (and the method signature). I am
certain that avoiding the lambda would result in more convoluted code to deal
with the memory management involved. Most likely it will turn into what the
lambda is syntactic sugar for.

The inner parts of the lambda now live in SizeInterpolationFunctions.

+        const CSSValue& item = list->item(index / 2);
+        if (item.isValuePair()) {
+            const CSSValuePair& pair = toCSSValuePair(item);
+            const CSSValue& side = index % 2 == 0 ? pair.first() : pair.second();
+            if (side.isPrimitiveValue() && toCSSPrimitiveValue(side).getValueID() == CSSValueAuto)
+                return convertKeyword(CSSValueAuto);
+            return wrapConvertedLength(CSSLengthInterpolationType::maybeConvertCSSValue(side));
+        }
+        if (!item.isPrimitiveValue())
+            return nullptr;
+        CSSValueID keyword = toCSSPrimitiveValue(item).getValueID();
+        if (keyword)
+            return convertKeyword(keyword);
+        // A single length is equivalent to "<length> auto".
+        if (index % 2 == 0)
+            return wrapConvertedLength(CSSLengthInterpolationType::maybeConvertCSSValue(item));
+        return convertKeyword(CSSValueAuto);
+    });
+}
+
+static PairwiseInterpolationValue maybeMergeSingleSizes(InterpolationValue&& start, InterpolationValue&& end)
+{
+    if (!sizesAreCompatible(start.nonInterpolableValue.get(), end.nonInterpolableValue.get()))
+        return nullptr;
+    return PairwiseInterpolationValue(
+        std::move(start.interpolableValue),
+        std::move(end.interpolableValue),
+        start.nonInterpolableValue.release());
+}
+
+PairwiseInterpolationValue CSSSizeListInterpolationType::maybeMergeSingles(InterpolationValue&& start, InterpolationValue&& end) const
+{
+    return ListInterpolationFunctions::maybeMergeSingles(std::move(start), std::move(end), maybeMergeSingleSizes);
+}
+
+InterpolationValue CSSSizeListInterpolationType::maybeConvertUnderlyingValue(const InterpolationEnvironment& environment) const
+{
+    const ComputedStyle& style = *environment.state().style();
+    return convertSizeList(SizeListPropertyFunctions::getSizeList(cssProperty(), style), style.effectiveZoom());
+}
+
+void compositeSizes(std::unique_ptr<InterpolableValue>& underlyingInterpolableValue, RefPtr<NonInterpolableValue>& underlyingNonInterpolableValue, double underlyingFraction, const InterpolableValue& interpolableValue, const NonInterpolableValue* nonInterpolableValue)
+{
+    const auto& sizeNonInterpolableValue = toCSSSizeNonInterpolableValue(*nonInterpolableValue);
+    if (sizeNonInterpolableValue.isKeyword())
+        return;
+    auto& underlyingSizeNonInterpolableValue = toCSSSizeNonInterpolableValue(*underlyingNonInterpolableValue);
+    CSSLengthInterpolationType::composite(
+        underlyingInterpolableValue,
+        underlyingSizeNonInterpolableValue.lengthNonInterpolableValue(),
+        underlyingFraction,
+        interpolableValue,
+        sizeNonInterpolableValue.lengthNonInterpolableValue());
+}
+
+void CSSSizeListInterpolationType::composite(UnderlyingValueOwner& underlyingValueOwner, double underlyingFraction, const InterpolationValue& value, double interpolationFraction) const
+{
+    ListInterpolationFunctions::composite(underlyingValueOwner, underlyingFraction, *this, value, sizesAreCompatible, compositeSizes);
+}
+
+Length createLength(const InterpolableValue& interpolableValue, const CSSSizeNonInterpolableValue& nonInterpolableValue, const CSSToLengthConversionData& conversionData)
+{
+    if (nonInterpolableValue.isKeyword()) {
+        DCHECK_EQ(nonInterpolableValue.keyword(), CSSValueAuto);
+        return Length(Auto);
+    }
+    return CSSLengthInterpolationType::resolveInterpolableLength(interpolableValue, nonInterpolableValue.lengthNonInterpolableValue(), conversionData, ValueRangeNonNegative);

[suzyh_UTC10 (ex-contributor), 2016/08/26 04:14:34]

As a reader of this code it is surprising to have a reference to
CSSLengthInterpolationType here. I think that indicates an opportunity to move
some code to a helper function/library used here and by
CSSLengthInterpolationType.

Ditto above.

[alancutter (OOO until 2018), 2016/08/26 13:31:48]

Agreed, CSSLengthInterpolationType should have its static helper functions split
off into a LengthInterpolationFunctions class similar to
ShadowInterpolationFunctions and others.
Created dependant patch.

+}
+
+FillSize createFillSize(const InterpolableValue& interpolableValueA, const NonInterpolableValue* nonInterpolableValueA, const InterpolableValue& interpolableValueB, const NonInterpolableValue* nonInterpolableValueB, const CSSToLengthConversionData& conversionData)
+{
+    const auto& sideA = toCSSSizeNonInterpolableValue(*nonInterpolableValueA);
+    const auto& sideB = toCSSSizeNonInterpolableValue(*nonInterpolableValueB);
+    if (sideA.isKeyword()) {
+        switch (sideA.keyword()) {
+        case CSSValueCover:
+            DCHECK_EQ(sideA.keyword(), sideB.keyword());
+            return FillSize(Cover, LengthSize());
+        case CSSValueContain:
+            DCHECK_EQ(sideA.keyword(), sideB.keyword());
+            return FillSize(Contain, LengthSize());
+        case CSSValueAuto:
+            break;
+        default:
+            NOTREACHED();
+            break;
+        }
+    }
+    return FillSize(SizeLength, LengthSize(
+        createLength(interpolableValueA, sideA, conversionData),
+        createLength(interpolableValueB, sideB, conversionData)));
+}
+
+void CSSSizeListInterpolationType::apply(const InterpolableValue& interpolableValue, const NonInterpolableValue* nonInterpolableValue, InterpolationEnvironment& environment) const
+{
+    const auto& interpolableList = toInterpolableList(interpolableValue);
+    const auto& nonInterpolableList = toNonInterpolableList(*nonInterpolableValue);
+    size_t length = interpolableList.length();
+    DCHECK_EQ(length, nonInterpolableList.length());
+    DCHECK_EQ(length % 2, 0ul);
+    size_t sizeListLength = length / 2;
+    SizeList sizeList(sizeListLength);
+    for (size_t i = 0; i < sizeListLength; i++) {
+        sizeList[i] = createFillSize(
+            *interpolableList.get(i * 2),
+            nonInterpolableList.get(i * 2),
+            *interpolableList.get(i * 2 + 1),
+            nonInterpolableList.get(i * 2 + 1),
+            environment.state().cssToLengthConversionData());
+    }
+    SizeListPropertyFunctions::setSizeList(cssProperty(), *environment.state().style(), sizeList);
+}
+
+} // namespace blink