Closed Bug 1108055 Opened 10 years ago Closed 9 years ago

Implement ComputedTiming for Web Animation API

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: milan.loveless, Assigned: boris, Mentored)

References

(
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 32 obsolete files)

Part 1: Add ComputedTimingProperties dictionary (v6)
3.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 2: Refine ComputedTiming. (v6, carry birtles' r+)
20.09 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 3: Implement GetComputedTiming method. (v9, carry birtles' r+)
7.45 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 4: Add ComputedTiming mochitests. (v12, carry birtles' r+)
35.59 KB, patch
boris
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.122 Safari/537.36 Steps to reproduce: I read the web animation spec at http://w3c.github.io/web-animations/#the-computedanimationtiming-interface I saw that this was not yet implemented in the source. Actual results: nothing! Expected results: Someone should write this code.
URL: http://w3c.github.io/web-animations/#...
OS: Mac OS X → All
Hardware: x86 → All
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Hi Milan, Thanks for filing this. Is there any reason you marked this invalid? It looks valid to me so I'm marking it re-opened.
Blocks: 998245
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
I thought I had found it already implemented after we discussed working on this. There is a similarly named method in the source. I'll start implementing right away.
(In reply to Milan Loveless from comment #2) > I thought I had found it already implemented after we discussed working on > this. There is a similarly named method in the source. I'll start > implementing right away. No problem. There is a method in the source already but it's not exposed to script and doesn't provide all the required fields yet. Let me know how you go. I'd be happy to spell out the steps I think are required to implement this.
That would be very helpful. I'm still trying to figure out which parts are already in place and which are not. Thanks!
Ok, roughly I think the pieces are: 1. Add the ComputedTimingProperties dictionary definition to dom/webidl/ComputedTimingProperties.webidl - You can copy the boilerplate from another file in that directory. - The definition itself is basically just copy-paste from the spec: http://w3c.github.io/web-animations/#dictdef-computedtimingproperties - This should be a separate patch because it requires review from a DOM peer. - Use 'mach build' to update with changes to the WebIDL (normally 'mach build binaries' is enough but it doesn't pick up WebIDL changes) - You can read up on details of the WebIDL to C++ mapping here: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types - I've never actually added a dictionary type so I'm not familiar with it (- There's a test somewhere that lists all interface names that you need to update when you add a new WebIDL interface but I'm pretty sure we don't need to add dictionaries to it.) 2. Update dom/webidl/Animation.webidl to add: readonly attribute ComputedTimingProperties computedTiming; - This should really be AnimationNode.webidl but currently we've flattened the AnimationNode and Animation interfaces into just Animation since we don't expect to implement animation groups for a little while - Once you try doing 'mach build' you'll probably get complaints because Animation::ComputedTiming() (or possibly GetComputedTiming) doesn't exist - Update dom/animation/Animation.{h,cpp} to add a stub implementation that just returns a ComputedTimingProperties dictionary object with all the members set to their default values. - I'd discourage actually trying to fill out the dictionary object with meaningful values and instead just keep this patch very simple so it's easy for a DOM peer to review. - At this point you should be able to load up a document with a running CSS animation or transition, open up the developer console, and write something like 'elem.getAnimationPlayers()[0].source.computedTiming'. That should bring up an object that you can click on and inspect its properties (which will all just be the defaults at this stage). 3. Write some tests in dom/animation/test/css-animations/test_animation-computed-timing.html - You'll need to add this file and then add it to the list of dom/animation/test/mochitest.ini - You can run the test by running: mach mochitest dom/animation/test/css-animations/test_animations-computed-timing.html (You can run all CSS animation tests with 'mach mochitest dom/animation/test/') - The tests should be as short as possible and you can probably get a good idea by looking at tests in nearby files - You probably want to write one or two tests for each property and check that they fail (as expected) 4. Likewise, we should write some tests for transitions which live in dom/animation/test/css-transitions/ 3 and 4 could be put together in a separate patch. 5. Actually fill in the parameters of computed timing! This is where we might need to change what we calculate which could be tricky. This is the fun bit though. Compare what we have to what's in the spec and see what we need to add. Once you get up to this point let me know and we can work it out together. 6. Calling getComputedTiming for CSS animations / transitions should flush style. We'll need to add a test for this and then do special handling for CSS animations / transitions. I'll be happy to explain this in more detail when we get up to it. I hope that's enough to get started!
Assignee: nobody → milan.loveless
Mentor: jwatt
Attached patch WIP Patch (obsolete) — — Splinter Review
I've added AnimationTimingProperties, ComputedTimingProperties, AnimationNodeReadonly, AnimationReadonly webidl files and have started to stub out the corresponding C++ files. I'm still figuring out some of the data types - unrestricted doubles, DOMStrings, and Union types.
Milan, your patch creation seems to have gone a bit wrong - it's empty. :)
Oops, let's try this again!
Attachment #8565854 - Attachment is obsolete: true
Is there an updated patch for these properties?
Attached patch computedTiming (obsolete) — — Splinter Review
Here's the current status. AnimationTimingProperties and ComputedTimingProperties have been implemented. More of their properties still need to be fleshed out and tests need to be added.
Comment on attachment 8572740 [details] [diff] [review] computedTiming Since I worked on this with Milan I probably shouldn't review it. Brian, can you?
Attachment #8572740 - Flags: review?(bbirtles)
Comment on attachment 8572740 [details] [diff] [review] computedTiming Review of attachment 8572740 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine to me (bar the comments below) but a DOM peer will have a much better idea. smaug, note that this is only a method temporarily. The spec says this should be an attribute (called computedTiming) because heycam assured me that we will probably allow dictionary attributes when we introduce FrozenArray attributes. At that point I guess we'll have to spec when the identity of the returned object changes. ::: dom/webidl/Animation.webidl @@ +18,5 @@ > readonly attribute AnimationEffect? effect; > // FIXME: This should be writeable (bug 1067769) > readonly attribute Element? target; > + [BinaryName="getComputedTimingProperties"] > + ComputedTimingProperties getComputedTiming(); Add a comment here saying that this will be changed to an attribute when WebIDL allows dictionary attributes. ::: dom/webidl/AnimationTimingProperties.webidl @@ +10,5 @@ > + * liability, trademark and document use rules apply. > + */ > + > +dictionary AnimationTimingProperties { > + double delay; This should specify the default value (0) as shown in the spec. http://w3c.github.io/web-animations/#dictdef-animationtimingproperties Likewise for the commented out members. ::: dom/webidl/ComputedTimingProperties.webidl @@ +12,5 @@ > + > +dictionary ComputedTimingProperties : AnimationTimingProperties { > +// double startTime; > +// unrestricted double endTime; > + unrestricted double activeDuration; The spec doesn't give any default values for these members but if we were to make up some defaults (e.g. 0.0 for the non-nullable members, and null for the nullable members) then I think they would end up being represented as plain doubles in the generated dictionary objects[1] which seems a little better than wrapping them in Optional<>. At least it would make setting them in Animation.cpp a little easier to read. Also, I don't *think* it would be an observable difference since these objects are only ever returned from the API, never passed-in. smaug will know better however. [1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types
Attachment #8572740 - Flags: review?(bugs)
Attachment #8572740 - Flags: review?(bbirtles)
Attachment #8572740 - Flags: review+
Also, obviously this needs tests but perhaps you're planning to do that along with implementing the other properties?
Comment on attachment 8572740 [details] [diff] [review] computedTiming One additional comment: >--- a/dom/animation/Animation.h >+++ b/dom/animation/Animation.h >@@ -12,16 +12,17 @@ > #include "nsIDocument.h" > #include "nsWrapperCache.h" > #include "mozilla/Attributes.h" > #include "mozilla/StickyTimeDuration.h" > #include "mozilla/StyleAnimationValue.h" > #include "mozilla/TimeStamp.h" > #include "mozilla/dom/Element.h" > #include "mozilla/dom/Nullable.h" >+#include "mozilla/dom/ComputedTimingPropertiesBinding.h" > #include "nsSMILKeySpline.h" > #include "nsStyleStruct.h" // for nsTimingFunction We should just use a forward declaration here. i.e. add: struct ComputedTimingProperties; after the following line: class AnimationEffect; Then include the .h file from Animation.cpp
Comment on attachment 8572740 [details] [diff] [review] computedTiming > [Func="nsDocument::IsWebAnimationsEnabled"] > interface Animation { > // FIXME: |effect| should have type (AnimationEffect or EffectCallback)? > // but we haven't implemented EffectCallback yet. > [Cached,Pure] > readonly attribute AnimationEffect? effect; > // FIXME: This should be writeable (bug 1067769) > readonly attribute Element? target; >+ [BinaryName="getComputedTimingProperties"] >+ ComputedTimingProperties getComputedTiming(); > }; I'm lost now with this. Shouldn't effect be AnimationEffectReadonly, and that dictionary has computedTiming >diff --git a/dom/webidl/AnimationTimingProperties.webidl b/dom/webidl/AnimationTimingProperties.webidl >new file mode 100644 >--- /dev/null >+++ b/dom/webidl/AnimationTimingProperties.webidl >@@ -0,0 +1,23 @@ >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. >+ * >+ * The origin of this IDL file is >+ * http://w3c.github.io/web-animations/#dictdef-animationtimingproperties The # part doesn't like to lead to anything. >+ * >+ * Copyright © 2014 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C >+ * liability, trademark and document use rules apply. >+ */ >+ >+dictionary AnimationTimingProperties { >+ double delay; >+// double endDelay; >+// FillMode fill; >+// double iterationStart; >+// unrestricted double iterations; >+// (unrestricted double or DOMString) duration; >+// double playbackRate; >+// PlaybackDirection direction; >+// DOMString easing; >+}; Missing default values. >+++ b/dom/webidl/ComputedTimingProperties.webidl >@@ -0,0 +1,20 @@ >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. >+ * >+ * The origin of this IDL file is >+ * http://w3c.github.io/web-animations/#the-computedanimationtiming-interface >+ * >+ * Copyright © 2014 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C >+ * liability, trademark and document use rules apply. >+ */ >+ >+dictionary ComputedTimingProperties : AnimationTimingProperties { >+// double startTime; >+// unrestricted double endTime; >+ unrestricted double activeDuration; >+// double? localTime; >+// unrestricted double timeFraction; >+// unsigned long? currentIteration; >+}; Do we really need a separate file for these dictionaries? Why not just put them to Animation.webidl or something. But if you or Brian or someone thinks separate files make this easier to handle, fine. But I can't really review this, since this is so different what the spec has. We shouldn't release stuff which is very different to specs. But Brian, please explain the situation.
Attachment #8572740 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #15) > Comment on attachment 8572740 [details] [diff] [review] > computedTiming > > > [Func="nsDocument::IsWebAnimationsEnabled"] > > interface Animation { > > // FIXME: |effect| should have type (AnimationEffect or EffectCallback)? > > // but we haven't implemented EffectCallback yet. > > [Cached,Pure] > > readonly attribute AnimationEffect? effect; > > // FIXME: This should be writeable (bug 1067769) > > readonly attribute Element? target; > >+ [BinaryName="getComputedTimingProperties"] > >+ ComputedTimingProperties getComputedTiming(); > > }; > > I'm lost now with this. Shouldn't effect be AnimationEffectReadonly, and > that dictionary has computedTiming Sorry, that's my fault. I made a major update to the Web Animations spec last Friday which involved renaming a number of things. The part added by this patch is still correct except that it's a method rather than an attribute since WebIDL doesn't yet support dictionary attributes. I think we could land this part and fix up all the interface names in a subsequent patch. > >diff --git a/dom/webidl/AnimationTimingProperties.webidl b/dom/webidl/AnimationTimingProperties.webidl > >new file mode 100644 > >--- /dev/null > >+++ b/dom/webidl/AnimationTimingProperties.webidl > >@@ -0,0 +1,23 @@ > >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > >+/* This Source Code Form is subject to the terms of the Mozilla Public > >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, > >+ * You can obtain one at http://mozilla.org/MPL/2.0/. > >+ * > >+ * The origin of this IDL file is > >+ * http://w3c.github.io/web-animations/#dictdef-animationtimingproperties > The # part doesn't like to lead to anything. Again, my fault. That is now: http://w3c.github.io/web-animations/#dictdef-animationeffecttimingproperties > >+ * > >+ * Copyright © 2014 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C > >+ * liability, trademark and document use rules apply. > >+ */ > >+ > >+dictionary AnimationTimingProperties { > >+ double delay; > >+// double endDelay; > >+// FillMode fill; > >+// double iterationStart; > >+// unrestricted double iterations; > >+// (unrestricted double or DOMString) duration; > >+// double playbackRate; > >+// PlaybackDirection direction; > >+// DOMString easing; > >+}; > Missing default values. Yes, we need to fix this. > >+dictionary ComputedTimingProperties : AnimationTimingProperties { > >+// double startTime; > >+// unrestricted double endTime; > >+ unrestricted double activeDuration; > >+// double? localTime; > >+// unrestricted double timeFraction; > >+// unsigned long? currentIteration; > >+}; > > Do we really need a separate file for these dictionaries? No, I don't think they need to be.
Thanks for the feedback everyone! I'll be working on this in the next few days and should have an update soon.
Attachment #8665345 - Attachment is obsolete: true
(In reply to Boris Chiou [:boris] from comment #19) > (In reply to Boris Chiou [:boris] from comment #18) > > (In reply to Milan Loveless from comment #17) > > > Thanks for the feedback everyone! I'll be working on this in the next few > > > days and should have an update soon. > > > > Hi, Milan > > > > Are you still working on this bug? > > If not, could I take this bug? > > Thanks. I take this bug. If you want to continue this work, please feel free to talk to me and take it back. Thanks.
Assignee: milan.loveless → boris.chiou
Flags: needinfo?(milan.loveless)
Hi, Brian I have a little question about this bug. In KeyframeEffect.h, we already has a ComputedTiming struct. (https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.h?offset=1000#73) However, this bug implements the ComputedTimingProperties dictionary which generates another ComputedTimingProperties struct in the binding header. My question is, are these structs identical? Should I replace the ComputedTiming struct and AnimationTiming struct in KeyframeEffect.h with the new generated ComputedTimingProperties and AnimationEffectTimingProperties structs? Thanks.
Flags: needinfo?(bbirtles)
Ideally if the struct generated by the WebIDL bindings looks reasonable then we could just use that instead. I'm pretty sure it won't have some stuff we need like animation phases however. Perhaps we can just inherit from the generated one and tack on the extra info we need. Can you paste/attach the generated struct somewhere so we can see what is missing? By the way, I spoke to Cameron today and we are wondering if, instead of returning a dictionary object, we should return a live ComputedTiming object instead. I'll send a mail to some of the lists tomorrow to see what others think.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #27) > By the way, I spoke to Cameron today and we are wondering if, instead of > returning a dictionary object, we should return a live ComputedTiming object > instead. I'll send a mail to some of the lists tomorrow to see what others > think. Sent: https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0000.html For now I think using getComputedTiming() is fine.
Component: DOM → DOM: Animation
(In reply to Brian Birtles (:birtles) from comment #27) > Ideally if the struct generated by the WebIDL bindings looks reasonable then > we could just use that instead. I'm pretty sure it won't have some stuff we > need like animation phases however. Perhaps we can just inherit from the > generated one and tack on the extra info we need. Can you paste/attach the > generated struct somewhere so we can see what is missing? > OK. I can use composition or inheritance method to reuse the generated one and add some stuff, such as animation phases. Here is the generated structs according to my webidl file in Attachment 8667142 [details] [diff] (local path: objdir/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h): https://pastebin.mozilla.org/8848039 > By the way, I spoke to Cameron today and we are wondering if, instead of > returning a dictionary object, we should return a live ComputedTiming object > instead. I'll send a mail to some of the lists tomorrow to see what others > think. Thanks for your help. I've read this mail.
(In reply to Boris Chiou [:boris] from comment #29) > (In reply to Brian Birtles (:birtles) from comment #27) > > Ideally if the struct generated by the WebIDL bindings looks reasonable then > > we could just use that instead. I'm pretty sure it won't have some stuff we > > need like animation phases however. Perhaps we can just inherit from the > > generated one and tack on the extra info we need. Can you paste/attach the > > generated struct somewhere so we can see what is missing? > > > OK. I can use composition or inheritance method to reuse the generated one > and add some stuff, such as animation phases. > Here is the generated structs according to my webidl file in Attachment > 8667142 [details] [diff] > (local path: > objdir/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h): > https://pastebin.mozilla.org/8848039 Thanks. That looks useable to me. But I guess you'll need to try using it and see if it is suitable or not.
(In reply to Brian Birtles (:birtles) from comment #28) > (In reply to Brian Birtles (:birtles) from comment #27) > > By the way, I spoke to Cameron today and we are wondering if, instead of > > returning a dictionary object, we should return a live ComputedTiming object > > instead. I'll send a mail to some of the lists tomorrow to see what others > > think. > > Sent: https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0000.html > > For now I think using getComputedTiming() is fine. Hi Brian, About this function "getComputedTiming()", we define it in AnimationEffectReadOnly.webidl. However, KeyframeEffect.h also defines this function and has a different signature (ex. different return type). (https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.h?from=ComputedTiming&offset=0#283 v.s. void AnimationEffectReadOnly::GetComputedTimingProperties(Nullable<ComputedTimingProperties>& aRetVal) ) Looks like these two function should do the same thing, so should I also merge these two function, or keep each one and let AnimationEffectReadOnly::GetComputedTiming() just return a default ComputedTimingProperties object? Thanks.
(In reply to Brian Birtles (:birtles) from comment #30) > (In reply to Boris Chiou [:boris] from comment #29) > > (In reply to Brian Birtles (:birtles) from comment #27) > > > Ideally if the struct generated by the WebIDL bindings looks reasonable then > > > we could just use that instead. I'm pretty sure it won't have some stuff we > > > need like animation phases however. Perhaps we can just inherit from the > > > generated one and tack on the extra info we need. Can you paste/attach the > > > generated struct somewhere so we can see what is missing? > > > > > OK. I can use composition or inheritance method to reuse the generated one > > and add some stuff, such as animation phases. > > Here is the generated structs according to my webidl file in Attachment > > 8667142 [details] [diff] > > (local path: > > objdir/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h): > > https://pastebin.mozilla.org/8848039 > > Thanks. That looks useable to me. But I guess you'll need to try using it > and see if it is suitable or not. I am trying to use it and I think it works. Only this data member, "mActiveDuration", has different types. We define it as "StickyTimeDuration" in KeyframeEffect.h, but it is defined as "unrestricted double" in the spec, so I just comment it out in AnimationEffectReadOnly.webidl to prevent the variable hiding problem.
(In reply to Boris Chiou [:boris] from comment #32) > I am trying to use it and I think it works. Only this data member, > "mActiveDuration", has different types. We define it as "StickyTimeDuration" > in KeyframeEffect.h, but it is defined as "unrestricted double" in the spec, > so I just comment it out in AnimationEffectReadOnly.webidl to prevent the > variable hiding problem. Oh, that's tricky. We really do want to keep the StickyTimeDuration representation and use that internally. When we return the API object, however, we should return just the double version (AnimationUtils::TimeDurationToDouble converts these values although we should probably add another version that takes a StickyTimeDuration and correctly handles Forever/infinity values). Maybe it's just easier to do something like: AnimationEffectReadOnly.webidl: struct ComputedTiming { // largely as-is but with the addition of: ComputedTimingProperties AsDict() const; // (the above would call AnimationUtils::TimeDurationToDouble etc. to convert) }; interface AnimationEffectReadOnly { [BinaryName="getComputedTimingAsDict"] readonly attribute ComputedTimingProperties computedTiming; }; class KeyframeEffectReadOnly { public: // ... ComputedTimingProperties GetComputedTimingAsDict() const { return GetComputedTiming().AsDict(); } }; Alternatively, there are probably a lot of things we need to set in ComputedTimingProperties that we don't need to set in ComputedTiming. I suspect the internal ComputedTiming struct really only needs to contain the information in the ComputedTimingProperties struct itself, and none of the inherited fields. So we could either extend ComputedTiming::AsDict() to take a parameter with the extra information that we need to fill in, or, probably better, just extend KeyframeEffectReadOnly::GetComputedTimingAsDict() to fill the information in.
(In reply to Brian Birtles (:birtles) from comment #33) > ... > Oh, that's tricky. We really do want to keep the StickyTimeDuration > representation and use that internally. When we return the API object, > however, we should return just the double version > (AnimationUtils::TimeDurationToDouble converts these values although we > should probably add another version that takes a StickyTimeDuration and > correctly handles Forever/infinity values). > ... > So we could either extend ComputedTiming::AsDict() to take a parameter with > the extra information that we need to fill in, or, probably better, just > extend KeyframeEffectReadOnly::GetComputedTimingAsDict() to fill the > information in. OK. Thanks for your advice. Let me try this idea.
Attachment #8667142 - Attachment is obsolete: true
Attachment #8667793 - Attachment is obsolete: true
Attachment #8670122 - Attachment is obsolete: true
Hi Brian, I have a question about some properties. I'm not sure how to get the correct "EndDelay" and "IterationStart" from ComputedTiming, AnimationTiming, KeyframeEffectReadOnly, or other Animation code. It looks like we don't have these attributes in current implementation of CSS animation, right? Should I just set a default value (ex. 0.0) for these attributes in this bug? Another question is: Is "AnimationEffectTiming" interface in the spec is the equivalent of "AnimationTiming" in KeyframeEffect.h? (http://w3c.github.io/web-animations/#animationeffecttiming) Should we make "AnimationTiming" in KeyframeEffect.h look like "AnimationEffectTiming" interface in the spec or just convert AnimationTiming into AnimationEffectTiming in other bug? Thanks.
Attachment #8670126 - Attachment is obsolete: true
Attachment #8670669 - Attachment is obsolete: true
Attachment #8670121 - Flags: feedback?(bbirtles)
Comment on attachment 8672534 [details] [diff] [review] Part 2: Combine generated dictionaries and interface (v3) Review of attachment 8672534 [details] [diff] [review]: ----------------------------------------------------------------- Hi Brian, Could you please give me some feedback about the current implementation (part 1 & part 2)? Part 1: Webidl revision. I added the new dictionary, and used "mach webidl-examples AnimationEffectReadOnly" to generate the class. Also added some keywords, ex. virtual, const, to getComputedProperties(), so KeyframeEffectReadOnly can override this function. If you agree the current implementation, this patch would be reviewed by DOM peers. Part 2: Try to merge the generated class/struct into current CSS animations/transitions. I set mEndDelay and mIterationStart to the default value (0.0f) in current implementation, and replaced the current enumerations with the new generated ones (ex. FillMode, PlaybackDirection). Thanks.
Attachment #8672534 - Flags: feedback?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #40) > Hi Brian, > > I have a question about some properties. I'm not sure how to get the correct > "EndDelay" and "IterationStart" from ComputedTiming, AnimationTiming, > KeyframeEffectReadOnly, or other Animation code. It looks like we don't have > these attributes in current implementation of CSS animation, right? Should I > just set a default value (ex. 0.0) for these attributes in this bug? Yes, these should both be 0. > Another question is: Is "AnimationEffectTiming" interface in the spec is the > equivalent of "AnimationTiming" in KeyframeEffect.h? > (http://w3c.github.io/web-animations/#animationeffecttiming) > Should we make "AnimationTiming" in KeyframeEffect.h look like > "AnimationEffectTiming" interface in the spec or just convert > AnimationTiming into AnimationEffectTiming in other bug? > Thanks. AnimationTiming is similar to AnimationEffectTiming. However, AnimationEffectTiming is an interface, i.e. it will be represented as a ref-counted object. I think we're better off leaving KeyframeEffectReadOnly.mTiming as POD that we can allocate on the stack. We use that same struct in places like SampleAnimations in AsyncCompositionManager.cpp so I think we should keep it light-weight as best possible. We might be able to merge it with the AnimationEffectTimingProperties dictionary depending on what that ends up looking like.
Comment on attachment 8670121 [details] [diff] [review] Part 1: Add ComputedTimingProperties dictionary (v3) >+AnimationEffectReadOnly::AnimationEffectReadOnly(nsISupports* aParent) >+ : mParent(aParent) >+{ >+} >+ >+nsISupports* >+AnimationEffectReadOnly::GetParentObject() const >+{ >+ return mParent; >+} Do we need to move these? >diff --git a/dom/animation/AnimationEffectReadOnly.h b/dom/animation/AnimationEffectReadOnly.h >index 9986c80..752102b 100644 >--- a/dom/animation/AnimationEffectReadOnly.h >+++ b/dom/animation/AnimationEffectReadOnly.h >@@ -1,43 +1,48 @@ >-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >-/* vim: set ts=8 sts=2 et sw=2 tw=80: */ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim:set ts=2 sw=2 sts=2 et cindent: */ I'm pretty sure this is right as-is: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line >-class AnimationEffectReadOnly >- : public nsISupports >- , public nsWrapperCache >+struct ComputedTimingProperties; >+ >+} // namespace dom >+} // namespace mozilla >+ >+namespace mozilla { >+namespace dom { No need to close and re-open the namespace blocks here. > [Func="nsDocument::IsWebAnimationsEnabled"] > interface AnimationEffectReadOnly { > // Not yet implemented: > // readonly attribute AnimationEffectTimingReadOnly timing; > // readonly attribute ComputedTimingProperties computedTiming; >+ >+ // Cannot create a dictionary attribute now, so use a getter to access it >+ [BinaryName="getComputedTimingAsDict"] >+ ComputedTimingProperties? getComputedTimingProperties(); > }; The spec is going to change here to just: ComputedTimingProperties getComputedTiming();
Attachment #8670121 - Flags: feedback?(bbirtles) → feedback+
Comment on attachment 8672534 [details] [diff] [review] Part 2: Combine generated dictionaries and interface (v3) First, bear in mind that we have bug 1208940 for moving ComputedTiming to its own file. We should do that soon. >Bug 1108055 - Part 2: Combine generated dictionaries and interface > >1. Use new generated ComputedTimingProperties in ComputedTiming. >2. Use new generated scoped enum, FillMode and PlaybackDirection, > in AnimationTiming. >3. Add new webidl method in KeyframeEffectReadOnly In future, it would be easier if these were separate patches. >diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp >index 6cd503f..1f1cd27 100644 >--- a/dom/animation/KeyframeEffect.cpp >+++ b/dom/animation/KeyframeEffect.cpp >@@ -6,20 +6,52 @@ > > #include "mozilla/dom/KeyframeEffect.h" > #include "mozilla/dom/KeyframeEffectBinding.h" > #include "mozilla/FloatingPoint.h" > #include "AnimationCommon.h" > #include "nsCSSPropertySet.h" > #include "nsCSSProps.h" // For nsCSSProps::PropHasFlags > #include "nsStyleUtil.h" >+#include <algorithm> Please add a comment saying what this is for. > void >+ComputedTiming::AsDict(Nullable<dom::ComputedTimingProperties>& aDict, >+ const Nullable<TimeDuration>& aLocalTime, >+ const AnimationTiming& aTiming) const >+{ >+ dom::ComputedTimingProperties dic; >+ >+ // AnimationEffectTimingProperties >+ // TODO: Get valid EndDelay and IterationStart from our Timing Model As discussed, this is not needed. >+ dic.mDelay = aTiming.mDelay.ToMilliseconds(); >+ dic.mEndDelay = 0.0; >+ dic.mFill = aTiming.mFillMode; >+ dic.mIterationStart = 0.0; >+ dic.mIterations = aTiming.mIterationCount; >+ dic.mDuration.SetAsUnrestrictedDouble() = aTiming.mIterationDuration.ToMilliseconds(); >+ dic.mDirection = aTiming.mDirection; >+ dic.mEasing = aTiming.mEasing; >+ >+ // ComputedTimingProperties >+ dic.mActiveDuration = mActiveDuration.ToMilliseconds(); We need to test that this handles infinity correctly. I think it probably does, however. >+ // The animation start time is currently always zero. >+ dic.mEndTime = std::max(dic.mDelay + dic.mActiveDuration + dic.mEndDelay, 0.0); >+ dic.mLocalTime = dom::AnimationUtils::TimeDurationToDouble(aLocalTime); >+ dic.mProgress = mProgress; >+ if (!dic.mProgress.IsNull()) { >+ dic.mCurrentIteration.SetValue(mCurrentIteration); >+ } >+ >+ aDict.SetValue(mozilla::Move(dic)); >+} Looking at how much external information this method brings in, it feels a bit awkward to call it AsDict(). I think this might make sense as a static method, e.g. static ComputedTimingProperties GetComputedTimingDictionary(const Nullable<TimeDuration>& aLocalTime, const AnimationTiming& aTiming, const ComputedTiming& aComputedTiming); What do you think? >@@ -189,40 +224,40 @@ KeyframeEffectReadOnly::GetComputedTimingAt( > // When we finish exactly at the end of an iteration we need to report > // the end of the final iteration and not the start of the next iteration > // so we set up a flag for that case. > bool isEndOfFinalIteration = false; > > // Get the normalized time within the active interval. > StickyTimeDuration activeTime; > if (localTime >= aTiming.mDelay + result.mActiveDuration) { >- result.mPhase = ComputedTiming::AnimationPhase_After; >+ result.mPhase = ComputedTiming::AnimationPhase::After; If possible, please split the changes to AnimationPhase etc. into separate patches. >@@ -38,26 +38,27 @@ class AnimValuesStyleRule; > * by content but for now it encapsulates just the subset of those > * parameters passed to GetPositionInIteration. > */ > struct AnimationTiming > { > TimeDuration mIterationDuration; > TimeDuration mDelay; > float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite >- uint8_t mDirection; >- uint8_t mFillMode; >+ dom::PlaybackDirection mDirection; >+ dom::FillMode mFillMode; >+ nsString mEasing; I don't think we need mEasing yet. We don't implement animation-level easing yet. For now we should just return "linear" at the point when we generate the dictionary or for the getter to AnimationEffectTimingReadOnly. >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp >index 7276641..4b213a5 100644 >--- a/gfx/layers/composite/AsyncCompositionManager.cpp >+++ b/gfx/layers/composite/AsyncCompositionManager.cpp >@@ -529,40 +529,40 @@ SampleAnimations(Layer* aLayer, TimeStamp aPoint) > } > > AnimationTiming timing; > timing.mIterationDuration = animation.duration(); > // Currently animations run on the compositor have their delay factored > // into their start time, hence the delay is effectively zero. > timing.mDelay = TimeDuration(0); > timing.mIterationCount = animation.iterationCount(); >- timing.mDirection = animation.direction(); >+ timing.mDirection = static_cast<dom::PlaybackDirection>(animation.direction()); What guarantees that this is correct? Can't we just use the same enum in both places? >@@ -280,17 +280,17 @@ CSSAnimation::HasEndEventToQueue() const > { > if (!mEffect) { > return false; > } > > bool wasActive = mPreviousPhaseOrIteration != PREVIOUS_PHASE_BEFORE && > mPreviousPhaseOrIteration != PREVIOUS_PHASE_AFTER; > bool isActive = mEffect->GetComputedTiming().mPhase == >- ComputedTiming::AnimationPhase_Active; >+ ComputedTiming::AnimationPhase::Active; > > return wasActive && !isActive; > } This method no longer exists, you might need to rebase. >@@ -690,18 +690,22 @@ nsAnimationManager::BuildAnimations(nsStyleContext* aStyleContext, > dest->SetAnimationIndex(static_cast<uint64_t>(animIdx)); > aAnimations.AppendElement(dest); > > AnimationTiming timing; > timing.mIterationDuration = > TimeDuration::FromMilliseconds(src.GetDuration()); > timing.mDelay = TimeDuration::FromMilliseconds(src.GetDelay()); > timing.mIterationCount = src.GetIterationCount(); >- timing.mDirection = src.GetDirection(); >- timing.mFillMode = src.GetFillMode(); >+ timing.mDirection = static_cast<dom::PlaybackDirection>(src.GetDirection()); >+ timing.mFillMode = static_cast<dom::FillMode>(src.GetFillMode()); Can we avoid this casting by using the same enum here too? >+ ComputedTimingFunction ctf; >+ ctf.Init(src.GetTimingFunction()); >+ ctf.AppendToString(timing.mEasing); I think we can drop this when we drop mEasing. >@@ -683,16 +683,17 @@ nsTransitionManager::ConsiderStartingTransition( > prop.mWinsInCascade = true; > > AnimationPropertySegment& segment = *prop.mSegments.AppendElement(); > segment.mFromValue = startValue; > segment.mToValue = endValue; > segment.mFromKey = 0; > segment.mToKey = 1; > segment.mTimingFunction.Init(tf); >+ segment.mTimingFunction.AppendToString(pt->Timing().mEasing); Again, we should be able to drop this too. The rest looks fine but this will be a lot easier to review if it's split up into one change per patch.
Attachment #8672534 - Flags: feedback?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #45) > Comment on attachment 8670121 [details] [diff] [review] > Part 1: Add ComputedTimingProperties dictionary (v3) > > >+AnimationEffectReadOnly::AnimationEffectReadOnly(nsISupports* aParent) > >+ : mParent(aParent) > >+{ > >+} > >+ > >+nsISupports* > >+AnimationEffectReadOnly::GetParentObject() const > >+{ > >+ return mParent; > >+} > > Do we need to move these? Actually, no. I will revert them :) > > >diff --git a/dom/animation/AnimationEffectReadOnly.h b/dom/animation/AnimationEffectReadOnly.h > >index 9986c80..752102b 100644 > >--- a/dom/animation/AnimationEffectReadOnly.h > >+++ b/dom/animation/AnimationEffectReadOnly.h > >@@ -1,43 +1,48 @@ > >-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > >-/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > >+/* vim:set ts=2 sw=2 sts=2 et cindent: */ > > I'm pretty sure this is right as-is: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Coding_Style#Mode_Line OK. I just copied the generated code from AnimationEffectReadOnly-examples.h/cpp. I will revert this part. > > >-class AnimationEffectReadOnly > >- : public nsISupports > >- , public nsWrapperCache > >+struct ComputedTimingProperties; > >+ > >+} // namespace dom > >+} // namespace mozilla > >+ > >+namespace mozilla { > >+namespace dom { > > No need to close and re-open the namespace blocks here. OK. > > > [Func="nsDocument::IsWebAnimationsEnabled"] > > interface AnimationEffectReadOnly { > > // Not yet implemented: > > // readonly attribute AnimationEffectTimingReadOnly timing; > > // readonly attribute ComputedTimingProperties computedTiming; > >+ > >+ // Cannot create a dictionary attribute now, so use a getter to access it > >+ [BinaryName="getComputedTimingAsDict"] > >+ ComputedTimingProperties? getComputedTimingProperties(); > > }; > > The spec is going to change here to just: > > ComputedTimingProperties getComputedTiming(); OK. Thanks.
See Also: → 1208940
(In reply to Brian Birtles (:birtles) from comment #46) > Comment on attachment 8672534 [details] [diff] [review] > Part 2: Combine generated dictionaries and interface (v3) > > First, bear in mind that we have bug 1208940 for moving ComputedTiming to > its own file. We should do that soon. OK. Thanks for your reminder. > > >Bug 1108055 - Part 2: Combine generated dictionaries and interface > > > >1. Use new generated ComputedTimingProperties in ComputedTiming. > >2. Use new generated scoped enum, FillMode and PlaybackDirection, > > in AnimationTiming. > >3. Add new webidl method in KeyframeEffectReadOnly > > In future, it would be easier if these were separate patches. Sure, I will separate this into different smaller patches soon. > > >diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp > >index 6cd503f..1f1cd27 100644 > >--- a/dom/animation/KeyframeEffect.cpp > >+++ b/dom/animation/KeyframeEffect.cpp > >@@ -6,20 +6,52 @@ > > #include "nsCSSProps.h" // For nsCSSProps::PropHasFlags > > #include "nsStyleUtil.h" > >+#include <algorithm> > > Please add a comment saying what this is for. OK. > > > void > >+ComputedTiming::AsDict(Nullable<dom::ComputedTimingProperties>& aDict, > >+ const Nullable<TimeDuration>& aLocalTime, > >+ const AnimationTiming& aTiming) const > >+{ > >+ // AnimationEffectTimingProperties > >+ // TODO: Get valid EndDelay and IterationStart from our Timing Model > > As discussed, this is not needed. OK. Let me remove this comment in the next patch. > > >+ dic.mDirection = aTiming.mDirection; > >+ dic.mEasing = aTiming.mEasing; > >+ > >+ // ComputedTimingProperties > >+ dic.mActiveDuration = mActiveDuration.ToMilliseconds(); > > We need to test that this handles infinity correctly. I think it probably > does, however. I checked my mochitest and set the iteration to be infinite. The activeDuraation is infinite as well. I think it can handle infinitely correctly. It's a good reminder, and I will add one more test for this case into part 3. > > Looking at how much external information this method brings in, it feels a > bit awkward to call it AsDict(). I think this might make sense as a static > method, e.g. > > static ComputedTimingProperties > GetComputedTimingDictionary(const Nullable<TimeDuration>& aLocalTime, > const AnimationTiming& aTiming, > const ComputedTiming& aComputedTiming); > > What do you think? Using it as a static function is OK to me. Or add a private function into KeyframeEffectReadOnly because only KeyframeEffectReadOnly::GetComputedTiming() use this function. ex. 1 define this static function in KeyframeEffect.cpp as you mentioned: static ComputedTimingProperties GetComputedTimingDictionary(const Nullable<TimeDuration>& aLocalTime, const AnimationTiming& aTiming, const ComputedTiming& aComputedTiming); ex. 2 private: ComputedTimingProperties KeyframeEffecrReadOnly::GetComputedTimingDictionary(...); Using a static function is more general, so I would use the idea you mentioned in my next patch. > > >@@ -189,40 +224,40 @@ KeyframeEffectReadOnly::GetComputedTimingAt( > > if (localTime >= aTiming.mDelay + result.mActiveDuration) { > >- result.mPhase = ComputedTiming::AnimationPhase_After; > >+ result.mPhase = ComputedTiming::AnimationPhase::After; > > If possible, please split the changes to AnimationPhase etc. into separate > patches. OK. This makes sense. > > >@@ -38,26 +38,27 @@ class AnimValuesStyleRule; > > struct AnimationTiming > > { > >- uint8_t mDirection; > >- uint8_t mFillMode; > >+ dom::PlaybackDirection mDirection; > >+ dom::FillMode mFillMode; > >+ nsString mEasing; > > I don't think we need mEasing yet. We don't implement animation-level easing > yet. For now we should just return "linear" at the point when we generate > the dictionary or for the getter to AnimationEffectTimingReadOnly. OK. I will use "linear" whenever returning the dictionary in this bug. > > >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp > >index 7276641..4b213a5 100644 > >--- a/gfx/layers/composite/AsyncCompositionManager.cpp > >+++ b/gfx/layers/composite/AsyncCompositionManager.cpp > >@@ -529,40 +529,40 @@ SampleAnimations(Layer* aLayer, TimeStamp aPoint) > > timing.mIterationCount = animation.iterationCount(); > >- timing.mDirection = animation.direction(); > >+ timing.mDirection = static_cast<dom::PlaybackDirection>(animation.direction()); > > What guarantees that this is correct? Can't we just use the same enum in > both places? OK. I will revert this part. I will use the original enumeration of PlaybackDirection in the next patch. > > >@@ -280,17 +280,17 @@ CSSAnimation::HasEndEventToQueue() const > > { > > ... > > } > > This method no longer exists, you might need to rebase. Sure. Thanks for your reminder. > > >@@ -690,18 +690,22 @@ nsAnimationManager::BuildAnimations(nsStyleContext* aStyleContext, > > timing.mDelay = TimeDuration::FromMilliseconds(src.GetDelay()); > > timing.mIterationCount = src.GetIterationCount(); > >- timing.mDirection = src.GetDirection(); > >- timing.mFillMode = src.GetFillMode(); > >+ timing.mDirection = static_cast<dom::PlaybackDirection>(src.GetDirection()); > >+ timing.mFillMode = static_cast<dom::FillMode>(src.GetFillMode()); > > Can we avoid this casting by using the same enum here too? Sure. We can only do casting in GetComputedTiming() because the generated enumeration from webidl is a scoped enumeration, and keep other parts the same as before. > > >+ ComputedTimingFunction ctf; > >+ ctf.Init(src.GetTimingFunction()); > >+ ctf.AppendToString(timing.mEasing); > > I think we can drop this when we drop mEasing. OK. > > >@@ -683,16 +683,17 @@ nsTransitionManager::ConsiderStartingTransition( > > prop.mWinsInCascade = true; > > > > AnimationPropertySegment& segment = *prop.mSegments.AppendElement(); > > segment.mFromValue = startValue; > > segment.mToValue = endValue; > > segment.mFromKey = 0; > > segment.mToKey = 1; > > segment.mTimingFunction.Init(tf); > >+ segment.mTimingFunction.AppendToString(pt->Timing().mEasing); > > Again, we should be able to drop this too. OK. > > > The rest looks fine but this will be a lot easier to review if it's split up > into one change per patch. Thanks for your clear explanation and feedback. I will separate this patch, so you can review this easier.
Depends on: 1208940
See Also: 1208940 →
Attachment #8670121 - Attachment is obsolete: true
Do some minor revisions in struct ComputedTiming. 1. Use Nullable<double> mProgress, so remove the static const kNullProgress. 2. Use scoped enums for AnimationPhase.
Attachment #8672534 - Attachment is obsolete: true
Attachment #8672535 - Attachment is obsolete: true
Implement KeyframeEffectReadOnly::GetComputedTiming().
Attachment #8672981 - Attachment is obsolete: true
Add test_animation-computed-timing.html in css-animations and css-transitions.
(In reply to Brian Birtles (:birtles) from comment #44) > (In reply to Boris Chiou [:boris] from comment #40) > > Hi Brian, > > > > I have a question about some properties. I'm not sure how to get the correct > > "EndDelay" and "IterationStart" from ComputedTiming, AnimationTiming, > > KeyframeEffectReadOnly, or other Animation code. It looks like we don't have > > these attributes in current implementation of CSS animation, right? Should I > > just set a default value (ex. 0.0) for these attributes in this bug? > > Yes, these should both be 0. OK. In this bug I would set these three attributes default values in the current implementation: mEndDelay = 0.0f; mIterationStart = 0.0f; mEasing = "linear"; Therefore, the mochitest wouldn't test these values according to CSS config. Only test the default values. > > > Another question is: Is "AnimationEffectTiming" interface in the spec is the > > equivalent of "AnimationTiming" in KeyframeEffect.h? > > (http://w3c.github.io/web-animations/#animationeffecttiming) > > Should we make "AnimationTiming" in KeyframeEffect.h look like > > "AnimationEffectTiming" interface in the spec or just convert > > AnimationTiming into AnimationEffectTiming in other bug? > > Thanks. > > AnimationTiming is similar to AnimationEffectTiming. However, > AnimationEffectTiming is an interface, i.e. it will be represented as a > ref-counted object. I think we're better off leaving > KeyframeEffectReadOnly.mTiming as POD that we can allocate on the stack. We > use that same struct in places like SampleAnimations in > AsyncCompositionManager.cpp so I think we should keep it light-weight as > best possible. We might be able to merge it with the > AnimationEffectTimingProperties dictionary depending on what that ends up > looking like. Great. Thanks for your explanation. Do we have a bug for merging AnimationEffectTimingProperties into AnimationTiming and a bug for implement AnimationEffectTiming interface? Actually, this is why I replaced the "uint8 mDirection" and "uint8 mFill" in "struct AnimationTiming" with the new generated scoped enums in my obsolete patches. Maybe we should use the new defined enums in the future. However, it's better to handle this in another bug. https://pastebin.mozilla.org/8849204
(In reply to Boris Chiou [:boris] from comment #48) > > >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp > > >index 7276641..4b213a5 100644 > > >--- a/gfx/layers/composite/AsyncCompositionManager.cpp > > >+++ b/gfx/layers/composite/AsyncCompositionManager.cpp > > >@@ -529,40 +529,40 @@ SampleAnimations(Layer* aLayer, TimeStamp aPoint) > > > timing.mIterationCount = animation.iterationCount(); > > >- timing.mDirection = animation.direction(); > > >+ timing.mDirection = static_cast<dom::PlaybackDirection>(animation.direction()); > > > > What guarantees that this is correct? Can't we just use the same enum in > > both places? > OK. I will revert this part. I will use the original enumeration of > PlaybackDirection in the next patch. I think it's fine to use the scoped enum. I was just wondering if we can use the scoped enum everywhere. That might be difficult, however. > > >@@ -690,18 +690,22 @@ nsAnimationManager::BuildAnimations(nsStyleContext* aStyleContext, > > > timing.mDelay = TimeDuration::FromMilliseconds(src.GetDelay()); > > > timing.mIterationCount = src.GetIterationCount(); > > >- timing.mDirection = src.GetDirection(); > > >- timing.mFillMode = src.GetFillMode(); > > >+ timing.mDirection = static_cast<dom::PlaybackDirection>(src.GetDirection()); > > >+ timing.mFillMode = static_cast<dom::FillMode>(src.GetFillMode()); > > > > Can we avoid this casting by using the same enum here too? > Sure. We can only do casting in GetComputedTiming() because the generated > enumeration from webidl is a scoped enumeration, and keep other parts the > same as before. Again, if it's possible to use the scoped enum everywhere, that would be nice.
(In reply to Boris Chiou [:boris] from comment #54) > (In reply to Brian Birtles (:birtles) from comment #44) > > (In reply to Boris Chiou [:boris] from comment #40) > > > Hi Brian, > > > > > > I have a question about some properties. I'm not sure how to get the correct > > > "EndDelay" and "IterationStart" from ComputedTiming, AnimationTiming, > > > KeyframeEffectReadOnly, or other Animation code. It looks like we don't have > > > these attributes in current implementation of CSS animation, right? Should I > > > just set a default value (ex. 0.0) for these attributes in this bug? > > > > Yes, these should both be 0. > OK. In this bug I would set these three attributes default values in the > current implementation: > mEndDelay = 0.0f; > mIterationStart = 0.0f; > mEasing = "linear"; When we are returning a dictionary, I think the Init method does this automatically? > Therefore, the mochitest wouldn't test these values according to CSS config. > Only test the default values. It's not possible to set these values from CSS anyway. (CSS can only set mEasing on a keyframe, not on the animation. And there are no CSS properties corresponding to "end delay" or "iteration start"). > > > Another question is: Is "AnimationEffectTiming" interface in the spec is the > > > equivalent of "AnimationTiming" in KeyframeEffect.h? > > > (http://w3c.github.io/web-animations/#animationeffecttiming) > > > Should we make "AnimationTiming" in KeyframeEffect.h look like > > > "AnimationEffectTiming" interface in the spec or just convert > > > AnimationTiming into AnimationEffectTiming in other bug? > > > Thanks. > > > > AnimationTiming is similar to AnimationEffectTiming. However, > > AnimationEffectTiming is an interface, i.e. it will be represented as a > > ref-counted object. I think we're better off leaving > > KeyframeEffectReadOnly.mTiming as POD that we can allocate on the stack. We > > use that same struct in places like SampleAnimations in > > AsyncCompositionManager.cpp so I think we should keep it light-weight as > > best possible. We might be able to merge it with the > > AnimationEffectTimingProperties dictionary depending on what that ends up > > looking like. > > Great. Thanks for your explanation. Do we have a bug for merging > AnimationEffectTimingProperties into AnimationTiming and a bug for implement > AnimationEffectTiming interface? No. Unfortunately the code in KeyframeEffect.{h,cpp} is quite disorganized. It was moved from layout/style without changing it very much so we haven't organized it very well yet. Feel free to file extra bugs as you see fit. > Actually, this is why I replaced the "uint8 mDirection" and "uint8 mFill" in > "struct AnimationTiming" with the new generated scoped enums in my obsolete > patches. Maybe we should use the new defined enums in the future. However, > it's better to handle this in another bug. > https://pastebin.mozilla.org/8849204 Yes, it would be good to use those scoped enums wherever possible.
(In reply to Brian Birtles (:birtles) from comment #55) > (In reply to Boris Chiou [:boris] from comment #48) > > > >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp > > > >index 7276641..4b213a5 100644 > > > >--- a/gfx/layers/composite/AsyncCompositionManager.cpp > > > >+++ b/gfx/layers/composite/AsyncCompositionManager.cpp > > > >@@ -529,40 +529,40 @@ SampleAnimations(Layer* aLayer, TimeStamp aPoint) > > > > timing.mIterationCount = animation.iterationCount(); > > > >- timing.mDirection = animation.direction(); > > > >+ timing.mDirection = static_cast<dom::PlaybackDirection>(animation.direction()); > > > > > > What guarantees that this is correct? Can't we just use the same enum in > > > both places? > > OK. I will revert this part. I will use the original enumeration of > > PlaybackDirection in the next patch. > > I think it's fine to use the scoped enum. I was just wondering if we can use > the scoped enum everywhere. That might be difficult, however. > > > > >@@ -690,18 +690,22 @@ nsAnimationManager::BuildAnimations(nsStyleContext* aStyleContext, > > > > timing.mDelay = TimeDuration::FromMilliseconds(src.GetDelay()); > > > > timing.mIterationCount = src.GetIterationCount(); > > > >- timing.mDirection = src.GetDirection(); > > > >- timing.mFillMode = src.GetFillMode(); > > > >+ timing.mDirection = static_cast<dom::PlaybackDirection>(src.GetDirection()); > > > >+ timing.mFillMode = static_cast<dom::FillMode>(src.GetFillMode()); > > > > > > Can we avoid this casting by using the same enum here too? > > Sure. We can only do casting in GetComputedTiming() because the generated > > enumeration from webidl is a scoped enumeration, and keep other parts the > > same as before. > > Again, if it's possible to use the scoped enum everywhere, that would be > nice. AnimationEffectTiming interface and AnimationEffectTimingProperties dictionary also use the scoped enum. I think we can try to use the new scoped enum in that bug, so I would still do casting only when generating the dictionary here. Thanks.
(In reply to Brian Birtles (:birtles) from comment #56) > (In reply to Boris Chiou [:boris] from comment #54) > > (In reply to Brian Birtles (:birtles) from comment #44) > > > (In reply to Boris Chiou [:boris] from comment #40) > > > > Hi Brian, > > > > > > > > I have a question about some properties. I'm not sure how to get the correct > > > > "EndDelay" and "IterationStart" from ComputedTiming, AnimationTiming, > > > > KeyframeEffectReadOnly, or other Animation code. It looks like we don't have > > > > these attributes in current implementation of CSS animation, right? Should I > > > > just set a default value (ex. 0.0) for these attributes in this bug? > > > > > > Yes, these should both be 0. > > OK. In this bug I would set these three attributes default values in the > > current implementation: > > mEndDelay = 0.0f; > > mIterationStart = 0.0f; > > mEasing = "linear"; > > When we are returning a dictionary, I think the Init method does this > automatically? > > > Therefore, the mochitest wouldn't test these values according to CSS config. > > Only test the default values. > > It's not possible to set these values from CSS anyway. (CSS can only set > mEasing on a keyframe, not on the animation. And there are no CSS properties > corresponding to "end delay" or "iteration start"). Yes. Sorry for my confused comment. The init values are set automatically, so I wouldn't put these assignments. > > > > > Another question is: Is "AnimationEffectTiming" interface in the spec is the > > > > equivalent of "AnimationTiming" in KeyframeEffect.h? > > > > (http://w3c.github.io/web-animations/#animationeffecttiming) > > > > Should we make "AnimationTiming" in KeyframeEffect.h look like > > > > "AnimationEffectTiming" interface in the spec or just convert > > > > AnimationTiming into AnimationEffectTiming in other bug? > > > > Thanks. > > > > > > AnimationTiming is similar to AnimationEffectTiming. However, > > > AnimationEffectTiming is an interface, i.e. it will be represented as a > > > ref-counted object. I think we're better off leaving > > > KeyframeEffectReadOnly.mTiming as POD that we can allocate on the stack. We > > > use that same struct in places like SampleAnimations in > > > AsyncCompositionManager.cpp so I think we should keep it light-weight as > > > best possible. We might be able to merge it with the > > > AnimationEffectTimingProperties dictionary depending on what that ends up > > > looking like. > > > > Great. Thanks for your explanation. Do we have a bug for merging > > AnimationEffectTimingProperties into AnimationTiming and a bug for implement > > AnimationEffectTiming interface? > > No. Unfortunately the code in KeyframeEffect.{h,cpp} is quite disorganized. > It was moved from layout/style without changing it very much so we haven't > organized it very well yet. Feel free to file extra bugs as you see fit. > > > Actually, this is why I replaced the "uint8 mDirection" and "uint8 mFill" in > > "struct AnimationTiming" with the new generated scoped enums in my obsolete > > patches. Maybe we should use the new defined enums in the future. However, > > it's better to handle this in another bug. > > https://pastebin.mozilla.org/8849204 > > Yes, it would be good to use those scoped enums wherever possible. Great. I should file another bug to handle this case. Thanks. BTW, should I wait for Bug 1208940 to move ComputedTimingFunction in a new file? I think my new patches are ready to review again. Thanks.
(In reply to Boris Chiou [:boris] from comment #58) > BTW, should I wait for Bug 1208940 to move ComputedTimingFunction in a new > file? I think my new patches are ready to review again. Thanks. No, no one is working on bug 1208940 currently (you are welcome to take it!) so you should not wait for it. However, I think you should wait for bug 1208951 to land before landing this.
(In reply to Brian Birtles (:birtles) from comment #59) > (In reply to Boris Chiou [:boris] from comment #58) > > BTW, should I wait for Bug 1208940 to move ComputedTimingFunction in a new > > file? I think my new patches are ready to review again. Thanks. > > No, no one is working on bug 1208940 currently (you are welcome to take it!) > so you should not wait for it. However, I think you should wait for bug > 1208951 to land before landing this. OK. After finish this bug, I will take it. Thanks.
Add two dictionaries into AnimationEffectReadOnly.webidl: 1. AnimationEffectTimingProperties 2. ComputedTimingProperties And then re-generate this class. Update AnimationEffectReadOnly according to the new spec version.
Attachment #8672993 - Attachment is obsolete: true
Attachment #8673475 - Flags: review?(bugs)
Attachment #8672983 - Flags: review?(bbirtles)
Blocks: 1214536
Comment on attachment 8672983 [details] [diff] [review] Part 2: Refine ComputedTiming (v4) >Bug 1108055 - Part 2: Refine ComputedTiming > >Do some minor revisions in struct ComputedTiming. >1. Use Nullable<double> mProgress, so remove the static const kNullProgress. >2. Use scoped enums for AnimationPhase. We should probably mention that the purpose of this is to better line up with the generated type for the ComputedTimingProperties dictionary. >diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h > #include "mozilla/StickyTimeDuration.h" > #include "mozilla/StyleAnimationValue.h" > #include "mozilla/TimeStamp.h" > #include "mozilla/dom/AnimationEffectReadOnly.h" >+#include "mozilla/dom/AnimationEffectReadOnlyBinding.h" > #include "mozilla/dom/Element.h" > #include "mozilla/dom/KeyframeBinding.h" >-#include "mozilla/dom/Nullable.h" > #include "nsSMILKeySpline.h" > #include "nsStyleStruct.h" // for nsTimingFunction Why are we removing Nullable.h? I'm pretty sure we still use the Nullable type? (Even if AnimationEffectReadOnlyBinding.h also happens to include Nullable.h we should still include it directly here.) Also, it would be useful to indicate why we are including AnimationEffectReadOnlyBinding.h (i.e. what types are we using from there). > struct ComputedTiming > { >- ComputedTiming() >- : mProgress(kNullProgress) >- , mCurrentIteration(0) >- , mPhase(AnimationPhase_Null) >- { } >+ enum class AnimationPhase { >+ Null, // Not sampled (null sample time) >+ Before, // Sampled prior to the start of the active interval >+ Active, // Sampled within the active interval >+ After // Sampled after (or at) the end of the active interval >+ }; This is good. Any reason to put it at the top of the struct definition? If not, I think putting it next where it is used would make more sense. >+ ComputedTiming() = default; Why is this needed? > // Progress towards the end of the current iteration. If the effect is > // being sampled backwards, this will go from 1.0 to 0.0. > // Will be kNullProgress if the animation is neither animating nor > // filling at the sampled time. >- double mProgress; >- >+ Nullable<double> mProgress; The comment here needs to be updated to say "Will be null if the ..." > // Zero-based iteration index (meaningless if mProgress is kNullProgress). >- uint64_t mCurrentIteration; This comment needs to be updated too. > class ComputedTimingFunction > { > public: > typedef nsTimingFunction::Type Type; > typedef nsTimingFunction::StepSyntax StepSyntax; > void Init(const nsTimingFunction &aFunction); >@@ -268,18 +257,19 @@ public: > // member of the return value if the animation should not be run > // (because it is not currently active and is not filling at this time). > static ComputedTiming > GetComputedTimingAt(const Nullable<TimeDuration>& aLocalTime, > const AnimationTiming& aTiming); We need to update the comment for this method since it refers to kNullProgress (although that line is not shown in this diff). >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp >index 96f1834..624c71f 100644 >--- a/gfx/layers/composite/AsyncCompositionManager.cpp >+++ b/gfx/layers/composite/AsyncCompositionManager.cpp >@@ -578,29 +578,29 @@ SampleAnimations(Layer* aLayer, TimeStamp aPoint) > // example, while they are waiting to be removed) we currently just > // assume that we should fill. > timing.mFillMode = NS_STYLE_ANIMATION_FILL_MODE_BOTH; > > ComputedTiming computedTiming = > dom::KeyframeEffectReadOnly::GetComputedTimingAt( > Nullable<TimeDuration>(elapsedDuration), timing); > >- MOZ_ASSERT(0.0 <= computedTiming.mProgress && >- computedTiming.mProgress <= 1.0, >+ MOZ_ASSERT(0.0 <= computedTiming.mProgress.Value() && >+ computedTiming.mProgress.Value() <= 1.0, > "iteration progress should be in [0-1]"); We should make this assertion check for null progress too, i.e. !computedTiming.mProgress.IsNull() && 0.0 <= computedTiming.mProgress.Value() && computedTiming.mProgress.Value() <= 1.0 (Previously, when null progress was defined as +Infinity this assertion would have caught that case too, but now that we use a nullable it won't so we need to explicitly check for it.)
Attachment #8672983 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #62) > Comment on attachment 8672983 [details] [diff] [review] > Part 2: Refine ComputedTiming (v4) > > >Bug 1108055 - Part 2: Refine ComputedTiming > > > >Do some minor revisions in struct ComputedTiming. > >1. Use Nullable<double> mProgress, so remove the static const kNullProgress. > >2. Use scoped enums for AnimationPhase. > > We should probably mention that the purpose of this is to better line up with > the generated type for the ComputedTimingProperties dictionary. OK. I will add more comments to mention the purpose. > > >diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h > >+#include "mozilla/dom/AnimationEffectReadOnlyBinding.h" > >-#include "mozilla/dom/Nullable.h" > > Why are we removing Nullable.h? I'm pretty sure we still use the Nullable > type? (Even if AnimationEffectReadOnlyBinding.h also happens to include > Nullable.h we should still include it directly here.) > > Also, it would be useful to indicate why we are including > AnimationEffectReadOnlyBinding.h (i.e. what types are we using from there). Yes, AnimationEffectReadOnlyBinding.h also includes Nullable.h, so I removed Nullable.h in this patch. You're right, I should include Nullable direcely here. > > > struct ComputedTiming > > { > >+ enum class AnimationPhase { > >+ Null, // Not sampled (null sample time) > >+ Before, // Sampled prior to the start of the active interval > >+ Active, // Sampled within the active interval > >+ After // Sampled after (or at) the end of the active interval > >+ }; > > This is good. Any reason to put it at the top of the struct definition? > If not, I think putting it next where it is used would make more sense. I put it at the top because it is a type definition and other h/cpp files may use it, so my habit is to put it at the top. But I think you are right. It's better to put it just above the first usage in this class/struct. Thanks. > > >+ ComputedTiming() = default; > > Why is this needed? I should remove this because we could just use the generated constructor now. Thanks. > > > // Progress towards the end of the current iteration. If the effect is > > // being sampled backwards, this will go from 1.0 to 0.0. > > // Will be kNullProgress if the animation is neither animating nor > > // filling at the sampled time. > >- double mProgress; > >- > >+ Nullable<double> mProgress; > > The comment here needs to be updated to say "Will be null if the ..." > > > // Zero-based iteration index (meaningless if mProgress is kNullProgress). > >- uint64_t mCurrentIteration; > > This comment needs to be updated too. > > >@@ -268,18 +257,19 @@ public: > > // member of the return value if the animation should not be run > > // (because it is not currently active and is not filling at this time). > > static ComputedTiming > > GetComputedTimingAt(const Nullable<TimeDuration>& aLocalTime, > > const AnimationTiming& aTiming); > > We need to update the comment for this method since it refers to > kNullProgress (although that line is not shown in this diff). OK. I will update these comments. > > >diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp > >index 96f1834..624c71f 100644 > >--- a/gfx/layers/composite/AsyncCompositionManager.cpp > >+++ b/gfx/layers/composite/AsyncCompositionManager.cpp > >@@ -578,29 +578,29 @@ SampleAnimations(Layer* aLayer, TimeStamp aPoint) > > > >- MOZ_ASSERT(0.0 <= computedTiming.mProgress && > >- computedTiming.mProgress <= 1.0, > >+ MOZ_ASSERT(0.0 <= computedTiming.mProgress.Value() && > >+ computedTiming.mProgress.Value() <= 1.0, > > "iteration progress should be in [0-1]"); > > We should make this assertion check for null progress too, i.e. > > !computedTiming.mProgress.IsNull() && > 0.0 <= computedTiming.mProgress.Value() && > computedTiming.mProgress.Value() <= 1.0 > > (Previously, when null progress was defined as +Infinity this assertion would > have caught that case too, but now that we use a nullable it won't so we need > to explicitly check for it.) I should re-check these assertions and add one more check for nullable. Thanks for your review.
Do some minor revisions in struct ComputedTiming. 1. Use Nullable<double> mProgress, so remove the static const kNullProgress. The generated ComputedTimingProperties dictionary uses "Nullable" variable, so we replace the origin type in ComputedTiming to make it more consistent with that in ComputedTimingProperties dictionary. 2. Use scoped enums for AnimationPhase.
Attachment #8674063 - Flags: review+
Attachment #8672983 - Attachment is obsolete: true
Implement KeyframeEffectReadOnly::GetComputedTiming().
Attachment #8672984 - Attachment is obsolete: true
Comment on attachment 8674064 [details] [diff] [review] Part 3: Implement GetComputedTiming method (v5) Review of attachment 8674064 [details] [diff] [review]: ----------------------------------------------------------------- Note: Use a static function to generate a ComputedTimingProperties dictionary from current ComputedTiming and AnimationTiming objects; therefore, KeyframeEffectReadOnly::GetComputedTiming() can use this helper function to return the current ComputedTimingProperties dictionary.
Attachment #8674064 - Flags: review?(bbirtles)
Comment on attachment 8674064 [details] [diff] [review] Part 3: Implement GetComputedTiming method (v5) Review of attachment 8674064 [details] [diff] [review]: ----------------------------------------------------------------- It looks mostly fine except for the enum casting. We should do something safer there. ::: dom/animation/KeyframeEffect.cpp @@ +19,5 @@ > +static void > +GetComputedTimingDictionary(dom::ComputedTimingProperties& aRetVal, > + const ComputedTiming& aComputedTiming, > + const Nullable<TimeDuration>& aLocalTime, > + const AnimationTiming& aTiming) I'm curious why you went with an out parameter instead of using the return value. Is Init() on a dictionary expensive? If so, this is fine, but I think it would make sense to list the input (const) params first? @@ +23,5 @@ > + const AnimationTiming& aTiming) > +{ > + // AnimationEffectTimingProperties > + aRetVal.mDelay = aTiming.mDelay.ToMilliseconds(); > + aRetVal.mFill = static_cast<dom::FillMode>(aTiming.mFillMode); Casting from a uint8 to an enum type here is unsafe. If someone updates the values on either side we'd have a mismatch and run into potentially nasty bugs. We really should convert AnimationTiming.mFillMode to the same type first. Is there a bug for doing this? As an interim step, I'd rather have a function that maps the values across one-by-one and asserts if it encounters a value we don't know about. @@ +26,5 @@ > + aRetVal.mDelay = aTiming.mDelay.ToMilliseconds(); > + aRetVal.mFill = static_cast<dom::FillMode>(aTiming.mFillMode); > + aRetVal.mIterations = aTiming.mIterationCount; > + aRetVal.mDuration.SetAsUnrestrictedDouble() = aTiming.mIterationDuration.ToMilliseconds(); > + aRetVal.mDirection = static_cast<dom::PlaybackDirection>(aTiming.mDirection); Likewise here. @@ +36,5 @@ > + aRetVal.mLocalTime = dom::AnimationUtils::TimeDurationToDouble(aLocalTime); > + aRetVal.mProgress = aComputedTiming.mProgress; > + if (!aRetVal.mProgress.IsNull()) { > + aRetVal.mCurrentIteration.SetValue(aComputedTiming.mCurrentIteration); > + } There's a FIXME comment in KeyframeEffectReadOnly::GetComputedTimingAt saying that we need to turn an mCurrentIteration of UINT64_MAX into positive infinity at the API. We should probably handle that here and update the comment to say where that is handled. Please add a test to verify this as well. ::: dom/animation/KeyframeEffect.h @@ +267,5 @@ > return GetComputedTimingAt(GetLocalTime(), aTiming ? *aTiming : mTiming); > } > > + virtual void > + GetComputedTimingAsDict(ComputedTimingProperties& aRetVal) const override; I think we decided to drop 'virtual' whenever we also specify 'override'. "Method declarations must use at most one of the following keywords: virtual, override, or final." See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods There's still a lot of old code that uses both, but we will gradually convert it to this new style.
Attachment #8674064 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #67) > Comment on attachment 8674064 [details] [diff] [review] > Part 3: Implement GetComputedTiming method (v5) > > Review of attachment 8674064 [details] [diff] [review]: > ----------------------------------------------------------------- > > It looks mostly fine except for the enum casting. We should do something > safer there. > > ::: dom/animation/KeyframeEffect.cpp > @@ +19,5 @@ > > +static void > > +GetComputedTimingDictionary(dom::ComputedTimingProperties& aRetVal, > > + const ComputedTiming& aComputedTiming, > > + const Nullable<TimeDuration>& aLocalTime, > > + const AnimationTiming& aTiming) > > I'm curious why you went with an out parameter instead of using the return > value. Is Init() on a dictionary expensive? If so, this is fine, but I think > it would make sense to list the input (const) params first? I understand your question. I also want to use the return value, but I got this compilation error: ~/gecko/dom/animation/KeyframeEffect.cpp:41:10: error: no matching constructor for initialization of 'dom::ComputedTimingProperties' ../../objdirs/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h:125:3: note: candidate constructor not viable: requires 0 arguments, but 1 was provided ComputedTimingProperties(); ^ 1 error generated. The generated ComputedTimingProperties: https://pastebin.mozilla.org/8848039 The copy constructor is "explicit", so I think I cannot return a dom::ComputedTimingProperties object because the compiler wanna copy the return object but cannot find a suitable constructor. Therefore, I use a parameter to handle the return value. However, I should put the output parameter last. Thanks. > > @@ +23,5 @@ > > + const AnimationTiming& aTiming) > > +{ > > + // AnimationEffectTimingProperties > > + aRetVal.mDelay = aTiming.mDelay.ToMilliseconds(); > > + aRetVal.mFill = static_cast<dom::FillMode>(aTiming.mFillMode); > > Casting from a uint8 to an enum type here is unsafe. If someone updates the > values on either side we'd have a mismatch and run into potentially nasty > bugs. We really should convert AnimationTiming.mFillMode to the same type > first. Is there a bug for doing this? > > As an interim step, I'd rather have a function that maps the values across > one-by-one and asserts if it encounters a value we don't know about. Actually, I want to handle this problem in Bug 1214536 (convert AnimationTiming.mFillMode and mDirection to the same type). In this bug, we can add a special function to check the mapping, as you mentioned. > > @@ +26,5 @@ > > + aRetVal.mDelay = aTiming.mDelay.ToMilliseconds(); > > + aRetVal.mFill = static_cast<dom::FillMode>(aTiming.mFillMode); > > + aRetVal.mIterations = aTiming.mIterationCount; > > + aRetVal.mDuration.SetAsUnrestrictedDouble() = aTiming.mIterationDuration.ToMilliseconds(); > > + aRetVal.mDirection = static_cast<dom::PlaybackDirection>(aTiming.mDirection); > > Likewise here. > > @@ +36,5 @@ > > + aRetVal.mLocalTime = dom::AnimationUtils::TimeDurationToDouble(aLocalTime); > > + aRetVal.mProgress = aComputedTiming.mProgress; > > + if (!aRetVal.mProgress.IsNull()) { > > + aRetVal.mCurrentIteration.SetValue(aComputedTiming.mCurrentIteration); > > + } > > There's a FIXME comment in KeyframeEffectReadOnly::GetComputedTimingAt > saying that we need to turn an mCurrentIteration of UINT64_MAX into positive > infinity at the API. We should probably handle that here and update the > comment to say where that is handled. OK. So I have to check if mCurrentIteration == UINT64_MAX, and then set aDict.mCurrentIteration = NS_IEEEPositiveInfinity(), and turn this return member into positive Infinty, right? I will also add a comment to explain this. > > Please add a test to verify this as well. Sure. > > ::: dom/animation/KeyframeEffect.h > @@ +267,5 @@ > > return GetComputedTimingAt(GetLocalTime(), aTiming ? *aTiming : mTiming); > > } > > > > + virtual void > > + GetComputedTimingAsDict(ComputedTimingProperties& aRetVal) const override; > > I think we decided to drop 'virtual' whenever we also specify 'override'. > > "Method declarations must use at most one of the following keywords: > virtual, override, or final." > See: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Coding_Style#Methods > > There's still a lot of old code that uses both, but we will gradually > convert it to this new style. Sorry, I didn't notice this coding style. It makes sense, and I should remove 'virtual'. Thanks.
Attachment #8674064 - Attachment is obsolete: true
Comment on attachment 8673475 [details] [diff] [review] Part 1: Add ComputedTimingProperties dictionary (v6) >+dictionary ComputedTimingProperties : AnimationEffectTimingProperties { >+ unrestricted double endTime = 0.0; >+ unrestricted double activeDuration = 0.0; >+ double? localTime = null; >+ unrestricted double? progress = null; >+ unrestricted double? currentIteration = null; >+}; The spec doesn't have default values for these. Why do we need to have? The implementation of GetComputedTimingDictionary does not seem to set the value of currentIteration always, so it does matter whether there is default value or not. (whether currentIteration is null or undefined). But then, the spec has separate text "As with unresolved times, an unresolved current iteration is represented by a null value." So I guess default values are fine here.
Attachment #8673475 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #70) > Comment on attachment 8673475 [details] [diff] [review] > Part 1: Add ComputedTimingProperties dictionary (v6) > >+dictionary ComputedTimingProperties : AnimationEffectTimingProperties { > >+ unrestricted double endTime = 0.0; > >+ unrestricted double activeDuration = 0.0; > >+ double? localTime = null; > >+ unrestricted double? progress = null; > >+ unrestricted double? currentIteration = null; > >+}; > The spec doesn't have default values for these. Why do we need to have? > > The implementation of GetComputedTimingDictionary does not seem to set the > value of > currentIteration always, so it does matter whether there is default value or > not. > (whether currentIteration is null or undefined). > But then, the spec has separate text "As with unresolved times, an > unresolved current iteration is represented by a null value." > > > So I guess default values are fine here. Hi Brian, As smaug said, the spec doesn't have the default values for these, but I added them in this patch because I saw that Comment 12 said it might be better to make up some defaults for these values. According to the current implementation, I can always assign some values (double or null) to these members in GetComputedTimingProperties(). Do you have any suggestion? Or removing these default values in this webidl file is also acceptible to you. Thanks.
I think it is ok to keep the defaults, but from reviewing point of view making our .webidl as similar to the spec as possible is always good.
(In reply to Olli Pettay [:smaug] from comment #72) > I think it is ok to keep the defaults, but from reviewing point of view > making our .webidl as similar to the spec as possible is always good. OK. Got it. Thanks for your explanation and review. :)
Attachment #8674164 - Attachment is obsolete: true
Comment on attachment 8674657 [details] [diff] [review] Part 3: Implement GetComputedTiming method (v7) Review of attachment 8674657 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/KeyframeEffect.cpp @@ +49,5 @@ > + default: > + MOZ_ASSERT(false, "The mapping of PlaybackDirection is not correct"); > + return dom::PlaybackDirection::Normal; > + } > +} Add two functions to convert FillMode and PlaybackDirection to scoped enums. After replacing the AnimationTiming.mFillMode and Animation.mDirection with the new types (in Bug 1214536), we could remove these two function. @@ +75,5 @@ > + // Convert the returned currentIteration into Infinity if we set > + // (uint64_t) aComputedTiming.mCurrentIteration to UINT64_MAX > + double iteration = aComputedTiming.mCurrentIteration == UINT64_MAX > + ? NS_IEEEPositiveInfinity() > + : static_cast<double>(aComputedTiming.mCurrentIteration); I am trying to add a test for this case. However, I have a question about how to reach the condition of mCurrentIteration == UINT64_MAX. e.g. I set the Animation: 'AnyAnimation 10s linear 0s infinite' which has infinite iteration, I don't know how to go to the "End Iteration" (isEndOfFinalIteration == true) because I cannot just call animation.finish() or set currentTime = Infinity. These statements would throw exceptions in mochitest. Do you have any method to reach this condition?
Attachment #8674657 - Flags: review?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #75) > @@ +75,5 @@ > > + // Convert the returned currentIteration into Infinity if we set > > + // (uint64_t) aComputedTiming.mCurrentIteration to UINT64_MAX > > + double iteration = aComputedTiming.mCurrentIteration == UINT64_MAX > > + ? NS_IEEEPositiveInfinity() > > + : static_cast<double>(aComputedTiming.mCurrentIteration); > > I am trying to add a test for this case. However, I have a question about > how to reach the condition of mCurrentIteration == UINT64_MAX. > e.g. > I set the Animation: 'AnyAnimation 10s linear 0s infinite' which has > infinite iteration, I don't know how to go to the "End Iteration" > (isEndOfFinalIteration == true) because I cannot just call > animation.finish() or set currentTime = Infinity. These statements would > throw exceptions in mochitest. Do you have any method to reach this > condition? Yes, you need to use a duration of zero and an iteration count of infinity.
(In reply to Brian Birtles (:birtles) from comment #76) > Yes, you need to use a duration of zero and an iteration count of infinity. Cool. This is really helpful. I got an infinite currentIteration by using a duration of zero, an iteration count of infinity and a direction of forwards.
Comment on attachment 8674657 [details] [diff] [review] Part 3: Implement GetComputedTiming method (v7) Review of attachment 8674657 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks for adding those enum conversion methods. r=birtles with comments addressed. ::: dom/animation/KeyframeEffect.cpp @@ +67,5 @@ > + > + // ComputedTimingProperties > + aRetVal.mActiveDuration = aComputedTiming.mActiveDuration.ToMilliseconds(); > + aRetVal.mEndTime > + = std::max(aRetVal.mDelay + aRetVal.mActiveDuration + aRetVal.mEndDelay, 0.0); Can you add a test that this does the right thing when mActiveDuration is infinity. I.e. set a negative/positive delay and an infinite iteration-count and check that the end time is infinity. @@ +74,5 @@ > + if (!aRetVal.mProgress.IsNull()) { > + // Convert the returned currentIteration into Infinity if we set > + // (uint64_t) aComputedTiming.mCurrentIteration to UINT64_MAX > + double iteration = aComputedTiming.mCurrentIteration == UINT64_MAX > + ? NS_IEEEPositiveInfinity() We should use PositiveInfinity<double>() here from mozilla/FloatingPoint.h.
Attachment #8674657 - Flags: review?(bbirtles) → review+
Comment on attachment 8674657 [details] [diff] [review] Part 3: Implement GetComputedTiming method (v7) Review of attachment 8674657 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your review. I will update this patch and the test cases. ::: dom/animation/KeyframeEffect.cpp @@ +67,5 @@ > + > + // ComputedTimingProperties > + aRetVal.mActiveDuration = aComputedTiming.mActiveDuration.ToMilliseconds(); > + aRetVal.mEndTime > + = std::max(aRetVal.mDelay + aRetVal.mActiveDuration + aRetVal.mEndDelay, 0.0); Sure. I just add a test for this locally and I got a infinite end time. @@ +74,5 @@ > + if (!aRetVal.mProgress.IsNull()) { > + // Convert the returned currentIteration into Infinity if we set > + // (uint64_t) aComputedTiming.mCurrentIteration to UINT64_MAX > + double iteration = aComputedTiming.mCurrentIteration == UINT64_MAX > + ? NS_IEEEPositiveInfinity() Got it.
Attachment #8674657 - Attachment is obsolete: true
Add test_animation-computed-timing.html in css-animations and css-transitions.
Attachment #8673018 - Attachment is obsolete: true
Do some minor revisions in struct ComputedTiming. 1. Use Nullable<double> mProgress, so remove the static const kNullProgress. The generated ComputedTimingProperties dictionary uses "Nullable" variable, so we replace the origin type in ComputedTiming to make it more consistent with that in ComputedTimingProperties dictionary. 2. Use scoped enums for AnimationPhase. Rebased.
Attachment #8674063 - Attachment is obsolete: true
Implement KeyframeEffectReadOnly::GetComputedTiming(). Rebase and fix try server build errors on linux.
Attachment #8674725 - Attachment is obsolete: true
Attachment #8675519 - Flags: review+
Attachment #8675520 - Flags: review+
Attachment #8674766 - Attachment is obsolete: true
Blocks: 1208940
Depends on: 1208951
No longer depends on: 1208940
Comment on attachment 8675542 [details] [diff] [review] Part 4: Add ComputedTiming mochitests (v6) Thanks so much for doing this. I'm really sorry but I think we should take a different approach here. I think it would be better to test each property individually. For example: * tests for localTime: test it is initially zero, test is always equal to current time (currently anyway, once Cameron's patch for KeyframeReadOnly constructor lands we need to add a test for when there is no associated animation), test that it immediately reflects an update to playbackRate * tests for currentIteration: test it is initially zero, test when there's no animation, test when iteration count = infinity and duration = 0 (both before and after starting), basically test all the branches of http://w3c.github.io/web-animations/#current-iteration * etc. I think that's going to lead to better tests since we can test one algorithm at a time. I also think it will make for much shorter and simpler tests. For example, test(function(t) { var div = addDiv(t); div.style.animation = 'anim 100s'; var anim = div.getAnimations()[0]; assert_equals(anim.effect.getComputedTiming().currentIteration, 0, 'Initial value of currentIteration'); }, 'currentIteration of a new animation is zero'); test(function(t) { var div = addDiv(t); div.style.animation = 'anim 0s infinite'; var anim = div.getAnimations()[0]; assert_equals(anim.effect.getComputedTiming().currentIteration, Infinity, 'Initial value of currentIteration count (after phase)'); // Seek backwards anim.currentTime -= 1000; assert_equals(anim.effect.getComputedTiming().currentIteration, 0, 'Value of currentIteration count during before phase'); }, 'currentIteration of an infinitely repeating zero-duration animation'); // Add tests that we apply floor correctly, e.g. an iteration-count of 1.5 // should mean we see currentIteration = 0; then after calling finish() it // should be come 1 // etc. // etc. // After bug 1208951 lands: test(function(t) { var effect = new KeyframeEffectReadOnly({ left: [ '0px', '100px' ] }, 2000); assert_equals(effect.getComputedTiming().currentIteration, null, 'currentIteration for orphaned effect'); }, 'currentIteration of an AnimationEffect without an Animation'); What do you think about reworking these tests to cover one property at a time?
Flags: needinfo?(boris.chiou)
For what it's worth, ideally I'd like to write these tests now. However, if you find it's too much work and you want to land this soon (e.g. so it doesn't block bug 1208940), then I think it would be ok to do additional tests in a follow-up bug. The main thing I'd like to be sure we test correctly before landing is the handling of infinite values.
(In reply to Brian Birtles (:birtles) from comment #87) > What do you think about reworking these tests to cover one property at a > time? Sure, I can re-organize my original patch to test each property individually, and then re-send a feedback request to you to make sure most cases are covered.
Flags: needinfo?(boris.chiou)
Attachment #8675542 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #88) > For what it's worth, ideally I'd like to write these tests now. However, if > you find it's too much work and you want to land this soon (e.g. so it > doesn't block bug 1208940), then I think it would be ok to do additional > tests in a follow-up bug. The main thing I'd like to be sure we test > correctly before landing is the handling of infinite values. I agree. It makes sense. I think it's better to write these test cases in this bug. :)
Attachment #8675542 - Attachment is obsolete: true
Add test_animation-computed-timing.html in css-animations and css-transitions.
Attachment #8676137 - Attachment is obsolete: true
Comment on attachment 8677334 [details] [diff] [review] Part 4: Add ComputedTiming mochitests (v8) Review of attachment 8677334 [details] [diff] [review]: ----------------------------------------------------------------- I will revise the FIXME part after Bug 1208951 lands.
Attachment #8677334 - Flags: feedback?(bbirtles)
Comment on attachment 8677334 [details] [diff] [review] Part 4: Add ComputedTiming mochitests (v8) Review of attachment 8677334 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-animations/file_animation-computed-timing.html @@ +588,5 @@ > + 'currentIteration for orphaned effect'); > +}, 'currentIteration of an AnimationEffect without an Animation');*/ > + > +// TODO: If iteration duration is Infinity, currentIteration is 0. > +// However, we cannot set iteration duration to Infinity in CSS Animation now. Actually, I am not sure how to test this case. The spec mentions this case, so I add a comment for it. "If the iteration duration is infinity, the result of floor(scaled active time / iteration duration) will be zero as defined by IEEE 754-2008." from https://w3c.github.io/web-animations/#calculating-the-current-iteration
Comment on attachment 8677334 [details] [diff] [review] Part 4: Add ComputedTiming mochitests (v8) Review of attachment 8677334 [details] [diff] [review]: ----------------------------------------------------------------- I've only looked at the first half of this. Is there anything you were looking for feedback on? If not, please set review? ::: dom/animation/test/css-animations/file_animation-computed-timing.html @@ +20,5 @@ > +// delay > +// -------------------- > +test(function(t) { > + var div = addDiv(t); > + div.style.animation = 'moveAnimation 100s'; I think you can make this a little more compact: var div = addDiv(t, { style: 'animation: moveAnimation 100s' }); @@ +24,5 @@ > + div.style.animation = 'moveAnimation 100s'; > + > + var anim = div.getAnimations()[0]; > + assert_equals(getComputedTiming(anim).delay, 0, > + 'Initial value of delay'); Minor nit: You could just do: var effect = div.getAnimations()[0].effect; assert_equals(effect.getComputedTiming().delay, 0, 'Initial value of delay'); Then you don't need a separate getComputedTiming() function and someone reading the test doesn't need to jump around the file to understand and debug the test. This is my personal preference: to make the tests as self-contained and simple as possible to make them easy to read and debug. It's a very minor point, however, feel free to leave the test as is if you think it's better. @@ +147,5 @@ > + > + var anim = div.getAnimations()[0]; > + assert_equals(getComputedTiming(anim).duration, 0, > + 'Initial value of duration'); > +}, 'Duration of a infinitely repeating zero-duration animation'); Nit #1: All these test descriptions start with the name of the member we're testing. I think it would be more clear to use the same capitalization as the actually we're actually testing. i.e. 'duration' instead of 'Duration', 'endTime' instead of 'EndTime'. In this case "Duration" sounds like we're testing some concept of duration but it's not clear *what* duration. If we say "duration" however, it's more clear we're talking about the member that reflects the iteration duration (as opposed to the active duration). Nit #2: The iteration duration is not affected by the iteration count so I don't think we need this test? @@ +185,5 @@ > + > + var anim = div.getAnimations()[0]; > + assert_equals(getComputedTiming(anim).easing, 'linear', > + 'Initial value of easing'); > +}, 'Easing of a new animation is linear (default value)'); Looks good. It's possible to test the other values too if you want to. I'm not sure if you were intending to do that. @@ +200,5 @@ > + var anim = div.getAnimations()[0]; > + var answer = 100000 - 5000; // ms > + assert_equals(getComputedTiming(anim).endTime, answer, > + 'Initial value of endTime'); > +}, 'EndTime of a new animation'); 'endTime of an animation with a negative delay' ? @@ +209,5 @@ > + > + var anim = div.getAnimations()[0]; > + assert_equals(getComputedTiming(anim).endTime, Infinity, > + 'Initial value of endTime'); > +}, 'EndTime of a infinitely repeateing animation'); 'an infinitely repeating' @@ +229,5 @@ > + > + var anim = div.getAnimations()[0]; > + assert_equals(getComputedTiming(anim).endTime, 0, > + 'Initial value of endTime'); > +}, 'EndTime should be larger than or equal to zero'); Nit: Test description should be 'endTime of an animation that finishes before its startTime' @@ +275,5 @@ > + // If either the iteration duration or iteration count are zero, > + // the active duration is zero. > + assert_equals(getComputedTiming(anim).activeDuration, 0, > + 'Initial value of activeDuration'); > +}, 'ActiveDuration of a no repeating animation'); 'activeDuration of an animation with zero iterations' ('no repeating' sounds like the iteration count is 1) @@ +579,5 @@ > + assert_equals(getComputedTiming(anim).currentIteration, 5, > + 'Value of currentIteration after phase'); > +}, 'CurrentIteration should be applied floor correctly'); > + > +// FIXME: wait for Bug 1208951 That bug has landed now so you can enable this test. @@ +588,5 @@ > + 'currentIteration for orphaned effect'); > +}, 'currentIteration of an AnimationEffect without an Animation');*/ > + > +// TODO: If iteration duration is Infinity, currentIteration is 0. > +// However, we cannot set iteration duration to Infinity in CSS Animation now. Right, we can only do that using the API. It's fine to leave this test out for now.
Attachment #8677334 - Flags: feedback?(bbirtles) → feedback+
(In reply to Brian Birtles (:birtles) from comment #95) > I've only looked at the first half of this. Is there anything you were > looking for feedback on? If not, please set review? Thanks for your feedback. I would like to set review? after Bug 1208951 lands (it landed yesterday). The first half of this patch is the most important part, so I think your feedback is sufficient. I will set review? in my next patch. > > ::: dom/animation/test/css-animations/file_animation-computed-timing.html > @@ +20,5 @@ > > +// delay > > +// -------------------- > > +test(function(t) { > > + var div = addDiv(t); > > + div.style.animation = 'moveAnimation 100s'; > > I think you can make this a little more compact: > > var div = addDiv(t, { style: 'animation: moveAnimation 100s' }); > > @@ +24,5 @@ > > + div.style.animation = 'moveAnimation 100s'; > > + > > + var anim = div.getAnimations()[0]; > > + assert_equals(getComputedTiming(anim).delay, 0, > > + 'Initial value of delay'); > > Minor nit: > > You could just do: > > var effect = div.getAnimations()[0].effect; > assert_equals(effect.getComputedTiming().delay, 0, > 'Initial value of delay'); > > Then you don't need a separate getComputedTiming() function and someone > reading the test doesn't need to jump around the file to understand and > debug the test. > > This is my personal preference: to make the tests as self-contained and > simple as possible to make them easy to read and debug. It's a very minor > point, however, feel free to leave the test as is if you think it's better. I agree with you. I also think about this problem for a long time. Your preference makes sense. > > @@ +147,5 @@ > > + > > + var anim = div.getAnimations()[0]; > > + assert_equals(getComputedTiming(anim).duration, 0, > > + 'Initial value of duration'); > > +}, 'Duration of a infinitely repeating zero-duration animation'); > > Nit #1: All these test descriptions start with the name of the member we're > testing. I think it would be more clear to use the same capitalization as > the actually we're actually testing. i.e. 'duration' instead of 'Duration', > 'endTime' instead of 'EndTime'. > > In this case "Duration" sounds like we're testing some concept of duration > but it's not clear *what* duration. If we say "duration" however, it's more > clear we're talking about the member that reflects the iteration duration > (as opposed to the active duration). > > Nit #2: The iteration duration is not affected by the iteration count so I > don't think we need this test? OK. I should fix all the descriptions. > > @@ +185,5 @@ > > + > > + var anim = div.getAnimations()[0]; > > + assert_equals(getComputedTiming(anim).easing, 'linear', > > + 'Initial value of easing'); > > +}, 'Easing of a new animation is linear (default value)'); > > Looks good. It's possible to test the other values too if you want to. I'm > not sure if you were intending to do that. In this bug (part 3), we always assign 'linear' to easing property, so I only test 'linear' in this patch. Does Bug 1208951 implement the easing part? If you want, I can add a patch to fix the easing string, so we can test other values. Or file another bug to handle this. Thanks.
Implement KeyframeEffectReadOnly::GetComputedTiming(). Rebased.
Attachment #8677917 - Flags: review+
Attachment #8675520 - Attachment is obsolete: true
Add test_animation-computed-timing.html in css-animations and css-transitions.
Attachment #8677933 - Flags: review?(bbirtles)
Attachment #8677334 - Attachment is obsolete: true
Comment on attachment 8677933 [details] [diff] [review] Part 4: Add ComputedTiming mochitests (v9) Review of attachment 8677933 [details] [diff] [review]: ----------------------------------------------------------------- Great work. r=birtles with comments addressed. ::: dom/animation/test/css-animations/file_animation-computed-timing.html @@ +104,5 @@ > + var div = addDiv(t, {style: 'animation: moveAnimation 100s infinite'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().iterations, Infinity, > + 'Initial value of iterations'); > +}, 'iterations of a infinitely repeating animation'); 'an infinitely' @@ +177,5 @@ > + var div = addDiv(t, {style: 'animation: moveAnimation 10s -100s infinite'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().endTime, Infinity, > + 'Initial value of endTime'); > +}, 'endTime of a infinitely repeateing animation'); 'an infinitely repeating' @@ +184,5 @@ > + var div = addDiv(t, {style: 'animation: moveAnimation 0s 100s infinite'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().endTime, 100000, > + 'Initial value of endTime'); > +}, 'endTime of an infinitely repeateing zero-duration animation'); 'repeating' @@ +213,5 @@ > + var div = addDiv(t, {style: 'animation: moveAnimation 100s infinite'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().activeDuration, Infinity, > + 'Initial value of activeDuration'); > +}, 'activeDuration of a infinitely repeating animation'); 'an infinitely' Here and for a number of other tests. @@ +274,5 @@ > + var timelineDiff = anim.timeline.currentTime - prevTimelineCurrentTime; > + assert_approx_equals(localTime - prevLocalTime, > + timelineDiff * anim.playbackRate, > + 0.001, // accuracy of DOMHighResTimeStamp > + 'localTime should be 2 times faster than timeline'); How does DOMHighResTimeStamp relate here? I think this test is probably fine (without the comment about DOMHighResTimeStamp) but I'd be a little bit concerned that it might fail temporarily if the interval between frames is short due to floating-point error introduced along the way. If that turns out to be the case, it might be enough simply to check that localTime == anim.currentTime since we have tests that the currentTime reflects the playbackRate elsewhere. (Technically, all we really need to test is that localTime == anim.currentTime.) @@ +295,5 @@ > +test(function(t) { > + var div = addDiv(t, {style: 'animation: moveAnimation 100s 1s'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().progress, null, > + 'Initial value of progress before phase'); 'during before phase' or 'in before phase' @@ +296,5 @@ > + var div = addDiv(t, {style: 'animation: moveAnimation 100s 1s'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().progress, null, > + 'Initial value of progress before phase'); > +}, 'progress of a new animation is unresolved before phase (none fill)'); 'no backwards fill' @@ +342,5 @@ > + ani = getAnimWithFill('both'); > + assert_true(checkStatus(ani, 0.0), 'both (initial)'); > + ani.finish(); > + assert_true(checkStatus(ani, 1.0), 'both (finish)'); > +}, 'progress of an animation with different fill modes'); I think this test covers the same content as the two tests before it? Also, I think we could rewrite this as: test(function(f) { [ { fill: '', progress: [ null, null ] }, { fill: 'none', progress: [ null, null ] }, { fill: 'forwards', progress: [ null, 1.0 ] }, { fill: 'backwards', progress: [ 0.0, null ] }, { fill: 'both', progress: [ 0.0, 1.0 ] } ] .forEach(function(test) { var div = addDiv(t, {style: 'animation: moveAnimation 100s 10s ' + test.fill}); var anim = div.getAnimations()[0]; assert_true(anim.effect.getComputedTiming().progress === test.progress[0], 'initial progress with "' + test.fill + '" fill'); anim.finish(); assert_true(anim.effect.getComputedTiming().progress === test.progress[1], 'finished progress with "' + test.fill + '" fill'); }); }); Then we could drop the two tests before this, I think. @@ +348,5 @@ > +test(function(t) { > + var div = addDiv(t); > + // Note: FillMode here is "both" because > + // 1. Fill forwards for zero-duration animation. If fill isn't forwards, > + // the return object of div.getAnimations()[0] is undefined. It might be more clear to say: 1. Since this a zero-duration animation, it will already have finished so it won't be returned by getAnimations() unless it fills forwards. @@ +387,5 @@ > + > + // Seek forwards to finish time (or call anim.finish()) > + // Using iteration duration of 1 now. > + // currentIteration now is floor(10.25) = 10, so progress should be 25%. > + anim.currentTime += 5000; Why don't we just use finish() here? @@ +400,5 @@ > + assert_equals(anim.effect.getComputedTiming().progress, 1.0, > + 'Initial value of progress (before phase)'); > + > + // Seek forwards > + anim.currentTime += 5000; Again, why not just use finish(). @@ +446,5 @@ > + anim.finish() > + assert_equals(anim.effect.getComputedTiming().progress, 0.75, > + 'Value of progress'); > +}, 'progress of a non-integral repeating animation ' + > + 'with alternate-reversing direction'); This is great. Do we also need a test for alternate and alternate-reverse with a zero-duration animation? @@ +457,5 @@ > + var div = addDiv(t, {style: 'animation: moveAnimation 100s 2s'}); > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().currentIteration, null, > + 'Initial value of currentIteration before phase'); > +}, 'currentIteration of a new animation is unresolved before phase'); 'currentIteration of a new animation with no backwards fill is unresolved in before phase' @@ +469,5 @@ > + > +test(function(t) { > + // Note: FillMode here is "both" because > + // 1. Fill forwards for zero-duration animation. If fill isn't forwards, > + // the return object of div.getAnimations()[0] is undefined. Again, this comment could be reworked as described before. @@ +488,5 @@ > +test(function(t) { > + // Note: FillMode here is "both" because > + // 1. Fill forwards for zero-duration animation. > + // 2. Fill backwards, so the currentIteration (before phase) wouldn't be > + // unresolved (null value). This description is a bit confusing. I'm not sure it's even necessary--I think we could just delete it. @@ +495,5 @@ > + > + // Note: currentIteration = ceil(iteration start + iteration count) - 1 > + assert_equals(anim.effect.getComputedTiming().currentIteration, 10, > + 'Initial value of currentIteration calculated by ceil ' + > + '(after phase)'); I think the description here could be just 'Initial value of currentIteration'. No need to refer to the algorithm used in the test description. The comment is fine, however. @@ +513,5 @@ > + // 3rd iteration > + // Note: currentIteration = floor(scaled active time / iteration duration) > + anim.currentTime = 250000; // ms > + assert_equals(anim.effect.getComputedTiming().currentIteration, 2, > + 'Value of 3rd currentIteration'); 'Value of currentIteration during 3rd iteration' @@ +526,5 @@ > + var effect = new KeyframeEffectReadOnly(div, {left: ["0px", "100px"]}); > + > + assert_equals(effect.getComputedTiming().currentIteration, null, > + 'currentIteration for orphaned effect'); > +}, 'currentIteration of an AnimationEffect without an Animation'); This is great. It seems we don't have any tests for an *integral* iteration count, however. We should probably test an animation that repeats twice (specifically, to test that when it finishes it has a currentIteration of 2.0), and an animation with the default iteration count of 1. ::: dom/animation/test/css-animations/test_animation-computed-timing.html @@ +3,5 @@ > +<script src="/resources/testharness.js"></script> > +<script src="/resources/testharnessreport.js"></script> > +<div id="log"></div> > +<script> > +// Bug 1108055 - Implement ComputedTiming for Web Animation API No need for the bug number here. This test will eventually be merged to the cross-browser web-platform-tests repository. ::: dom/animation/test/css-transitions/file_animation-computed-timing.html @@ +17,5 @@ > +// delay > +// -------------------- > +test(function(t) { > + var div = addDiv(t, {'class': 'animated-div'}); > + div.style.transition = 'margin-left 10s'; Since we do this a lot, we could make it a little more compact, var div = addDiv(t, { 'class': 'animated-div', transition: 'margin-left 10s' }); But it's up to you. Either way is fine. @@ +61,5 @@ > + > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().endDelay, 0, > + 'Initial value of endDelay'); > +}, 'endDelay of a new transition is zero'); Nit: For some tests we say the expected result, e.g. 'endDelay of a new transition is zero', but for others we don't, e.g. 'fill of a new transition'. It might be nice to make them consistent? e.g. make this just 'endDelay of a new transition' @@ +75,5 @@ > + div.style.marginLeft = '10px'; > + > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().fill, 'backwards', > + 'Fill forwards and backwards'); This comment doesn't seem to match what we are testing. @@ +240,5 @@ > + timelineDiff * anim.playbackRate, > + 0.001, // accuracy of DOMHighResTimeStamp > + 'localTime should be 2 times faster than timeline'); > + })); > +}, 'localTime reflects playbackRate immediately'); See my comments on the similar test for CSS animations. In brief, this is probably fine, but we could make it more simple but just comparing localTime to anim.currentTime. @@ +255,5 @@ > + > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().progress, 0.0, > + 'Initial value of progress'); > +}, 'progress of a new transition is 0%'); We should also test the value in the before phase when we have a positive delay. @@ +282,5 @@ > + > + var effect = div.getAnimations()[0].effect; > + assert_equals(effect.getComputedTiming().currentIteration, 0, > + 'Initial value of currentIteration'); > +}, 'currentIteration of a new transition is zero'); Again, we could possibly test this when we have a positive delay. ::: dom/animation/test/css-transitions/test_animation-computed-timing.html @@ +3,5 @@ > +<script src="/resources/testharness.js"></script> > +<script src="/resources/testharnessreport.js"></script> > +<div id="log"></div> > +<script> > +// Bug 1108055 - Implement ComputedTiming for Web Animation API Again, no bug number needed.
Attachment #8677933 - Flags: review?(bbirtles) → review+
Thanks for your review, those suggestions are really helpful. Looks like I have many typos, and I should check the again. (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #99) > Comment on attachment 8677933 [details] [diff] [review] > Part 4: Add ComputedTiming mochitests (v9) > > Review of attachment 8677933 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great work. r=birtles with comments addressed. > > @@ +274,5 @@ > > + var timelineDiff = anim.timeline.currentTime - prevTimelineCurrentTime; > > + assert_approx_equals(localTime - prevLocalTime, > > + timelineDiff * anim.playbackRate, > > + 0.001, // accuracy of DOMHighResTimeStamp > > + 'localTime should be 2 times faster than timeline'); > > How does DOMHighResTimeStamp relate here? > > I think this test is probably fine (without the comment about > DOMHighResTimeStamp) but I'd be a little bit concerned that it might fail > temporarily if the interval between frames is short due to floating-point > error introduced along the way. If that turns out to be the case, it might > be enough simply to check that localTime == anim.currentTime since we have > tests that the currentTime reflects the playbackRate elsewhere. > (Technically, all we really need to test is that localTime == > anim.currentTime.) I agree. Actually I just revise that test and do it again here. I think removing this is fine. > > @@ +342,5 @@ > > + ani = getAnimWithFill('both'); > > + assert_true(checkStatus(ani, 0.0), 'both (initial)'); > > + ani.finish(); > > + assert_true(checkStatus(ani, 1.0), 'both (finish)'); > > +}, 'progress of an animation with different fill modes'); > > I think this test covers the same content as the two tests before it? > > Also, I think we could rewrite this as: > > test(function(f) { > [ { fill: '', progress: [ null, null ] }, > { fill: 'none', progress: [ null, null ] }, > { fill: 'forwards', progress: [ null, 1.0 ] }, > { fill: 'backwards', progress: [ 0.0, null ] }, > { fill: 'both', progress: [ 0.0, 1.0 ] } ] > .forEach(function(test) { > var div = > addDiv(t, {style: 'animation: moveAnimation 100s 10s ' + test.fill}); > var anim = div.getAnimations()[0]; > assert_true(anim.effect.getComputedTiming().progress === > test.progress[0], > 'initial progress with "' + test.fill + '" fill'); > anim.finish(); > assert_true(anim.effect.getComputedTiming().progress === > test.progress[1], > 'finished progress with "' + test.fill + '" fill'); > }); > }); > > Then we could drop the two tests before this, I think. Great! This is helpful. > @@ +387,5 @@ > > + > > + // Seek forwards to finish time (or call anim.finish()) > > + // Using iteration duration of 1 now. > > + // currentIteration now is floor(10.25) = 10, so progress should be 25%. > > + anim.currentTime += 5000; > > Why don't we just use finish() here? Yes, using finish() is much clearer. > > @@ +446,5 @@ > > + anim.finish() > > + assert_equals(anim.effect.getComputedTiming().progress, 0.75, > > + 'Value of progress'); > > +}, 'progress of a non-integral repeating animation ' + > > + 'with alternate-reversing direction'); > > This is great. Do we also need a test for alternate and alternate-reverse > with a zero-duration animation? Sure. > > @@ +526,5 @@ > > + var effect = new KeyframeEffectReadOnly(div, {left: ["0px", "100px"]}); > > + > > + assert_equals(effect.getComputedTiming().currentIteration, null, > > + 'currentIteration for orphaned effect'); > > +}, 'currentIteration of an AnimationEffect without an Animation'); > > This is great. It seems we don't have any tests for an *integral* iteration > count, however. > > We should probably test an animation that repeats twice (specifically, to > test that when it finishes it has a currentIteration of 2.0), and an > animation with the default iteration count of 1. OK. > ::: dom/animation/test/css-transitions/file_animation-computed-timing.html > @@ +17,5 @@ > > +// delay > > +// -------------------- > > +test(function(t) { > > + var div = addDiv(t, {'class': 'animated-div'}); > > + div.style.transition = 'margin-left 10s'; > > Since we do this a lot, we could make it a little more compact, > > var div = addDiv(t, { 'class': 'animated-div', transition: 'margin-left > 10s' }); > > But it's up to you. Either way is fine. I prefer the compact one. :) > @@ +255,5 @@ > > + > > + var effect = div.getAnimations()[0].effect; > > + assert_equals(effect.getComputedTiming().progress, 0.0, > > + 'Initial value of progress'); > > +}, 'progress of a new transition is 0%'); > > We should also test the value in the before phase when we have a positive > delay. OK.
>+async_test(function(t) { >+ var div = addDiv(t, {style: 'animation: moveAnimation 100s'}); >+ >+ var anim = div.getAnimations()[0]; >+ var prevLocalTime; >+ var prevTimelineCurrentTime; >+ >+ anim.playbackRate = 2; // 2 times faster >+ >+ anim.ready.then(function() { >+ prevLocalTime = anim.effect.getComputedTiming().localTime; >+ prevTimelineCurrentTime = anim.timeline.currentTime; >+ return waitForFrame(); One more thing I remembered, we should use t.step_func here and elsewhere where we have callback functions in async_tests. i.e. anim.ready.then(t.step_func(function() {
Attachment #8677933 - Attachment is obsolete: true
Attachment #8679823 - Flags: review+
Attachment #8679823 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Add test_animation-computed-timing.html in css-animations and css-transitions. Fix missing html tags.
Attachment #8681813 - Flags: review+
Attachment #8679833 - Attachment is obsolete: true
Keywords: checkin-needed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: