Web Animations API: Start implementation of Element.animate().

Source/core/animation/AnimatedElement.cpp

+/*
+ * Copyright (C) 2013 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:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *     * Neither the name of Google Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * 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"
+

[Steve Block, 2013/11/29 07:31:19]

NO blank line here

[rjwright, 2013/12/04 05:05:54]

Done.

+#include "core/animation/AnimatedElement.h"
+
+#include "core/animation/AnimatableLength.h"
+#include "core/animation/AnimationEffect.h"
+#include "core/animation/DocumentTimeline.h"
+#include "core/animation/KeyframeAnimationEffect.h"
+#include "core/animation/css/CSSAnimatableValueFactory.h"
+#include "core/animation/css/CSSAnimations.h"
+#include "core/css/resolver/StyleBuilder.h"
+#include "core/css/resolver/StyleResolverState.h"
+#include "core/page/RuntimeCSSEnabled.h"
+#include "core/rendering/style/RenderStyle.h"
+#include "wtf/text/StringBuilder.h"
+
+
+namespace WebCore {
+
+namespace {
+
+static bool isUpper(UChar ch) { return isASCIIUpper(ch); }

[Steve Block, 2013/11/29 07:31:19]

Why do you need this wrapper function?

[rjwright, 2013/12/04 05:05:54]

We don't.

Done.

+
+static CSSPropertyID v8PropertyToCSSPropertyID(String propertyName)

[Steve Block, 2013/11/29 07:31:19]

I'm surprised we don't have this logic elsewhere for the CSSOM but I imagine you
already checked this?

Why 'V8' property? This doesn't seem to be specific to V8. Maybe
camelCaseCSSPropertyNameToID()?

We should add a unit test for this function.

[apavlov, 2013/11/29 09:19:02]

FWIW, this logic (and a little bit more :)) is found in
V8CSSStyleDeclarationCustom.cpp,
static CSSPropertyInfo* cssPropertyInfo(v8::Handle<v8::String> v8PropertyName).

Factoring out the required chunk (to return CSSPropertyInfo* given a normal
String) is fairly straightforward.

[rjwright, 2013/12/04 05:05:54]

Name change done. The logic exists elsewhere but not in a useful form.

[rjwright, 2013/12/04 05:05:54]

Yes that's where I copied it from. There was no obvious way to expose it to Web
Animations so I copied it instead.

If you have any suggestions for how to expose it, or a more central place to
re-implement it, then I am all ears.

+{
+    StringBuilder builder;
+    size_t position = 0;
+    while (true) {
+        size_t end = propertyName.find(isUpper, position);
+        if (end == kNotFound) {

[Steve Block, 2013/11/29 07:31:19]

Is this simpler ...

size_t end;
while (end = propertyName.find(isUpper, position) != kNotFound) {
  builder.append(propertyName.substring(position, end) + "-" +
toASCIILower(propertyName[end]));
  position = end + 1;
}
builder.append(propertyName.substring(position));

[rjwright, 2013/12/04 05:05:54]

Done.

+            builder.append(propertyName.substring(position));
+            break;
+        }
+        builder.append(propertyName.substring(position, end) + "-" + toASCIILower(propertyName[end]));
+        position = end + 1;
+    }
+    return cssPropertyID(builder.toString());
+}
+
+} // namespace
+
+void AnimatedElement::animate(Element* element, Vector<Dictionary> dictionaryKeyframesVector)

[Steve Block, 2013/11/29 07:31:19]

s/dictionaryKeyframesVector/keyframeDictionaryVector to match the spec
terminology?

[rjwright, 2013/12/04 05:05:54]

Done.

+{
+    ASSERT(RuntimeEnabledFeatures::webAnimationsAPIEnabled());
+
+    KeyframeAnimationEffect::KeyframeVector keyframes;
+    RefPtr<MutableStylePropertySet> propertySet = MutableStylePropertySet::create();
+    for (size_t i = 0; i < dictionaryKeyframesVector.size(); ++i) {
+
+        Vector<String> keyframeProperties;
+        dictionaryKeyframesVector[i].getOwnPropertyNames(keyframeProperties);
+
+        if (!keyframeProperties.size())

[Steve Block, 2013/11/29 07:31:19]

This is incorrect. The spec requires that animation.effect.getFrames() should
return the original list of keyframes, as specified. This will be relevant once
we hook up element.animate() to return a Player/TimedItem. If there's a
particular reason for not handling empty keyframes now, add a FIXME and perhaps
a RELEASE_ASSERT_WITH_MESSAGE().

[rjwright, 2013/12/04 05:05:54]

Done.

+            continue;
+
+        keyframes.append(Keyframe::create());

[Steve Block, 2013/11/29 07:31:19]

Probably best to build up the keyframe before appending it to keyframes, to save
repeated calls to keyframes.last().

[rjwright, 2013/12/04 05:05:54]

Done.

+
+        double offset;
+        if (dictionaryKeyframesVector[i].get("offset", offset))
+            keyframes.last()->setOffset(offset);
+        else
+            keyframes.last()->setOffset(std::numeric_limits<double>::quiet_NaN());

[Steve Block, 2013/11/29 07:31:19]

No need for this, it's the default offset for a keyframe. In any case, you
should use nullValue().

[rjwright, 2013/12/04 05:05:54]

Done. nullValue()?

+
+        String compositeOperationString;
+        dictionaryKeyframesVector[i].get("compositeOperation", compositeOperationString);

[Steve Block, 2013/12/02 04:45:45]

This should be 'composite'. 'CompositeOperation' is the name of the type.

http://dev.w3.org/fxtf/web-animations/#the-keyframe-dictionary

[rjwright, 2013/12/04 05:05:54]

Done.

+        if (compositeOperationString == String("add"))
+            keyframes.last()->setComposite(AnimationEffect::CompositeAdd);
+        else
+            keyframes.last()->setComposite(AnimationEffect::CompositeReplace);

[Steve Block, 2013/11/29 07:31:19]

Again, keyframes use replace composition by default.

[rjwright, 2013/12/04 05:05:54]

Done.

+
+        for (size_t j = 0; j < keyframeProperties.size(); ++j) {
+            String property = keyframeProperties[j];
+            String value;
+            dictionaryKeyframesVector[i].get(property, value);
+            CSSPropertyID id = v8PropertyToCSSPropertyID(property);
+
+            if (!id || !CSSAnimations::isAnimatableProperty(id))

[Steve Block, 2013/11/29 07:31:19]

Isn't there a CSSPropertyInvalid we can use here?

[rjwright, 2013/12/04 05:05:54]

If the property from the dictionary isn't a valid css property then id will be
CSSPropertyInvalid.

Trying to apply a value to CSSPropertyInvalid and use it to make an
AnimatableValue hits an assert somewhere.

+                continue;

[Steve Block, 2013/11/29 07:31:19]

Again, we shouldn't be skipping invalid keyframes as this stage, so that
animation.effect.getFrames() returns the original set of keyframes. This logic
should go in KeyframeAnimationEffect::normalizedKeyframes() (which matches the
algorithm described in the spec).

[Steve Block, 2013/12/02 01:59:43]

I guess we need to decide whether to change the API of KAE to describe
properties by strings (thereby allowing it to handle invalid properties) so that
we can expose KAE directly to JS, or to add a wrapper class around KAE that
keeps track of the 'original' keyframe list and handles the conversion between
string and CSSPropertyID. We should discuss this.

[rjwright, 2013/12/04 05:05:54]

We should discuss this. There's a bunch of different options and I don't like
any of them :P

+            if (!CSSParser::parseValue(propertySet.get(), id, value, false, element->document()))
+                continue;

[Steve Block, 2013/11/29 07:31:19]

Same as above

[rjwright, 2013/12/04 05:05:54]

I can't change this to anything more meaningful until we decide what to do with
invalid inputs.

+
+            RefPtr<RenderStyle> style = RenderStyle::clone(element->styleForRenderer().get());
+            StyleResolverState state(element->document(), element);
+            state.setStyle(style);
+            StyleBuilder::applyProperty(id, state, propertySet->getPropertyCSSValue(id).get());
+            keyframes.last()->setPropertyValue(id, CSSAnimatableValueFactory::create(id, *style.get()).get());
+        }
+        if (!keyframes.last()->properties().size() && isnan(keyframes.last()->offset()))

[Steve Block, 2013/11/29 07:31:19]

Same comment about getFrames(). Also, use isEmpty() rather than !size()

[rjwright, 2013/12/04 05:05:54]

Done.

+            keyframes.removeLast();
+    }
+
+    RefPtr<KeyframeAnimationEffect> effect = KeyframeAnimationEffect::create(keyframes);
+
+    // FIXME: Totally hardcoded Timing for now. Will handle timing parameters later.
+    Timing timing;
+    timing.hasIterationDuration = true;
+    timing.iterationDuration = 1;
+    timing.iterationCount = 1;

[Steve Block, 2013/11/29 07:31:19]

Superfluous. The default iterationCount is 1.

[rjwright, 2013/12/04 05:05:54]

Done.

+
+    RefPtr<Animation> animation = Animation::create(element, effect, timing);
+
+    DocumentTimeline* timeline = element->document().timeline();

[Steve Block, 2013/11/29 07:31:19]

ASSERT(timeline)

[rjwright, 2013/12/04 05:05:54]

Done.

+    // FIXME: If any property appears in only one keyframe (which includes the case where there is
+    // only one keyframe) this crashes.

[Steve Block, 2013/11/29 07:31:19]

This shouldn't be the case. KeyframeAnimationEffect should handle synthetic
keyframes for any distribution of properties in keyframes. If you can send me a
repro case (maybe an addition to KeyframeAnimationEffectTest), I'll take a look.

[Steve Block, 2013/12/02 04:45:45]

I think you're just seeing the fact that add composition isn't yet supported.
See https://codereview.chromium.org/98243002 Let me know if you see anything
else.

http://dev.w3.org/fxtf/web-animations/#the-unaccumulated-animation-value-of-a...

[rjwright, 2013/12/04 05:05:54]

Yep that's it. In that case I'll just pass those cases through.

+    // Handling for this case not yet implemented.
+    // We can check for this here but the check will be pretty clunky. Prefer to add release-assert
+    // where implementation is missing.
+    //
+    // FIXME: If there are more than two keyframes and any of those keyframes doesn't have a specified

[Steve Block, 2013/11/29 07:31:19]

It's not clear which of these you plan to fix before landing vs after landing
this patch. Uploading a work in progress is fine, but it's good to make this
clear. FIXME is used for TODOs you plan to fix after landing, so maybe use
FIXME_BEFORE_LANDING otherwise.

[rjwright, 2013/12/04 05:05:54]

Done.

+    // offset, this crashes. If there are exactly two keyframes then missing offsets are ok.
+    // Handling for this case not yet implemented.

[Steve Block, 2013/11/29 07:31:19]

This will be handled by KeyframeAnimationEffect. See the FIXME in
KeyframeAnimationEffect::normalizedKeyframes(). The reason I have't fixed this
yet is that the spec behaviour is still being debated. We should probably just
RELEASE_ASSERT_WITH_MESSAGE() if _any_ keyframes are missing offsets for now.

[rjwright, 2013/12/04 05:05:54]

Done.

+    // Can restrict input to 2 keyframes, or check that either all keyframes have offsets
+    // or no keyframes have offsets, or add release assert where implementation is missing.
+    timeline->play(animation.get());
+}
+
+} // namespace WebCore