This is needed for hanging getAnimationPlayers() off so we can fetch all the animations in the document

Comment on attachment 8408851 [details] [diff] [review] part 1 - Add pref for Web Animations API I'm not sure if this preference should be more fine-grained. I imagine in the future we might release subsets of features such as the following: * API for querying CSS animations (bug 998245) * API for creating animation from script (= to what blink will ship soon) * API with features currently only used by SVG (animation addition etc.) * API for animation groups (further in the future) If we end up shipping the first two items together then perhaps the current name is ok, otherwise layout.web-animations.api.core.enabled is better?

Comment on attachment 8409537 [details] [diff] [review] part 5 - Add tests for AnimationTimeline I'm not quite sure how to arrange the test files here. I'm hoping that a lot of the API tests will be hosted at web-platform-tests. I have a pull-request opened for that now: https://critic.hoppipolla.co.uk/r/1302 I think the remote structure might look something like: web-platform-tests/ web-animations/ core/ animation-timeline.html etc. support/ ... css-integration/ animation-direction.html etc. svg-integration/ ... Then in mozilla-central we would have dom/ animation/ test/ (Gecko-specific tests go directly here) submitted/ core/ test_animation-timeline.html support/ ... css-integration/ (should this go in layout/style/test ?) ... svg-integration/ (content/svg/content/test ?) Then when we run importTestsuite.py remove any tests from submitted that show up in imptests/web-animations/ ? I'm not sure how we normally arrange this.

Comment on attachment 8408853 [details] [diff] [review] part 2 - Add AnimationTimeline interface >+++ b/dom/animation/AnimationTimeline.h >+#include "mozilla/ErrorResult.h" >+#include "mozilla/dom/Nullable.h" These are both not needed, right? >+struct JSContext; #include "js/TypeDecls.h" r=me

Comment on attachment 8408854 [details] [diff] [review] part 3 - Add timeline member to Document interface >+ virtual already_AddRefed<mozilla::dom::AnimationTimeline> Timeline() = 0; virtual mozilla::dom::AnimationTimeline* Timeline() = 0; And then the impl can just "return mAnimationTimeline;" at the end. >+++ b/dom/webidl/Document.webidl Please add the new link to the comment at the start of the file too? Or just nix that list in the comment at the start, since it's out of sync anyway: it's missing the selectors API bits and undomanager. :( r=me

Comment on attachment 8409535 [details] [diff] [review] part 4 - Add currentTime member to AnimationTimeline Is the fact that being in a display:none subframe means currentTime returns null actually called for by the spec, or a consequence of where our refresh driver lives? Please document, either way, and if it's the latter file a followup bug to fix things up? The spec seems to conceptually have AnimationTimeline objects other than the document timeline. Is that something we should handle right now somehow (e.g. by having a DocumentAnimationTimeline subclass of AnimationTimeline) or something we'll worry about later if/when we add different types of timelines? r=me

Thanks for the reviews Boris! (In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #11) > Comment on attachment 8408854 [details] [diff] [review] > part 3 - Add timeline member to Document interface > > >+++ b/dom/webidl/Document.webidl > > Please add the new link to the comment at the start of the file too? Or > just nix that list in the comment at the start, since it's out of sync > anyway: it's missing the selectors API bits and undomanager. :( I just removed that list in the comment at the start since I suspect it will get out-of-sync easily. (In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #12) > Comment on attachment 8409535 [details] [diff] [review] > part 4 - Add currentTime member to AnimationTimeline > > Is the fact that being in a display:none subframe means currentTime returns > null actually called for by the spec, or a consequence of where our refresh > driver lives? Please document, either way, and if it's the latter file a > followup bug to fix things up? The latter. I hadn't even thought of that. I've filed bug 1002332 for this. > The spec seems to conceptually have AnimationTimeline objects other than the > document timeline. Is that something we should handle right now somehow > (e.g. by having a DocumentAnimationTimeline subclass of AnimationTimeline) > or something we'll worry about later if/when we add different types of > timelines? That's right. In the future I anticipate having SVGAnimationTimeline (since SVG has some extra per-document fragment APIs) and there have been ideas of mapping gestures / scroll position / media elements to timelines. I think it's better to worry about when/if we add them later since we don't know yet what they will look like. I expect that if there are members on AnimationTimeline that we don't want on other types of timelines then we can push them down to a DocumentAnimationTimeline subinterface at that time without much breakage.

(In reply to Brian Birtles (:birtles) from comment #8) > If we end up shipping the first two items together then perhaps the current > name is ok, otherwise layout.web-animations.api.core.enabled is better? On further thought, I think this should probably be: dom.animations.api.core.enabled dom.animations.api.element-animate.enabled (for the Blink subset) dom.animations.api.svg-features.enabled (for extra features added to support SVG) dom.animations.api.svg-mapping.enabled (for when we start exposing SVG animations through the API) etc. Maybe we can drop the 'api'?

(In reply to Brian Birtles (:birtles) from comment #9) > Comment on attachment 8409537 [details] [diff] [review] > part 5 - Add tests for AnimationTimeline > > I'm not quite sure how to arrange the test files here. > > I'm hoping that a lot of the API tests will be hosted at web-platform-tests. > I have a pull-request opened for that now: > https://critic.hoppipolla.co.uk/r/1302 > > I think the remote structure might look something like: > > web-platform-tests/ > web-animations/ > core/ > animation-timeline.html etc. > support/ > ... > css-integration/ > animation-direction.html etc. > svg-integration/ > ... After talking with folks about web-platform-tests I think the structure there will be more like: web-platform-tests/ web-animations/ animation-timeline/ animation-timeline-current-time.html etc. animation-player/ ... css-animation-api/ (or whatever the short name is for the spec that maps CSS Animations onto the API) ... animation-elements/ (for SVG mapping) ... I'm still not sure, however, how we arrange this on the m-c side in order to handle (a) Gecko-specific tests, (b) modifications to tests in web-platform-tests, (c) submitted tests that have yet to be approved etc.

Comment on attachment 8413553 [details] [diff] [review] part 2.5 - Add AnimationTimeline to list of interfaces r=me

Seems ok on try: https://tbpl.mozilla.org/?tree=Try&rev=0372896b6f70

Just a few extra tweaks based on feedback over at web-platform-tests

Fix bitrot

Fix bitrot

(In reply to Brian Birtles (:birtles) from comment #21) > I'm still not sure, however, how we arrange this on the m-c side in order to > handle (a) Gecko-specific tests, (b) modifications to tests in > web-platform-tests, (c) submitted tests that have yet to be approved etc. So the long-term plan is to automatically import all the web-platform tests. Exactly what "long-term" means here depends on how fast I can track down the remaining unstable tests; the whole suite is already running on Cedar [1] and seems to be green around three times out of four, which clearly isn't good enough to enable yet, but it's progress from the situation a week ago when it was closer to perma-orange. My so-far-untested plan for handling submitted tests and gecko-specific tests is to have two additional testsuites; web-platform-tests-outbound and web-platform-tests-mozilla (or something). Tests checked in to the former will run on our infrastructure and on try and so on, but once they land on m-c we will move the tests to upstream web-platform-tests, carrying forward the r+ from bugzilla, and remove them from the -outbound testsuite. The -local tests will just be a normal mozilla-specific testsuite but written using the same tools as web-platform-tests. So, that's the plan for the future. Hopefully the future will happen in weeks rather than months, but it's not very easy to predict. In the meantime, importing specific tests using the same mechanism as the imptests in dom seems like a sensible idea. [1] https://tbpl.mozilla.org/?tree=Cedar&rev=cbcc19607408

Comment on attachment 8408851 [details] [diff] [review] part 1 - Add pref for Web Animations API Maybe use "layout.web-animations-api.enabled" unless there are other parts of web animations that would have different prefs? r=dbaron

Comment on attachment 8414895 [details] [diff] [review] part 5 - Add tests for AnimationTimeline Punting this review to dholbert; sorry, should have done so sooner.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #28) > Comment on attachment 8408851 [details] [diff] [review] > part 1 - Add pref for Web Animations API > > Maybe use "layout.web-animations-api.enabled" unless there are other parts > of web animations that would have different prefs? > > r=dbaron Thanks David. I posted an alternative arrangement in comment 20 which I prefer. What do you think? If I don't hear from you I'll stick with your suggestion above.

If you want separate prefs, that proposal is fine too. But again maybe use "animations-api" instead of "animations.api"; I think that's probably better than dropping the "api" entirely.

(In reply to James Graham [:jgraham] from comment #27) > (In reply to Brian Birtles (:birtles) from comment #21) > > > I'm still not sure, however, how we arrange this on the m-c side in order to > > handle (a) Gecko-specific tests, (b) modifications to tests in > > web-platform-tests, (c) submitted tests that have yet to be approved etc. > > So the long-term plan is to automatically import all the web-platform tests. > Exactly what "long-term" means here depends on how fast I can track down the > remaining unstable tests; the whole suite is already running on Cedar [1] > and seems to be green around three times out of four, which clearly isn't > good enough to enable yet, but it's progress from the situation a week ago > when it was closer to perma-orange. > > My so-far-untested plan for handling submitted tests and gecko-specific > tests is to have two additional testsuites; web-platform-tests-outbound and > web-platform-tests-mozilla (or something). Tests checked in to the former > will run on our infrastructure and on try and so on, but once they land on > m-c we will move the tests to upstream web-platform-tests, carrying forward > the r+ from bugzilla, and remove them from the -outbound testsuite. The > -local tests will just be a normal mozilla-specific testsuite but written > using the same tools as web-platform-tests. > > So, that's the plan for the future. Hopefully the future will happen in > weeks rather than months, but it's not very easy to predict. In the > meantime, importing specific tests using the same mechanism as the imptests > in dom seems like a sensible idea. > > [1] https://tbpl.mozilla.org/?tree=Cedar&rev=cbcc19607408 Thanks James! That's good to know. On a related note, how to we handle features that may be prefed off? The features in this patch are enabled only on Nightly/Aurora. I don't want the tests to fail once they arrive on Beta. Is it reasonable to litter files in web-platform-tests with code like: var enabled = !window.SpecialPowers || SpecialPowers.getBoolPref("dom.animations-api.core.enabled"); if (!enabled) { // abort }

(In reply to Brian Birtles (:birtles) from comment #32) > (In reply to James Graham [:jgraham] from comment #27) > Thanks James! That's good to know. > > On a related note, how to we handle features that may be prefed off? The > features in this patch are enabled only on Nightly/Aurora. I don't want the > tests to fail once they arrive on Beta. Is it reasonable to litter files in > web-platform-tests with code like: You can't use specialPowers in web-platform-tests (you also can't use vendor prefixes). The right way to fix this is to edit [1] so that this feature is always enabled for mochitests. That prefs file will also be used for web-platform-tests when run from a Mozilla tree. [1] http://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js

Comment on attachment 8414895 [details] [diff] [review] part 5 - Add tests for AnimationTimeline High-level comments on the test patch (not having looked at the actual test itself yet): >diff --git a/dom/animation/moz.build b/dom/animation/moz.build >--- a/dom/animation/moz.build >+++ b/dom/animation/moz.build >+TEST_TOOL_DIRS += ['test'] I'm pretty sure this wants to be TEST_DIRS, since you're just adding tests -- not tools for use in testing. See definition of TOOL_DIRS, TEST_DIRS, and TEST_TOOL_DIRS here: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/sandbox_symbols.py#413 >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html Probably replace the hyphen in the test's filename with an underscore? In my experience, most of our mochitests use underscores for separation in the test name, and it's a bit strange to see both underscore and hyphen alongside each other in a single filename. >+++ b/dom/animation/test/mochitest.ini >@@ -0,0 +1,1 @@ >+[animation-timeline/test_animation-timeline.html] Do we gain anything from having this subdirectory here? We don't seem to group mochitests into subdirectories very often, for some reason. (though we do for reftests; I suspect this is a historical artifact due to the fact that reftests manifests are simpler) Given that we're listing the subdirectory's full contents (just one file for now) in its *parent directory's* mochitests.ini file, it doesn't seem like we're getting much organizational benefit from having the subdirectory, since we still have all the files (just one for now) having to be grouped together somewhere. So maybe drop the subdir, unless there's a strong reason for it?

Comment on attachment 8414895 [details] [diff] [review] part 5 - Add tests for AnimationTimeline >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html >@@ -0,0 +1,106 @@ >+<!doctype html> >+<title>Web Animations API: AnimationTimeline tests</title> Comparing this against another one of our (few) idlharness.js-based tests, I notice the other one has... <meta charset=utf-8> ...at the top, before <title>. Add that here, too, to keep us from spamming a warning to the error console about "The character encoding of the HTML document was not declared." >+<iframe src="data:text/html;charset=utf-8,%3Chtml%3E%3C%2Fhtml%3E" width="10" height="10" id="iframe"></iframe> Note: If you like, you could probably drop everything between the comma and the close-quote (the urlencoded <html></html>). There's no explicit <html> tag on the actual main test file here, so it feels a bit silly to have one on the empty iframe. :) But it doesn't really matter. >+test(function() { >+ var idlArray = new IdlArray(); >+ idlArray.add_idls( >+ document.getElementById('AnimationTimeline-IDL').textContent); >+ idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); >+ idlArray.test(); >+}, This IdlArray()-style test is new to me, so it might be good to get a sanity-check from someone who's written/reviewed one of these before (Ms2ger or AryehGregor, or maybe heycam?) I'm happy to grant r+, if you also get f+ or r+ from one of them. >+test(function() { >+ assert_equals(document.timeline, document.timeline, >+ 'document.timeline returns the same object every time'); >+ var iframe = document.getElementById('iframe'); >+ assert_not_equals(document.timeline, iframe.contentDocument.timeline, >+ 'document.timeline returns a different object for each document'); >+}, Maybe worth also asserting that in each of these cases, document.timeline is an actual thing (not null, not-undefined). I could imagine us having a bug where it's null in an iframe, or something. With that bug, we'd still pass your tests above and the rest of this file's tests, I think. Something like this would catch that: assert_equals(typeof(document.timeline), "object", 'document.timeline should be an object'); assert_not_equals(typeof(document.timeline), null, 'document.timeline should be non-null); ...plus the same for iframe.contentDocument. >+'document.timeline.currentTime value tests', >+{ >+ help: [ >+ 'http://dev.w3.org/fxtf/web-animations/#the-global-clock', >+ 'http://dev.w3.org/fxtf/web-animations/#the-document-timeline' >+ ], >+ assert: [ >+ 'The global clock is a source of monotonically increasing time values', >+ 'The time values of the document timeline are calculated as a fixed' + >+ ' offset from the global clock', >+ 'the zero time corresponds to the navigationStart moment', >+ 'the time value of each document timeline must be equal to the time ' + >+ 'passed to animation frame request callbacks for that browsing context' >+ ], Out of curiosity, what's the benefit of having this array of extra descriptive text in the "assert: " block here? Is there a 1:1 mapping between assert_* calls and entries in this array? If so, wouldn't it be simpler & less fragile for this text to be directly included in the assert_* calls? (If I'm understanding correctly, it seems like you could insert an assertion and forget to add a new string to this array, and then we'd get the textual-description wrong when there's a test-failure, and that would be pretty confusing. I might be misunderstanding how this works, though.) >+async_test(function(t) { >+ var valueAtStart = document.timeline.currentTime; >+ var timeAtStart = window.performance.now(); >+ while (window.performance.now() - timeAtStart < 100) { >+ // Wait 100ms >+ } (Maybe declare these variables 'const' instead of 'var' to be clearer (& enforce) that they're never gonna change? >+++ b/dom/animation/test/moz.build >@@ -0,0 +1,7 @@ >+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- >+# vim: set filetype=python: >+# 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/. >+ >+MOCHITEST_MANIFESTS += ['mochitest.ini'] IIRC nowadays the build folks want to explicitly review any moz.build changes, with a few small exceptions (for e.g. adding to an existing SOURCES list). I don't think adding a new moz.build file (trivial as it may be) is one of those exceptions, so you probably should have a build peer (maybe mshal or glandium) sign off on this change. (Probably on the dom/animation/moz.build addition, too.)

(by "dom/animation/moz.build addition" I meant "dom/animation/moz.build creation", which I presume happens in another patch here)

(In reply to Daniel Holbert [:dholbert] from comment #35) > Comment on attachment 8414895 [details] [diff] [review] > part 5 - Add tests for AnimationTimeline > > >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html > >@@ -0,0 +1,106 @@ > >+<!doctype html> > >+<title>Web Animations API: AnimationTimeline tests</title> > > Comparing this against another one of our (few) idlharness.js-based tests, I > notice the other one has... > <meta charset=utf-8> > ...at the top, before <title>. Add that here, too, to keep us from spamming > a warning to the error console about "The character encoding of the HTML > document was not declared." > > >+<iframe src="data:text/html;charset=utf-8,%3Chtml%3E%3C%2Fhtml%3E" width="10" height="10" id="iframe"></iframe> > > Note: If you like, you could probably drop everything between the comma and > the close-quote (the urlencoded <html></html>). There's no explicit <html> > tag on the actual main test file here, so it feels a bit silly to have one > on the empty iframe. :) But it doesn't really matter. > > >+test(function() { > >+ var idlArray = new IdlArray(); > >+ idlArray.add_idls( > >+ document.getElementById('AnimationTimeline-IDL').textContent); > >+ idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); > >+ idlArray.test(); > >+}, > > This IdlArray()-style test is new to me, so it might be good to get a > sanity-check from someone who's written/reviewed one of these before (Ms2ger > or AryehGregor, or maybe heycam?) I'm happy to grant r+, if you also get f+ > or r+ from one of them. Current practice is var idlArray; setup(function() { idlArray = new IdlArray(); idlArray.add_idls( document.getElementById('AnimationTimeline-IDL').textContent); idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); }); idlArray.test(); > >+test(function() { > >+ assert_equals(document.timeline, document.timeline, > >+ 'document.timeline returns the same object every time'); > >+ var iframe = document.getElementById('iframe'); > >+ assert_not_equals(document.timeline, iframe.contentDocument.timeline, > >+ 'document.timeline returns a different object for each document'); > >+}, > > Maybe worth also asserting that in each of these cases, document.timeline is > an actual thing (not null, not-undefined). I could imagine us having a bug > where it's null in an iframe, or something. With that bug, we'd still pass > your tests above and the rest of this file's tests, I think. > > Something like this would catch that: > assert_equals(typeof(document.timeline), "object", > 'document.timeline should be an object'); > assert_not_equals(typeof(document.timeline), null, > 'document.timeline should be non-null); > > ...plus the same for iframe.contentDocument. I assume you mean assert_not_equals(document.timeline, null, ...) without the typeof. > >+'document.timeline.currentTime value tests', > >+{ > >+ help: [ > >+ 'http://dev.w3.org/fxtf/web-animations/#the-global-clock', > >+ 'http://dev.w3.org/fxtf/web-animations/#the-document-timeline' > >+ ], > >+ assert: [ > >+ 'The global clock is a source of monotonically increasing time values', > >+ 'The time values of the document timeline are calculated as a fixed' + > >+ ' offset from the global clock', > >+ 'the zero time corresponds to the navigationStart moment', > >+ 'the time value of each document timeline must be equal to the time ' + > >+ 'passed to animation frame request callbacks for that browsing context' > >+ ], > > Out of curiosity, what's the benefit of having this array of extra > descriptive text in the "assert: " block here? Is there a 1:1 mapping > between assert_* calls and entries in this array? If so, wouldn't it be > simpler & less fragile for this text to be directly included in the assert_* > calls? > > (If I'm understanding correctly, it seems like you could insert an assertion > and forget to add a new string to this array, and then we'd get the > textual-description wrong when there's a test-failure, and that would be > pretty confusing. I might be misunderstanding how this works, though.) This is make-work for the CSSWG, so I wouldn't spend time on it. > >+async_test(function(t) { > >+ var valueAtStart = document.timeline.currentTime; > >+ var timeAtStart = window.performance.now(); > >+ while (window.performance.now() - timeAtStart < 100) { > >+ // Wait 100ms > >+ } > > (Maybe declare these variables 'const' instead of 'var' to be clearer (& > enforce) that they're never gonna change? Don't use const until it's widely supported. > >+++ b/dom/animation/test/moz.build > >@@ -0,0 +1,7 @@ > >+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > >+# vim: set filetype=python: > >+# 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/. > >+ > >+MOCHITEST_MANIFESTS += ['mochitest.ini'] > > IIRC nowadays the build folks want to explicitly review any moz.build > changes, with a few small exceptions (for e.g. adding to an existing SOURCES > list). I don't think adding a new moz.build file (trivial as it may be) is > one of those exceptions, so you probably should have a build peer (maybe > mshal or glandium) sign off on this change. > > (Probably on the dom/animation/moz.build addition, too.) There's no need to create a new moz.build here; you can add MOCHITEST_MANIFESTS += ['test/mochitest.ini'] to dom/animation/moz.build.

(In reply to James Graham [:jgraham] from comment #33) > You can't use specialPowers in web-platform-tests (you also can't use vendor > prefixes). > > The right way to fix this is to edit [1] so that this feature is always > enabled for mochitests. That prefs file will also be used for > web-platform-tests when run from a Mozilla tree. Great, thanks! Added to the patch. (In reply to Daniel Holbert [:dholbert] from comment #34) > Comment on attachment 8414895 [details] [diff] [review] > part 5 - Add tests for AnimationTimeline > > High-level comments on the test patch (not having looked at the actual test > itself yet): > > >diff --git a/dom/animation/moz.build b/dom/animation/moz.build > >--- a/dom/animation/moz.build > >+++ b/dom/animation/moz.build > >+TEST_TOOL_DIRS += ['test'] > > I'm pretty sure this wants to be TEST_DIRS, since you're just adding tests > -- not tools for use in testing. Fixed. > >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html > > Probably replace the hyphen in the test's filename with an underscore? In my > experience, most of our mochitests use underscores for separation in the > test name, and it's a bit strange to see both underscore and hyphen > alongside each other in a single filename. I think web-platform-tests tends to favor hyphens, e.g. https://github.com/w3c/web-platform-tests/tree/master/custom-elements/creating-and-passing-registries https://github.com/w3c/web-platform-tests/tree/master/battery-status etc. I agree it's weird but the 'test_' bit is simply something our build system requires (and something we should ultimately fix here). > >+++ b/dom/animation/test/mochitest.ini > >@@ -0,0 +1,1 @@ > >+[animation-timeline/test_animation-timeline.html] > > Do we gain anything from having this subdirectory here? We don't seem to > group mochitests into subdirectories very often, for some reason. (though we > do for reftests; I suspect this is a historical artifact due to the fact > that reftests manifests are simpler) Again, this is a web-platform-tests thing. It says: "For some of the specifications, the tree under the top-level directory represents the sections of the respective documents, using the section IDs for directory names, with a maximum of three levels deep. So if you're looking for tests in HTML for "The History interface", they will be under html/browsers/history/the-history-interface/" (https://github.com/w3c/web-platform-tests) We decided in the Web Animations group to follow this layout. I'm pretty sure the current 'animation-timeline.html' file will eventually be split up into several different files when we add more tests and this structure will probably make more sense then. (In reply to Daniel Holbert [:dholbert] from comment #35) > Comment on attachment 8414895 [details] [diff] [review] > part 5 - Add tests for AnimationTimeline > > >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html > >@@ -0,0 +1,106 @@ > >+<!doctype html> > >+<title>Web Animations API: AnimationTimeline tests</title> > > Comparing this against another one of our (few) idlharness.js-based tests, I > notice the other one has... > <meta charset=utf-8> > ...at the top, before <title>. Add that here, too, to keep us from spamming > a warning to the error console about "The character encoding of the HTML > document was not declared." Fixed. > >+<iframe src="data:text/html;charset=utf-8,%3Chtml%3E%3C%2Fhtml%3E" width="10" height="10" id="iframe"></iframe> > > Note: If you like, you could probably drop everything between the comma and > the close-quote (the urlencoded <html></html>). There's no explicit <html> > tag on the actual main test file here, so it feels a bit silly to have one > on the empty iframe. :) But it doesn't really matter. Fixed. > >+test(function() { > >+ var idlArray = new IdlArray(); > >+ idlArray.add_idls( > >+ document.getElementById('AnimationTimeline-IDL').textContent); > >+ idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); > >+ idlArray.test(); > >+}, > > This IdlArray()-style test is new to me, so it might be good to get a > sanity-check from someone who's written/reviewed one of these before (Ms2ger > or AryehGregor, or maybe heycam?) I'm happy to grant r+, if you also get f+ > or r+ from one of them. Will do. > >+test(function() { > >+ assert_equals(document.timeline, document.timeline, > >+ 'document.timeline returns the same object every time'); > >+ var iframe = document.getElementById('iframe'); > >+ assert_not_equals(document.timeline, iframe.contentDocument.timeline, > >+ 'document.timeline returns a different object for each document'); > >+}, > > Maybe worth also asserting that in each of these cases, document.timeline is > an actual thing (not null, not-undefined). I could imagine us having a bug > where it's null in an iframe, or something. With that bug, we'd still pass > your tests above and the rest of this file's tests, I think. The idlharness.js test should pick that up. I originally had that check but I was told it's better to use idlharness.js for that. I'm happy to re-add it though. What do you think? > >+'document.timeline.currentTime value tests', > >+{ > >+ help: [ > >+ 'http://dev.w3.org/fxtf/web-animations/#the-global-clock', > >+ 'http://dev.w3.org/fxtf/web-animations/#the-document-timeline' > >+ ], > >+ assert: [ > >+ 'The global clock is a source of monotonically increasing time values', > >+ 'The time values of the document timeline are calculated as a fixed' + > >+ ' offset from the global clock', > >+ 'the zero time corresponds to the navigationStart moment', > >+ 'the time value of each document timeline must be equal to the time ' + > >+ 'passed to animation frame request callbacks for that browsing context' > >+ ], > > Out of curiosity, what's the benefit of having this array of extra > descriptive text in the "assert: " block here? Is there a 1:1 mapping > between assert_* calls and entries in this array? If so, wouldn't it be > simpler & less fragile for this text to be directly included in the assert_* > calls? There's no 1:1 mapping from these sentences to actual code assertion. As I understand it, it's just saying, "these assertions are valid assertions to make because of these requirements in the spec" or conversely, "these statements in the spec are covered to some degree by this test". I'm not entirely sure how these assertions are used beyond that, i.e. if there is some automatic matching performed. > (If I'm understanding correctly, it seems like you could insert an assertion > and forget to add a new string to this array, and then we'd get the > textual-description wrong when there's a test-failure, and that would be > pretty confusing. I might be misunderstanding how this works, though.) Yes, I'm not really sure either. James? Ms2ger? > >+async_test(function(t) { > >+ var valueAtStart = document.timeline.currentTime; > >+ var timeAtStart = window.performance.now(); > >+ while (window.performance.now() - timeAtStart < 100) { > >+ // Wait 100ms > >+ } > > (Maybe declare these variables 'const' instead of 'var' to be clearer (& > enforce) that they're never gonna change? As per Ms2ger's comment below, I'll leave this as 'var' for now. > >+++ b/dom/animation/test/moz.build > >@@ -0,0 +1,7 @@ > >+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > >+# vim: set filetype=python: > >+# 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/. > >+ > >+MOCHITEST_MANIFESTS += ['mochitest.ini'] > > IIRC nowadays the build folks want to explicitly review any moz.build > changes, with a few small exceptions (for e.g. adding to an existing SOURCES > list). I don't think adding a new moz.build file (trivial as it may be) is > one of those exceptions, so you probably should have a build peer (maybe > mshal or glandium) sign off on this change. > > (Probably on the dom/animation/moz.build addition, too.) Ok. I've added it to dom/animation/moz.build as Ms2ger suggested. (In reply to :Ms2ger from comment #37) > Current practice is > > var idlArray; > setup(function() { > idlArray = new IdlArray(); > idlArray.add_idls( > document.getElementById('AnimationTimeline-IDL').textContent); > idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); > }); > idlArray.test(); Done. Out of curiosity, why is this preferred? > > >+++ b/dom/animation/test/moz.build > > >@@ -0,0 +1,7 @@ > > >+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > > >+# vim: set filetype=python: > > >+# 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/. > > >+ > > >+MOCHITEST_MANIFESTS += ['mochitest.ini'] > > > > IIRC nowadays the build folks want to explicitly review any moz.build > > changes, with a few small exceptions (for e.g. adding to an existing SOURCES > > list). I don't think adding a new moz.build file (trivial as it may be) is > > one of those exceptions, so you probably should have a build peer (maybe > > mshal or glandium) sign off on this change. > > > > (Probably on the dom/animation/moz.build addition, too.) > > There's no need to create a new moz.build here; you can add > MOCHITEST_MANIFESTS += ['test/mochitest.ini'] to dom/animation/moz.build. Great, done. Thanks!

(In reply to Brian Birtles (:birtles) from comment #39) > (In reply to James Graham [:jgraham] from comment #33) > > >+'document.timeline.currentTime value tests', > > >+{ > > >+ help: [ > > >+ 'http://dev.w3.org/fxtf/web-animations/#the-global-clock', > > >+ 'http://dev.w3.org/fxtf/web-animations/#the-document-timeline' > > >+ ], > > >+ assert: [ > > >+ 'The global clock is a source of monotonically increasing time values', > > >+ 'The time values of the document timeline are calculated as a fixed' + > > >+ ' offset from the global clock', > > >+ 'the zero time corresponds to the navigationStart moment', > > >+ 'the time value of each document timeline must be equal to the time ' + > > >+ 'passed to animation frame request callbacks for that browsing context' > > >+ ], > > > > Out of curiosity, what's the benefit of having this array of extra > > descriptive text in the "assert: " block here? Is there a 1:1 mapping > > between assert_* calls and entries in this array? If so, wouldn't it be > > simpler & less fragile for this text to be directly included in the assert_* > > calls? > > There's no 1:1 mapping from these sentences to actual code assertion. As I > understand it, it's just saying, "these assertions are valid assertions to > make because of these requirements in the spec" or conversely, "these > statements in the spec are covered to some degree by this test". I'm not > entirely sure how these assertions are used beyond that, i.e. if there is > some automatic matching performed. > > > (If I'm understanding correctly, it seems like you could insert an assertion > > and forget to add a new string to this array, and then we'd get the > > textual-description wrong when there's a test-failure, and that would be > > pretty confusing. I might be misunderstanding how this works, though.) > > Yes, I'm not really sure either. > > James? Ms2ger? It's just metadata. Occasionally people decide that they need to measure the coverage of tests relative to sentences in specs and then proceed to invent various schemes for linking the two, without fully considering the problems such metadata can cause (it is likely to become become stale, authors are less willing to write tests if it involves "useless" makework, etc.). If you want to use this for the web-animations spec feel free, but it absolutely isn't required.

Comment on attachment 8420813 [details] [diff] [review] part 5 - Add tests for AnimationTimeline (In reply to Brian Birtles (:birtles) from comment #39) > > >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html > > > > [...] it's a bit strange to see both underscore and hyphen > > alongside each other in a single filename. > > I think web-platform-tests tends to favor hyphens [...] > > I agree it's weird but the 'test_' bit is simply something our build system > requires (and something we should ultimately fix here). Hmm. So then would the idea be that we'd drop the "test_" prefix when uploading them to the web-platform-tests testsuite? I suppose this is fine; we can always rename them later if we have a better idea, I guess. > > Maybe worth also asserting that in each of these cases, document.timeline is > > an actual thing [...] > The idlharness.js test should pick that up. I originally had that check but > I was told it's better to use idlharness.js for that. I'm happy to re-add it > though. What do you think? Ah -- does idlharness.js automatically know to null-check these as a result of the... idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); ...statement? Also, does that check iframe.contentDocument.timeline, too? If so, great; if not, maybe consider adding a null-check for that one. > There's no 1:1 mapping from these sentences to actual code assertion. As I > understand it, it's just saying, "these assertions are valid assertions to > make because of these requirements in the spec" or conversely, "these > statements in the spec are covered to some degree by this test". I'm not > entirely sure how these assertions are used beyond that, i.e. if there is > some automatic matching performed. Gotcha. Sounds like these are optional, from Ms2ger's response, and aren't mapped in any particular way, so less fragile than I originally thought. So, I'm fine either way. r=me, with the above addressed (or not) as you see fit.

(er sorry, s/from Ms2ger's response/from jgraham's response/)

(In reply to Daniel Holbert [:dholbert] from comment #41) > Comment on attachment 8420813 [details] [diff] [review] > part 5 - Add tests for AnimationTimeline > > (In reply to Brian Birtles (:birtles) from comment #39) > > > >+++ b/dom/animation/test/animation-timeline/test_animation-timeline.html > > > > > > [...] it's a bit strange to see both underscore and hyphen > > > alongside each other in a single filename. > > > > I think web-platform-tests tends to favor hyphens [...] > > > > I agree it's weird but the 'test_' bit is simply something our build system > > requires (and something we should ultimately fix here). > > Hmm. So then would the idea be that we'd drop the "test_" prefix when > uploading them to the web-platform-tests testsuite? > > I suppose this is fine; we can always rename them later if we have a better > idea, I guess. Right. In fact, when we import tests from there we prepend 'test_' to the start: http://dxr.mozilla.org/mozilla-central/source/dom/imptests/importTestsuite.py#111 > > > Maybe worth also asserting that in each of these cases, document.timeline is > > > an actual thing > [...] > > The idlharness.js test should pick that up. I originally had that check but > > I was told it's better to use idlharness.js for that. I'm happy to re-add it > > though. What do you think? > > Ah -- does idlharness.js automatically know to null-check these as a result > of the... > idlArray.add_objects( { AnimationTimeline: ['document.timeline'] } ); > ...statement? Yes, that's my understanding and if I sub in 'null' above it fails. > Also, does that check iframe.contentDocument.timeline, too? If so, great; if > not, maybe consider adding a null-check for that one. Good idea! > > There's no 1:1 mapping from these sentences to actual code assertion. As I > > understand it, it's just saying, "these assertions are valid assertions to > > make because of these requirements in the spec" or conversely, "these > > statements in the spec are covered to some degree by this test". I'm not > > entirely sure how these assertions are used beyond that, i.e. if there is > > some automatic matching performed. > > Gotcha. Sounds like these are optional, from Ms2ger's response, and aren't > mapped in any particular way, so less fragile than I originally thought. > So, I'm fine either way. Yes, based on jgraham's comment I should probably go and trim these quotations but that can probably happen once they land over in web-platform-tests. Thanks Daniel!

Comment on attachment 8416298 [details] [diff] [review] part 2 - Add AnimationTimeline interface; Hi Mike, This patch adds a moz.build file and modifies another. Would you be able to check it is ok? Boris has already reviewed the rest of the patch. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd2d6d15c1e https://hg.mozilla.org/integration/mozilla-inbound/rev/f3276ed702c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0cd3c9f6713 https://hg.mozilla.org/integration/mozilla-inbound/rev/02e11e154e77 https://hg.mozilla.org/integration/mozilla-inbound/rev/733707bc889b https://hg.mozilla.org/integration/mozilla-inbound/rev/2c09731b4dc4

https://hg.mozilla.org/mozilla-central/rev/3cd2d6d15c1e https://hg.mozilla.org/mozilla-central/rev/f3276ed702c4 https://hg.mozilla.org/mozilla-central/rev/a0cd3c9f6713 https://hg.mozilla.org/mozilla-central/rev/02e11e154e77 https://hg.mozilla.org/mozilla-central/rev/733707bc889b https://hg.mozilla.org/mozilla-central/rev/2c09731b4dc4