Bug 151688 – [web-animations] Add AnimationTimeline, DocumentTimeline and add extensions to Document interface

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

[Nikos Andronikos]

This patch:

- Adds DocumentTimeline interface and class implementation
- Implements the DocumentAnimation extension to the Document Interface that contains a default DocumentTimeline
- Add AnimationTimeline interface stub (i.e. without getAnimations and currentTime)
- Adds AnimationTimeline class implementation for AnimationTimeline interface stub
- Adds ObjectiveC and Javascript bindings for the above classes and interfaces
- Enables the WEB_ANIMATIONS compiler switch

Note:
This patch adds classes but no real functionality yet. Tests will be added as class functionality is fleshed out.

This patch supersedes https://bugs.webkit.org/show_bug.cgi?id=123225 (which was simply to implement a stub DocumentTimeline class and reference it from Document)

[Nikos Andronikos]

Created attachment 266311 [details]
Test Patch - trying to diagnose script issues.

[Nikos Andronikos]

Created attachment 266317 [details]
Patch

[Nikos Andronikos]

Created attachment 266429 [details]
Patch

Checking build against fix (bug 151739) made for errors generated by bug 151682

[Nikos Andronikos]

Created attachment 266431 [details]
Patch

[Nikos Andronikos]

Comment on attachment 266431 [details]
Patch

Checking build against fix (bug 151739) made for errors generated by bug 151682 and fix up issue with missing file in CMake builds

[Nikos Andronikos]

Created attachment 266751 [details]
Patch - Checking build against fix (bug 151739) and fixing issues with GTK and Win build

[Nikos Andronikos]

Created attachment 270909 [details]
Patch

[Dean Jackson]

Comment on attachment 270909 [details]
Patch

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

> Source/WebCore/PlatformMac.cmake:175
> +    bindings/objc/DOMWebAnimations.mm

Don't worry about this. We don't need ObjC bindings.

> Source/WebCore/animation/AnimationTimeline.cpp:2
> + * Copyright (C) Canon Inc. 2015

2016

> Source/WebCore/animation/AnimationTimeline.cpp:33
> + */
> +#include "config.h"
> +
> +#if ENABLE(WEB_ANIMATIONS)
> +#include "AnimationTimeline.h"
> +
> +#include "DocumentTimeline.h"

Nits:

- needs blank line after license
- might as well put the #if ENABLE first
- blank line after #if ENABLE

> Source/WebCore/animation/AnimationTimeline.cpp:40
> +    // NOTE: We only have one type of subclass at the moment

Nit: needs .

> Source/WebCore/animation/AnimationTimeline.h:2
> + * Copyright (C) Canon Inc. 2015

2016 (all of them are like this)

> Source/WebCore/animation/DocumentAnimation.cpp:32
> + */
> +#include "config.h"
> +#include "DocumentAnimation.h"
> +
> +#if ENABLE(WEB_ANIMATIONS)
> +

Nits: same as before.

> Source/WebCore/animation/DocumentAnimation.cpp:48
> +    DocumentAnimation* documentAnimation = DocumentAnimation::from(&document);

Use auto.

> Source/WebCore/animation/DocumentAnimation.cpp:50
> +        documentAnimation->m_defaultTimeline = DocumentTimeline::create(0);

nullptr rather than 0

> Source/WebCore/bindings/objc/DOMWebAnimations.h:1
> +/*

I don't think we need to bother with ObjC bindings for this. Just the JS.

> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:104
> +                    "NodeIterator" => 1, "TreeWalker" => 1, "AbstractView" => 1, "Blob" => 1, "AnimationTimeline" => 1);

... so no need for this.

> WebKitLibraries/win/tools/vsprops/FeatureDefines.props:1
> -<?xml version="1.0" encoding="utf-8"?>
> +<?xml version="1.0" encoding="utf-8"?>

I'm a bit worried about this. Was there a reason we had the BOM?

> WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.props:1
> -<?xml version="1.0" encoding="utf-8"?>
> +<?xml version="1.0" encoding="utf-8"?>

Same.

[Nikos Andronikos]

Created attachment 271537 [details]
Patch

[Nikos Andronikos]

(In reply to comment #8)
> Comment on attachment 270909 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270909&action=review
> 
> > Source/WebCore/PlatformMac.cmake:175
> > +    bindings/objc/DOMWebAnimations.mm
> 
> Don't worry about this. We don't need ObjC bindings.

Okay, done.

> 
> > Source/WebCore/animation/AnimationTimeline.cpp:2
> > + * Copyright (C) Canon Inc. 2015
> 
> 2016

Done.

> 
> > Source/WebCore/animation/AnimationTimeline.cpp:33
> > + */
> > +#include "config.h"
> > +
> > +#if ENABLE(WEB_ANIMATIONS)
> > +#include "AnimationTimeline.h"
> > +
> > +#include "DocumentTimeline.h"
> 
> Nits:
> 
> - needs blank line after license
> - might as well put the #if ENABLE first
> - blank line after #if ENABLE

config.h needs to be before the #define. Otherwise, done.

> 
> > Source/WebCore/animation/AnimationTimeline.cpp:40
> > +    // NOTE: We only have one type of subclass at the moment
> 
> Nit: needs .
> 
> > Source/WebCore/animation/AnimationTimeline.h:2
> > + * Copyright (C) Canon Inc. 2015
> 
> 2016 (all of them are like this)
> 
> > Source/WebCore/animation/DocumentAnimation.cpp:32
> > + */
> > +#include "config.h"
> > +#include "DocumentAnimation.h"
> > +
> > +#if ENABLE(WEB_ANIMATIONS)
> > +
> 
> Nits: same as before.
> 
> > Source/WebCore/animation/DocumentAnimation.cpp:48
> > +    DocumentAnimation* documentAnimation = DocumentAnimation::from(&document);
> 
> Use auto.
> 
> > Source/WebCore/animation/DocumentAnimation.cpp:50
> > +        documentAnimation->m_defaultTimeline = DocumentTimeline::create(0);
> 
> nullptr rather than 0

This takes a double not a ptr.

> 
> > Source/WebCore/bindings/objc/DOMWebAnimations.h:1
> > +/*
> 
> I don't think we need to bother with ObjC bindings for this. Just the JS.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:104
> > +                    "NodeIterator" => 1, "TreeWalker" => 1, "AbstractView" => 1, "Blob" => 1, "AnimationTimeline" => 1);
> 
> ... so no need for this.
> 
> > WebKitLibraries/win/tools/vsprops/FeatureDefines.props:1
> > -<?xml version="1.0" encoding="utf-8"?>
> > +<?xml version="1.0" encoding="utf-8"?>
> 
> I'm a bit worried about this. Was there a reason we had the BOM?
> 
> > WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.props:1
> > -<?xml version="1.0" encoding="utf-8"?>
> > +<?xml version="1.0" encoding="utf-8"?>
> 
> Same.

Fixed.

[WebKit Commit Bot]

Comment on attachment 271537 [details]
Patch

Rejecting attachment 271537 [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-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 271537, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 197051 = c7ebd6663a86cc3e2258730f9e46406d8453e32e
r197052 = 87f285b2c68e4e99a87821bf7664f9fb67535a62
r197053 = e42d611c93de15481d12d5dd2fb7a10306c28255
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

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

[Dean Jackson]

Committed r197058: <http://trac.webkit.org/changeset/197058>

[Rawinder Singh]

*** Bug 123225 has been marked as a duplicate of this bug. ***