1141710 - Multiple updates to the css-animations version of test_animation-player-starttime.html to align with comments in bug 1072037

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 3

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 4

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 5

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8575520 [details] [diff] [review]
part 3

># HG changeset patch
># User Jonathan Watt <[e-mail]>
># 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.)

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 6

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 6

Updated per IRC discussion.

Comment 9

[Daniel Holbert [:dholbert]]

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.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: part 8

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8575594 [details] [diff] [review]
part 8

># HG changeset patch
># User Jonathan Watt <[e-mail]>
># 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.)

Comment 12

[Daniel Holbert [:dholbert]]

(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.)

Comment 13

[Jonathan Watt [:jwatt]]

(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.

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: part 7 [r=dholbert]

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: part 9 [r=dholbert]

Comment 16

[Jonathan Watt [:jwatt]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Jonathan Watt [:jwatt]]

Not this bugs fault.

Comment 19

[Jonathan Watt [:jwatt]]

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

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: part 10

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: part 11

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: part 12

Comment 23

[Brian Birtles (:birtles)]

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).

Comment 24

[Brian Birtles (:birtles)]

(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.

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: part 13

Comment 26

[Brian Birtles (:birtles)]

Comment on attachment 8575781 [details] [diff] [review]
part 13

Review of attachment 8575781 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful!

Comment 27

[Jonathan Watt [:jwatt]]

Attachment: part 14

Comment 28

[Jonathan Watt [:jwatt]]

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

[Carsten Book [:Tomcat]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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