Closed
Bug 1141710
Opened 9 years ago
Closed 9 years ago
Multiple updates to the css-animations version of test_animation-player-starttime.html to align with comments in bug 1072037
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jwatt, Assigned: jwatt)
Details
Attachments
(14 files, 1 obsolete file)
7.88 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.91 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
4.83 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
10.93 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
7.70 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
8.39 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
3.05 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
2.59 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
1.95 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
7.35 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
5.20 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
In bug 1072037 there are multiple comments related to the test there that should also be applied to the css-animations version of test_animation-player-starttime.html.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8575517 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8575519 -
Flags: review?(dholbert)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8575520 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8575521 -
Flags: review?(dholbert)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8575522 -
Flags: review?(dholbert)
Updated•9 years ago
|
Attachment #8575517 -
Flags: review?(dholbert) → review+
Updated•9 years ago
|
Attachment #8575519 -
Flags: review?(dholbert) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8575520 [details] [diff] [review] part 3 ># HG changeset patch ># User Jonathan Watt <jwatt@jwatt.org> ># Parent 2bd036dbf27778dc5ffa4953d6c1a0ecb4abfa5b >Bug 1141710, part 3 - Stop using ECMAScript 6 features, since other browsers don't support them. r=dholbert After "features", add: in test_animation-player-starttime.html ...or: in css animation test ...to make it clearer that this is a pretty targeted removal. (Otherwise it kinda sounds like it's removing these features across the board.)
Attachment #8575520 -
Flags: review?(dholbert) → review+
Updated•9 years ago
|
Attachment #8575521 -
Flags: review?(dholbert) → review+
Updated•9 years ago
|
Attachment #8575522 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8575555 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•9 years ago
|
||
Updated per IRC discussion.
Attachment #8575555 -
Attachment is obsolete: true
Attachment #8575555 -
Flags: review?(dholbert)
Attachment #8575576 -
Flags: review?(dholbert)
Comment 9•9 years ago
|
||
Comment on attachment 8575576 [details] [diff] [review] part 6 Review of attachment 8575576 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following: ::: dom/animation/test/css-animations/test_animation-player-starttime.html @@ +43,5 @@ > const ANIM_DELAY_MS = 1000000; // 1000s > const ANIM_DUR_MS = 1000000; // 1000s > > +/** > + * These helpers get the value that the startTime needs to be set to to put an comma after first "to", to make this read easier. @@ +435,4 @@ > async_test(function(t) { > + var div = addDiv(t, {'class': 'animated-div'}); > + var eventWatcher = new EventWatcher(div, CSS_ANIM_EVENTS); > + div.style.animation = 'anim ' + ANIM_DUR_MS + 'ms ' + ANIM_DELAY_MS + 'ms'; Let's put this string in a global "const" variable, e.g. ANIM_PROPERTY_VAL, so we don't have to piece this string together exactly right in every single async_test here. ::: dom/animation/test/testcommon.js @@ +59,5 @@ > + }); > + }); > + }); > +} > + Nit: this change is adding an extra blank line at the bottom of the file here -- drop that final blank-line.
Attachment #8575576 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8575594 -
Flags: review?(dholbert)
Comment 11•9 years ago
|
||
Comment on attachment 8575594 [details] [diff] [review] part 8 ># HG changeset patch ># User Jonathan Watt <jwatt@jwatt.org> ># Parent 4614205cd2eb35a884966c7d19d1f0d4d077d90e >Bug 1141710, part 7 - Create helpers to get the startTime corresponding to various points through the active duration. r=dholbert "Also, standardize on > >diff --git a/dom/animation/test/css-animations/test_animation-player-starttime.html b/dom/animation/test/css-animations/test_animation-player-starttime.html >--- a/dom/animation/test/css-animations/test_animation-player-starttime.html >+++ b/dom/animation/test/css-animations/test_animation-player-starttime.html >@@ -52,16 +52,28 @@ function startTimeForBeforePhase(timelin > return timeline.currentTime - ANIM_DELAY_MS / 2; > } > function startTimeForActivePhase(timeline) { > return timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS / 2; > } > function startTimeForAfterPhase(timeline) { > return timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS - ANIM_DELAY_MS / 2; > } >+function startTimeForStartOfActiveInterval(timeline) { >+ return timeline.currentTime - ANIM_DELAY_MS; >+} Please extend the existing documentation for these new functions. (The documentation you add in previous patch is pretty explicitly just about the first 3 functions -- "before, active and after phases, respectively" (or add a new block of documentation between startTimeForAfterPhase and startTimeForStartOfActiveInterval) >+function startTimeForNintyPercentThroughActiveInterval(timeline) { >+ return timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS * 0.9; >+} Typo: s/Ninty/Ninety/ >+function startTimeForFiftyPercentThroughActiveInterval(timeline) { >+ return timeline.currentTime - ANIM_DELAY_MS - ANIM_DUR_MS * 0.5; >+} Observation: this looks identical to startTimeForActivePhase(), though with "* 0.5" instead of "/ 2". Maybe worth acknowledging that in a parenthetical, in a comment. (You could combine them, but it looks like you're using them for different things, & they probably make sense being separate.)
Attachment #8575594 -
Flags: review?(dholbert) → review+
Comment 12•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #11) > >Bug 1141710, part 7 - Create helpers to get the startTime corresponding to various points through the active duration. r=dholbert > > "Also, standardize on Sorry, that was a half-thought that I thought I'd removed. (I was going to suggest adding: "also, standardize on using player.timeline instead of document.timeline" to the commit message, maybe in a second line -- since some of that patch is about that, & that's unrelated to these new helper-functions. Feel free to take or ignore this suggestion as you see fit.)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #12) > (In reply to Daniel Holbert [:dholbert] from comment #11) > > >Bug 1141710, part 7 - Create helpers to get the startTime corresponding to various points through the active duration. r=dholbert > > > > "Also, standardize on > > Sorry, that was a half-thought that I thought I'd removed. (I was going to > suggest adding: "also, standardize on using player.timeline instead of > document.timeline" to the commit message, maybe in a second line -- since > some of that patch is about that, & that's unrelated to these new > helper-functions. Feel free to take or ignore this suggestion as you see > fit.) Actually I'll split that part out into a separate commit since that's probably better and helps me see what changes I need to make to the transitions tests to sync it up too. (In reply to Daniel Holbert [:dholbert] from comment #9) > @@ +435,4 @@ > > async_test(function(t) { > > + var div = addDiv(t, {'class': 'animated-div'}); > > + var eventWatcher = new EventWatcher(div, CSS_ANIM_EVENTS); > > + div.style.animation = 'anim ' + ANIM_DUR_MS + 'ms ' + ANIM_DELAY_MS + 'ms'; > > Let's put this string in a global "const" variable, e.g. ANIM_PROPERTY_VAL, > so we don't have to piece this string together exactly right in every single > async_test here. I've done the same with this FWIW (put it in its own commit) since that patch was already complex enough.
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8575625 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8575594 -
Attachment description: part 7 → part 8
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8575626 -
Flags: review+
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd5ac448c46 https://hg.mozilla.org/integration/mozilla-inbound/rev/255c9236bb37 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee29275a1748 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac29161a0f98 https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf9f153d5bc https://hg.mozilla.org/integration/mozilla-inbound/rev/e1330135150d https://hg.mozilla.org/integration/mozilla-inbound/rev/9496b1468d9c https://hg.mozilla.org/integration/mozilla-inbound/rev/c67f9c7dfd4e https://hg.mozilla.org/integration/mozilla-inbound/rev/0e360d3f690b
All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/24872a839d9d for mochitest-1 bustage, along with bug 1141498: https://treeherder.mozilla.org/logviewer.html#?job_id=7437755&repo=mozilla-inbound
Flags: needinfo?(jwatt)
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df94b3368c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/263431cbffb7 https://hg.mozilla.org/integration/mozilla-inbound/rev/3427f7cbfc35 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c05cadcbe6 https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa39b879607 https://hg.mozilla.org/integration/mozilla-inbound/rev/428d97e00258 https://hg.mozilla.org/integration/mozilla-inbound/rev/7add863847c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/32b7fb0be1e3 https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c21f24c2c8
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8575767 -
Flags: review?(bbirtles)
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8575768 -
Flags: review?(bbirtles)
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8575779 -
Flags: review?(bbirtles)
Updated•9 years ago
|
Attachment #8575767 -
Flags: review?(bbirtles) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8575768 [details] [diff] [review] part 11 Review of attachment 8575768 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with that comment addressed. ::: dom/animation/test/css-animations/test_animation-player-starttime.html @@ +400,5 @@ > + assert_true(document.timeline.currentTime - previousTimelineTime < > + ANIMATION_DUR_MS, > + 'Sanity check that seeking worked rather than the events ' + > + 'firing after normal playback through the very long ' + > + 'ANIM_DELAY_MS + ANIM_DUR_MS'); I think this should be ANIMATION_DUR_MS in the first instance. Nit: For the comment, please drop the reference to ANIM_DELAY_MS + ANIM_DUR_MS and replace with something descriptive (e.g. '... the very long animation duration') or its numeric value (i.e. don't put it in quotes).
Attachment #8575768 -
Flags: review?(bbirtles) → review+
Updated•9 years ago
|
Attachment #8575779 -
Flags: review?(bbirtles) → review+
Comment 24•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #23) > ::: dom/animation/test/css-animations/test_animation-player-starttime.html > @@ +400,5 @@ > > + assert_true(document.timeline.currentTime - previousTimelineTime < > > + ANIMATION_DUR_MS, > > + 'Sanity check that seeking worked rather than the events ' + > > + 'firing after normal playback through the very long ' + > > + 'ANIM_DELAY_MS + ANIM_DUR_MS'); > > I think this should be ANIMATION_DUR_MS in the first instance. Sorry, I think it should be *ANIM_DUR_MS* in the first instance.
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8575781 -
Flags: review?(bbirtles)
Comment 26•9 years ago
|
||
Comment on attachment 8575781 [details] [diff] [review] part 13 Review of attachment 8575781 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful!
Attachment #8575781 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8575784 -
Flags: review?(bbirtles)
Updated•9 years ago
|
Attachment #8575784 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f66ce3b461 https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea7a6646027 https://hg.mozilla.org/integration/mozilla-inbound/rev/b791fa5fdf39 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5e8e18f74a https://hg.mozilla.org/integration/mozilla-inbound/rev/685afe4932f7
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8df94b3368c8 https://hg.mozilla.org/mozilla-central/rev/263431cbffb7 https://hg.mozilla.org/mozilla-central/rev/3427f7cbfc35 https://hg.mozilla.org/mozilla-central/rev/b7c05cadcbe6 https://hg.mozilla.org/mozilla-central/rev/3fa39b879607 https://hg.mozilla.org/mozilla-central/rev/428d97e00258 https://hg.mozilla.org/mozilla-central/rev/7add863847c6 https://hg.mozilla.org/mozilla-central/rev/32b7fb0be1e3 https://hg.mozilla.org/mozilla-central/rev/d5c21f24c2c8
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4f66ce3b461 https://hg.mozilla.org/mozilla-central/rev/1ea7a6646027 https://hg.mozilla.org/mozilla-central/rev/b791fa5fdf39 https://hg.mozilla.org/mozilla-central/rev/3d5e8e18f74a https://hg.mozilla.org/mozilla-central/rev/685afe4932f7
Flags: in-testsuite+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•