Bug 156096 – [web-animations] Add Animatable, AnimationEffect, KeyframeEffect and Animation interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Rawinder Singh]

This patch adds:

- Animatable interface (as an extension to element) and basic implementation of getAnimations method
- Document getAnimations method
- AnimationEffect interface stub
- KeyframeEffect interface and constructor implementation
- 'Animation' interface, constructor and query methods for effect and timeline
- A test case to query document.timeline and getAnimations (i.e. checks that animation has been linked to target and document)

[Rawinder Singh]

Created attachment 275372 [details]
Patch

[Build Bot]

Comment on attachment 275372 [details]
Patch

Attachment 275372 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1079475

New failing tests:
webanimations/Document.html

[Build Bot]

Created attachment 275378 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275372 [details]
Patch

Attachment 275372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1079480

New failing tests:
webanimations/Document.html

[Build Bot]

Created attachment 275379 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275372 [details]
Patch

Attachment 275372 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1079487

New failing tests:
webanimations/Document.html

[Build Bot]

Created attachment 275382 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275372 [details]
Patch

Attachment 275372 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1079490

New failing tests:
perf/nested-combined-selectors.html
webanimations/Document.html

[Build Bot]

Created attachment 275383 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Rawinder Singh]

Created attachment 275646 [details]
Patch

[Rawinder Singh]

Comment on attachment 275646 [details]
Patch

Enable Web Animations API runtime flag by default.
As a consequence, this patch is no longer dependant on the fix of Bug #156101.

[Build Bot]

Comment on attachment 275646 [details]
Patch

Attachment 275646 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1102040

New failing tests:
webanimations/Document.html

[Build Bot]

Created attachment 275647 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275646 [details]
Patch

Attachment 275646 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1102058

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 275648 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275646 [details]
Patch

Attachment 275646 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1102065

New failing tests:
webanimations/Document.html

[Build Bot]

Created attachment 275649 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Rawinder Singh]

Created attachment 275741 [details]
Patch

[Build Bot]

Comment on attachment 275741 [details]
Patch

Attachment 275741 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1107079

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 275746 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275741 [details]
Patch

Attachment 275741 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1107080

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 275747 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275741 [details]
Patch

Attachment 275741 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1107081

New failing tests:
perf/nested-combined-selectors.html

[Build Bot]

Created attachment 275748 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275741 [details]
Patch

Attachment 275741 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1107088

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 275749 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Rawinder Singh]

Created attachment 275755 [details]
Patch

[Build Bot]

Comment on attachment 275755 [details]
Patch

Attachment 275755 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1107466

New failing tests:
perf/nested-combined-selectors.html

[Build Bot]

Created attachment 275758 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Rawinder Singh]

Created attachment 278475 [details]
Patch

[Rawinder Singh]

The latest version of the patch addresses the issue of perf/nested-combined-selectors.html failing on the iOS simulator.  This was indirectly caused because I originally implemented the Element/Animatable interface relationship as a partial interface.  To implement the partial interface Element needed to inherit from Supplement<Element>.

As part of the perf/nested-combined-selectors.html test 256 Element objects are created/destroyed. They inherit from Supplemental<Element> which contains WTF::HashMap. It seems to be the presence of the hash map that affects the timing results.

The relationship between the Element/Animatable interface now follows the spec more accurately – that is, in the Web IDL ‘Element implements Animatable’ and the Element class implements the functionality required rather than introducing another class into the inheritance hierarchy.

The second item this patch update addresses is the runtime flag.  Initially, the runtime flag and compile time flags both controlled the existence of the Web Animations API interface. This wasn’t the right way to do it. The compile time flag should control whether the Web Animations API is available. The runtime flag should control whether CSS animations, transitions, and maybe SVG animations use the Web Animations engine rather than their own existing engines.

[Darin Adler]

Comment on attachment 278475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278475&action=review

I many small problems here that should be fixed before landing this code.

> Source/WebCore/animation/Animatable.idl:33
> +#if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C

Is this needed? I think Objective-C bindings will already skip getAnimations because it’s return type is sequence<WebAnimation>.

> Source/WebCore/animation/Animatable.idl:34
> +    // FIX: Incomplete interface (implement animate).

WebKit code uses FIXME for this, not FIX.

> Source/WebCore/animation/AnimationEffect.cpp:39
> +    : m_animation(nullptr)

Data members with simple types like this in new code should be initialized in the header, not in constructors.

> Source/WebCore/animation/AnimationEffect.cpp:54
> +    // FIX: Calculate whether animation is current according to spec.

WebKIt code uses FIXME, not FIX.

> Source/WebCore/animation/AnimationEffect.cpp:60
> +    // FIX: Calculate whether animation is in effect according to spec.

Ditto.

> Source/WebCore/animation/AnimationEffect.h:50
> +    WebAnimation* m_animation;

Why is it safe for an effect to have a raw pointer to a WebAnimation object? Since AnimationEffect is reference counted, it seems clear that an AnimationEffect could outlast the WebAnimation it is pointing to. So I believe this is unsafe.

> Source/WebCore/animation/AnimationTimeline.cpp:65
> +    if (classType() == DocumentTimelineClass) {
> +        downcast<DocumentTimeline>(*this).attach(animation);
> +        return;
> +    }

Why is this class using "static polymorphism"? I don’t understand what makes this necessary or desirable.

> Source/WebCore/animation/AnimationTimeline.cpp:68
> +    return;

Should not have a return at the end of a function like this.

> Source/WebCore/animation/AnimationTimeline.h:54
> +    void attachAnimation(WebAnimation*);
> +    void detachAnimation(WebAnimation*);

The functions should take WebAnimation&, not WebAnimation*.

> Source/WebCore/animation/AnimationTimeline.idl:-30
> -    EnabledAtRuntime=WebAnimations,

Why this change? It’s not mentioned in the change log.

> Source/WebCore/animation/DocumentAnimation.cpp:83
> +    struct {
> +        bool operator()(RefPtr<WebAnimation> a, RefPtr<WebAnimation> b)
> +        {
> +            // FIX: Sort using the composite order as described in the spec.
> +            UNUSED_PARAM(a);
> +            UNUSED_PARAM(b);
> +            return true;
> +        }
> +    } sortBasedOnPriority;

Should use a lambda, not a struct.

Arguments should be const RefPtr&, not RefPtr to avoid churning reference counts.

> Source/WebCore/animation/DocumentAnimation.cpp:85
> +    for (const auto& animation : m_animations) {

No need for const here.

> Source/WebCore/animation/DocumentAnimation.cpp:105
> +    size_t pos = m_animations.find(animation);

WebKit code uses words, not abbreviations, so this would be position, not pos.

The linear search here indicates we are using the wrong data structure for m_animations. We would need a data structure where removal is not linear. HashSet may be a good choice.

> Source/WebCore/animation/DocumentAnimation.cpp:106
> +    if (pos == WTF::notFound)

No need for WTF:: prefix here.

> Source/WebCore/animation/DocumentAnimation.h:60
> +    struct EffectCheck {
> +        virtual ~EffectCheck() { }
> +        bool operator()(AnimationEffect const& effect) const
> +        {
> +            return (effect.isCurrent() || effect.isInEffect()) && conditions(effect);
> +        }
> +
> +        virtual bool conditions(AnimationEffect const&) const { return true; }
> +    };
> +    WebAnimationVector getAnimations(EffectCheck const& = EffectCheck()) const;

Seems like this should take a std::function, rather than a custom class. Also, we don’t use the syntax EffectCheck const&, we put the const first.

WebKit coding style forbids the use of the word "get" in the name of a function like this. Unless that’s a name that comes directly from a WebIDL file from a web standard for this class.

> Source/WebCore/animation/DocumentAnimation.h:63
> +    void addAnimation(WebAnimation *);
> +    void removeAnimation(WebAnimation *);

These functions should take WebAnimation&, not WebAnimation*. Also the * goes next to the type name.

> Source/WebCore/animation/DocumentAnimation.idl:32
>  #if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C

Not sure this is right.

> Source/WebCore/animation/DocumentTimeline.h:42
> +    static PassRefPtr<DocumentTimeline> create(ScriptExecutionContext&, double);

The type of this argument should be Document&, not ScriptExecutionContext&.

New code should use Ref for a function result like this, not PassRefPtr.

> Source/WebCore/animation/DocumentTimeline.h:46
> +    void attach(WebAnimation*);
> +    void detach(WebAnimation*);

These function should take WebAnimation&, not WebAnimation*.

> Source/WebCore/animation/DocumentTimeline.h:49
> +    DocumentTimeline(ScriptExecutionContext&, double);

Ditto.

> Source/WebCore/animation/DocumentTimeline.h:52
> +    Document& m_document;

What guarantees this reference counted object won’t outlast the document it is pointing to?

> Source/WebCore/animation/KeyframeEffect.cpp:39
> +PassRefPtr<KeyframeEffect> KeyframeEffect::create(Element* target)

New code should use RefPtr or Ref for a function like this, not Ref.

> Source/WebCore/animation/KeyframeEffect.h:41
> +    static PassRefPtr<KeyframeEffect> create(Element*);

Argument type should be Element&, not Element*.

> Source/WebCore/animation/KeyframeEffect.h:49
> +    RefPtr<Element> m_target;

Should be Ref<Element>, not RefPtr<Element>.

Do we have a guarantee that there is no reference cycle here? Can the Element indirectly reference this effect? Maybe through an event handler function attached to the element that has a pointer to the effect? I think maybe it can and so this won’t work.

> Source/WebCore/animation/WebAnimation.h:44
> +    static PassRefPtr<WebAnimation> create(AnimationEffect*, AnimationTimeline*);

Should return RefPtr, not PassRefPtr, in new code. Please read the updated RefPtr document.

> Source/WebCore/dom/Element.cpp:1395
> +    struct EffectCheckTarget final : public DocumentAnimation::EffectCheck {
> +        EffectCheckTarget(Element& target)
> +            : m_target(target)
> +        {
> +        }
> +
> +        bool conditions(AnimationEffect const& effect) const override
> +        {
> +            return (static_cast<KeyframeEffect const&>(effect).target() == &m_target);
> +        }
> +        Element& m_target;
> +    };
> +    EffectCheckTarget checkTarget(*this);

This should be a lambda, not a class.

> Source/WebCore/dom/Element.h:519
> +    Vector<WebAnimation *> getAnimations();

WebKit coding style says this is <WebAnimation*>, not <WebAnimation *> with a space.

> LayoutTests/ChangeLog:16
> +        * platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt:

We need to update the other platform variations of this file, not just the mac-yosemite one.

[Dean Jackson]

Comment on attachment 278475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278475&action=review

> Source/JavaScriptCore/ChangeLog:13
> +        - Animatable interface and implementation of getAnimations in Element
> +        - Interface and implementation for Document getAnimations method.
> +        - AnimationEffect interface and class stub.
> +        - KeyframeEffect interface and constructor implementation.
> +        - 'Animation' interface, constructor and query methods for effect and timeline.

I wonder if some of these could be split into separate patches.

>> Source/WebCore/animation/Animatable.idl:33
>> +#if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
> 
> Is this needed? I think Objective-C bindings will already skip getAnimations because it’s return type is sequence<WebAnimation>.

Do we even need to worry about the Objective-C bindings?

>> Source/WebCore/animation/AnimationTimeline.idl:-30
>> -    EnabledAtRuntime=WebAnimations,
> 
> Why this change? It’s not mentioned in the change log.

It is mentioned (right at the end), but I would like to keep the runtime flag on. I think it's important that a new API isn't visible if the runtime flag is off, otherwise we'll get content that is detecting the API even though they can't use the feature.

> Source/WebCore/animation/WebAnimation.idl:34
> +[
> +    Conditional=WEB_ANIMATIONS,
> +    InterfaceName=Animation,
> +    ImplementationLacksVTable,
> +    Constructor([Default=Undefined] AnimationEffect? effect, [Default=Undefined] AnimationTimeline? timeline)
> +] interface WebAnimation { // FIX: Inherit from EventTarget.

I would like runtime enabling of all these new interfaces.

> Source/WebCore/dom/Element.idl:242
> +Element implements Animatable;

I guess this is why the runtime stuff was removed?

[Rawinder Singh]

Created attachment 279337 [details]
Patch

[Rawinder Singh]

(In reply to comment #32)
> Comment on attachment 278475 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278475&action=review
> 
> I many small problems here that should be fixed before landing this code.
> 
> > Source/WebCore/animation/Animatable.idl:33
> > +#if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
> 
> Is this needed? I think Objective-C bindings will already skip getAnimations
> because it’s return type is sequence<WebAnimation>.

You are right for getAnimations.  However, there are more functions to be added to this interface (i.e. Element.animate).

> 
> > Source/WebCore/animation/Animatable.idl:34
> > +    // FIX: Incomplete interface (implement animate).
> 
> WebKit code uses FIXME for this, not FIX.

Done.

> 
> > Source/WebCore/animation/AnimationEffect.cpp:39
> > +    : m_animation(nullptr)
> 
> Data members with simple types like this in new code should be initialized
> in the header, not in constructors.

Ok.

> 
> > Source/WebCore/animation/AnimationEffect.cpp:54
> > +    // FIX: Calculate whether animation is current according to spec.
> 
> WebKIt code uses FIXME, not FIX.
> 
> > Source/WebCore/animation/AnimationEffect.cpp:60
> > +    // FIX: Calculate whether animation is in effect according to spec.
> 
> Ditto.

Done.

> 
> > Source/WebCore/animation/AnimationEffect.h:50
> > +    WebAnimation* m_animation;
> 
> Why is it safe for an effect to have a raw pointer to a WebAnimation object?
> Since AnimationEffect is reference counted, it seems clear that an
> AnimationEffect could outlast the WebAnimation it is pointing to. So I
> believe this is unsafe.

You are right. I have updated the code to try and address this.

> 
> > Source/WebCore/animation/AnimationTimeline.cpp:65
> > +    if (classType() == DocumentTimelineClass) {
> > +        downcast<DocumentTimeline>(*this).attach(animation);
> > +        return;
> > +    }
> 
> Why is this class using "static polymorphism"? I don’t understand what makes
> this necessary or desirable.

The base class also uses a similar pattern so I didn’t really want to mix “static” and “dynamic” polymorphism (and introduce a vtable unnecessarily).

> 
> > Source/WebCore/animation/AnimationTimeline.cpp:68
> > +    return;
> 
> Should not have a return at the end of a function like this.

Ok.

> 
> > Source/WebCore/animation/AnimationTimeline.h:54
> > +    void attachAnimation(WebAnimation*);
> > +    void detachAnimation(WebAnimation*);
> 
> The functions should take WebAnimation&, not WebAnimation*.

Done.

> 
> > Source/WebCore/animation/AnimationTimeline.idl:-30
> > -    EnabledAtRuntime=WebAnimations,
> 
> Why this change? It’s not mentioned in the change log.

It is - see Dean’s comment 33 and reply for further discussion.

> 
> > Source/WebCore/animation/DocumentAnimation.cpp:83
> > +    struct {
> > +        bool operator()(RefPtr<WebAnimation> a, RefPtr<WebAnimation> b)
> > +        {
> > +            // FIX: Sort using the composite order as described in the spec.
> > +            UNUSED_PARAM(a);
> > +            UNUSED_PARAM(b);
> > +            return true;
> > +        }
> > +    } sortBasedOnPriority;
> 
> Should use a lambda, not a struct.

Ok.

> 
> Arguments should be const RefPtr&, not RefPtr to avoid churning reference
> counts.

Ok.

> 
> > Source/WebCore/animation/DocumentAnimation.cpp:85
> > +    for (const auto& animation : m_animations) {
> 
> No need for const here.

Ok.

> 
> > Source/WebCore/animation/DocumentAnimation.cpp:105
> > +    size_t pos = m_animations.find(animation);
> 
> WebKit code uses words, not abbreviations, so this would be position, not
> pos.
> 
> The linear search here indicates we are using the wrong data structure for
> m_animations. We would need a data structure where removal is not linear.
> HashSet may be a good choice.

I have made it a HashMap, as I am currently using WeakPtr for the reference to WebAnimation.  I use HashMap so that I can use the “animation” reference as the key (e.g. for remove), another option would be to update the WeakPtr class with a “==” operator.

> 
> > Source/WebCore/animation/DocumentAnimation.cpp:106
> > +    if (pos == WTF::notFound)
> 
> No need for WTF:: prefix here.

Ok.

> 
> > Source/WebCore/animation/DocumentAnimation.h:60
> > +    struct EffectCheck {
> > +        virtual ~EffectCheck() { }
> > +        bool operator()(AnimationEffect const& effect) const
> > +        {
> > +            return (effect.isCurrent() || effect.isInEffect()) && conditions(effect);
> > +        }
> > +
> > +        virtual bool conditions(AnimationEffect const&) const { return true; }
> > +    };
> > +    WebAnimationVector getAnimations(EffectCheck const& = EffectCheck()) const;
> 
> Seems like this should take a std::function, rather than a custom class.
> Also, we don’t use the syntax EffectCheck const&, we put the const first.

I’ve changed this to take a std::function and have a default lambda parameter for the base condition.

> 
> WebKit coding style forbids the use of the word "get" in the name of a
> function like this. Unless that’s a name that comes directly from a WebIDL
> file from a web standard for this class.

Yes, that’s the name of the function in IDL.

> 
> > Source/WebCore/animation/DocumentAnimation.h:63
> > +    void addAnimation(WebAnimation *);
> > +    void removeAnimation(WebAnimation *);
> 
> These functions should take WebAnimation&, not WebAnimation*. Also the *
> goes next to the type name.
> 
> > Source/WebCore/animation/DocumentAnimation.idl:32
> >  #if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
> 
> Not sure this is right.

I’m not sure what you think is wrong with it.  See discussion here: bug 151739, comment 8.

> 
> > Source/WebCore/animation/DocumentTimeline.h:42
> > +    static PassRefPtr<DocumentTimeline> create(ScriptExecutionContext&, double);
> 
> The type of this argument should be Document&, not ScriptExecutionContext&.
> 
> New code should use Ref for a function result like this, not PassRefPtr.

Ok.

> 
> > Source/WebCore/animation/DocumentTimeline.h:46
> > +    void attach(WebAnimation*);
> > +    void detach(WebAnimation*);
> 
> These function should take WebAnimation&, not WebAnimation*.

Done.

> 
> > Source/WebCore/animation/DocumentTimeline.h:49
> > +    DocumentTimeline(ScriptExecutionContext&, double);
> 
> Ditto.

Done.

> 
> > Source/WebCore/animation/DocumentTimeline.h:52
> > +    Document& m_document;
> 
> What guarantees this reference counted object won’t outlast the document it
> is pointing to?

Good point. I’ve now made it a WeakPtr - as DocumentAnimation has a reference to its default timeline – which may cause a reference cycle.

> 
> > Source/WebCore/animation/KeyframeEffect.cpp:39
> > +PassRefPtr<KeyframeEffect> KeyframeEffect::create(Element* target)
> 
> New code should use RefPtr or Ref for a function like this, not Ref.
> 
> > Source/WebCore/animation/KeyframeEffect.h:41
> > +    static PassRefPtr<KeyframeEffect> create(Element*);
> 
> Argument type should be Element&, not Element*.

According to the Web Animation specification, the target can be null.

> 
> > Source/WebCore/animation/KeyframeEffect.h:49
> > +    RefPtr<Element> m_target;
> 
> Should be Ref<Element>, not RefPtr<Element>.

It can be null, so should be RefPtr.

> 
> Do we have a guarantee that there is no reference cycle here? Can the
> Element indirectly reference this effect? Maybe through an event handler
> function attached to the element that has a pointer to the effect? I think
> maybe it can and so this won’t work.

According to the Web Animations specification and looking at the interfaces there isn’t a direct or indirect reference.

However, when we implement the underlying model this may change. For the moment I’d like to leave a FIXME comment in the code so that we can continue with the implementation and reorganize the references if the need arises. Let me know what you think?

> 
> > Source/WebCore/animation/WebAnimation.h:44
> > +    static PassRefPtr<WebAnimation> create(AnimationEffect*, AnimationTimeline*);
> 
> Should return RefPtr, not PassRefPtr, in new code. Please read the updated
> RefPtr document.

Done.

> 
> > Source/WebCore/dom/Element.cpp:1395
> > +    struct EffectCheckTarget final : public DocumentAnimation::EffectCheck {
> > +        EffectCheckTarget(Element& target)
> > +            : m_target(target)
> > +        {
> > +        }
> > +
> > +        bool conditions(AnimationEffect const& effect) const override
> > +        {
> > +            return (static_cast<KeyframeEffect const&>(effect).target() == &m_target);
> > +        }
> > +        Element& m_target;
> > +    };
> > +    EffectCheckTarget checkTarget(*this);
> 
> This should be a lambda, not a class.

Ok.

> 
> > Source/WebCore/dom/Element.h:519
> > +    Vector<WebAnimation *> getAnimations();
> 
> WebKit coding style says this is <WebAnimation*>, not <WebAnimation *> with
> a space.

Ok.

> 
> > LayoutTests/ChangeLog:16
> > +        * platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt:
> 
> We need to update the other platform variations of this file, not just the
> mac-yosemite one.

I tried resetting the results for the gtk build and it seems like the expected results includes a lot of changes unrelated to this patch. See LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt

I believe that this may be the case for the other platform variations as well (e.g. win and efl). I’m not sure what to do here (I don’t think a result reset for all platforms should be part of this patch).

[Rawinder Singh]

(In reply to comment #33)
> Comment on attachment 278475 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278475&action=review
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        - Animatable interface and implementation of getAnimations in Element
> > +        - Interface and implementation for Document getAnimations method.
> > +        - AnimationEffect interface and class stub.
> > +        - KeyframeEffect interface and constructor implementation.
> > +        - 'Animation' interface, constructor and query methods for effect and timeline.
> 
> I wonder if some of these could be split into separate patches.

This is the smallest patch I could create to introduce some actual functionality (due to the dependencies between the classes).

> 
> >> Source/WebCore/animation/Animatable.idl:33
> >> +#if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
> > 
> > Is this needed? I think Objective-C bindings will already skip getAnimations because it’s return type is sequence<WebAnimation>.
> 
> Do we even need to worry about the Objective-C bindings?
> 
> >> Source/WebCore/animation/AnimationTimeline.idl:-30
> >> -    EnabledAtRuntime=WebAnimations,
> > 
> > Why this change? It’s not mentioned in the change log.
> 
> It is mentioned (right at the end), but I would like to keep the runtime
> flag on. I think it's important that a new API isn't visible if the runtime
> flag is off, otherwise we'll get content that is detecting the API even
> though they can't use the feature.
> 
> > Source/WebCore/animation/WebAnimation.idl:34
> > +[
> > +    Conditional=WEB_ANIMATIONS,
> > +    InterfaceName=Animation,
> > +    ImplementationLacksVTable,
> > +    Constructor([Default=Undefined] AnimationEffect? effect, [Default=Undefined] AnimationTimeline? timeline)
> > +] interface WebAnimation { // FIX: Inherit from EventTarget.
> 
> I would like runtime enabling of all these new interfaces.
> 
> > Source/WebCore/dom/Element.idl:242
> > +Element implements Animatable;
> 
> I guess this is why the runtime stuff was removed?

Yes.

There is an issue for “Element implements Animatable” and “partial interface Document” (DocumentAnimation.idl).

I have added a bug 156101 for this – but it doesn’t look like a simple fix.

We also have a compile time flag to enable/disable the Web Animations API.

[Rawinder Singh]

Created attachment 279339 [details]
Patch

[Rawinder Singh]

(In reply to comment #35)

> > > Source/WebCore/animation/DocumentAnimation.cpp:105
> > > +    size_t pos = m_animations.find(animation);
> > 
> > WebKit code uses words, not abbreviations, so this would be position, not
> > pos.
> > 
> > The linear search here indicates we are using the wrong data structure for
> > m_animations. We would need a data structure where removal is not linear.
> > HashSet may be a good choice.
> 
> I have made it a HashMap, as I am currently using WeakPtr for the reference
> to WebAnimation.  I use HashMap so that I can use the “animation” reference
> as the key (e.g. for remove), another option would be to update the WeakPtr
> class with a “==” operator.

I've created a patch for Bug 157883 if using the HashSet is the preferred solution.

[Rawinder Singh]

(In reply to comment #38)
> (In reply to comment #35)
> 
> > > > Source/WebCore/animation/DocumentAnimation.cpp:105
> > > > +    size_t pos = m_animations.find(animation);
> > > 
> > > WebKit code uses words, not abbreviations, so this would be position, not
> > > pos.
> > > 
> > > The linear search here indicates we are using the wrong data structure for
> > > m_animations. We would need a data structure where removal is not linear.
> > > HashSet may be a good choice.
> > 
> > I have made it a HashMap, as I am currently using WeakPtr for the reference
> > to WebAnimation.  I use HashMap so that I can use the “animation” reference
> > as the key (e.g. for remove), another option would be to update the WeakPtr
> > class with a “==” operator.
> 
> I've created a patch for Bug 157883 if using the HashSet is the preferred
> solution.

It looks like I can't have a HashSet<WeakPtr<WebAnimation>>.

[Dean Jackson]

Comment on attachment 279339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279339&action=review

> LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt:96
> -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').value is AudioStreamTrack
> -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').hasOwnProperty('get') is false
> -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').hasOwnProperty('set') is false
> -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').enumerable is false
> -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').configurable is true
> +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').value is AudioTrack
> +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').hasOwnProperty('get') is false
> +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').hasOwnProperty('set') is false
> +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').enumerable is false
> +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').configurable is true
> +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrackList').value is AudioTrackList

I'm confused as to why there are all these changes here. Was it just because the test results were out of date?

[Rawinder Singh]

(In reply to comment #40)
> Comment on attachment 279339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279339&action=review
> 
> > LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt:96
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').value is AudioStreamTrack
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').hasOwnProperty('get') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').hasOwnProperty('set') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').enumerable is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').configurable is true
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').value is AudioTrack
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').hasOwnProperty('get') is false
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').hasOwnProperty('set') is false
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').enumerable is false
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').configurable is true
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrackList').value is AudioTrackList
> 
> I'm confused as to why there are all these changes here. Was it just because
> the test results were out of date?

Yes - see my last comment at the bottom of Comment 35 - related to Darin saying we needed to update other platforms (i.e. not just mac-yosemite) for global-constructors-attributes-expected.txt:

> I tried resetting the results for the gtk build and it seems like the expected results includes a lot of changes unrelated to this patch. See LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt
>
> I believe that this may be the case for the other platform variations as well (e.g. win and efl). I’m not sure what to do here (I don’t think a result reset for all platforms should be part of this patch).

[Rawinder Singh]

(In reply to comment #40)
> Comment on attachment 279339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279339&action=review
> 
> > LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt:96
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').value is AudioStreamTrack
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').hasOwnProperty('get') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').hasOwnProperty('set') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').enumerable is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'AudioStreamTrack').configurable is true
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').value is AudioTrack
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').hasOwnProperty('get') is false
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').hasOwnProperty('set') is false
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').enumerable is false
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrack').configurable is true
> > +PASS Object.getOwnPropertyDescriptor(global, 'AudioTrackList').value is AudioTrackList
> 
> I'm confused as to why there are all these changes here. Was it just because
> the test results were out of date?

It was marked as cq? but didn't land - are there still some issues with the patch?

[WebKit Commit Bot]

Comment on attachment 279339 [details]
Patch

Clearing flags on attachment: 279339

Committed r201810: <http://trac.webkit.org/changeset/201810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158563

[Michael Catanzaro]

Created attachment 280915 [details]
Build error

This broke the build without ENABLE_WEB_ANIMATIONS

The bots didn't notice because it's enabled for developer builds by FeatureList.pm, but it's disabled for end users in WebKitFeatures.cmake.

[Rawinder Singh]

(In reply to comment #46)
> Created attachment 280915 [details]
> Build error
> 
> This broke the build without ENABLE_WEB_ANIMATIONS
> 
> The bots didn't notice because it's enabled for developer builds by
> FeatureList.pm, but it's disabled for end users in WebKitFeatures.cmake.

This has now been fixed in Element.idl by specifying the Conditional:
[Conditional=WEB_ANIMATIONS] Element implements Animatable;

[Rawinder Singh]

Created attachment 281324 [details]
Patch

[Chris Dumez]

Comment on attachment 281324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281324&action=review

> Source/WebCore/dom/Element.idl:243
> +[Conditional=WEB_ANIMATIONS] Element implements Animatable;

Animatable is already marked as [Conditional=WEB_ANIMATIONS] so what does this bring exactly? All attributes / operations imported from Animatable to Element should already be marked as [Conditional=WEB_ANIMATIONS] without this change.

[Chris Dumez]

Comment on attachment 281324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281324&action=review

> Source/WebCore/CMakeLists.txt:3026
> +        "animation/Animatable.idl"

I believe the issue you had preprocess-idls.pl is because Animatable.idl is added conditionally here. So if WEB_ANIMATIONS is disabled, Animatable.idl is not passed to preprocess-idl.pl and you get an exception.
We already have [Conditional=WEB_ANIMATIONS] in the IDL, why do we need this to be conditional in the CMakeLists.txt too?

[Rawinder Singh]

Created attachment 281535 [details]
Issue with moving idl

(In reply to comment #50)
> Comment on attachment 281324 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281324&action=review
> 
> > Source/WebCore/CMakeLists.txt:3026
> > +        "animation/Animatable.idl"
> 
> I believe the issue you had preprocess-idls.pl is because Animatable.idl is
> added conditionally here. So if WEB_ANIMATIONS is disabled, Animatable.idl
> is not passed to preprocess-idl.pl and you get an exception.
> We already have [Conditional=WEB_ANIMATIONS] in the IDL, why do we need this
> to be conditional in the CMakeLists.txt too?

Moving the *.idl and *.cpp files inside the condition to the main body seems to cause a new set of issues (see the attachment for the error log).

[youenn fablet]

This patch seems to do several things at the same time.
I would try to further split the patch if that is possible.
It might be easier to get reviewed and digested by the bots.

[Rawinder Singh]

(In reply to comment #52)
> This patch seems to do several things at the same time.
> I would try to further split the patch if that is possible.
> It might be easier to get reviewed and digested by the bots.

Hi Youenn,

Thanks for the review comments, however this is the smallest patch I could create to introduce some actual functionality (due to the dependencies between the classes).

Dean previously review+ it and it passed the bots but was rolled back due to an issue (as discussed above).

Currently, this patch is waiting for one of the following bugs to be resolved (before I can resubmit it again):

- Bug #158975 - [WebIDL] Add extra checking of conditional attributes when including header files for the function type.

- Bug #158830 - [WebIDL] Add support for Conditional extended attribute for implement statement.

If you are able to look at either of these and share any thoughts that would be great

[Rawinder Singh]

Created attachment 285817 [details]
Patch

[Rawinder Singh]

Comment on attachment 285817 [details]
Patch

The latest patch fixes issue with the GTK build when web animations is turned off and issues discussed in Bug #158830, comment 14.

[WebKit Commit Bot]

Comment on attachment 285817 [details]
Patch

Rejecting attachment 285817 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 285817, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ibutes-expected.txt
patching file LayoutTests/platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt
Hunk #3 succeeded at 928 (offset 15 lines).
patching file LayoutTests/webanimations/Document-expected.txt
patching file LayoutTests/webanimations/Document.html
patching file LayoutTests/webanimations/script-tests/Document.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dean Jackson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/1888949

[Antoine Quint]

Created attachment 286366 [details]
Patch

[Antoine Quint]

Created attachment 286367 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 286367 [details]
Patch for landing

Clearing flags on attachment: 286367

Committed r204594: <http://trac.webkit.org/changeset/204594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 286367 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=286367&action=review

> Source/WebCore/animation/DocumentAnimation.cpp:71
> +WebAnimationVector DocumentAnimation::getAnimations(std::function<bool(const AnimationEffect&)> effect_test) const

This function name does not match WebKit coding style. We don’t use "get" in the names of functions with return values.

[Rawinder Singh]

(In reply to comment #61)
> Comment on attachment 286367 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286367&action=review
> 
> > Source/WebCore/animation/DocumentAnimation.cpp:71
> > +WebAnimationVector DocumentAnimation::getAnimations(std::function<bool(const AnimationEffect&)> effect_test) const
> 
> This function name does not match WebKit coding style. We don’t use "get" in
> the names of functions with return values.

Please see comment #35:


> > WebKit coding style forbids the use of the word "get" in the name of a
> > function like this. Unless that’s a name that comes directly from a WebIDL
> > file from a web standard for this class.
>
> Yes, that’s the name of the function in IDL.