Closed Bug 1141710 Opened 6 years ago Closed 6 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)

defect
Not set
normal

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.
Attached patch part 1Splinter Review
Attachment #8575517 - Flags: review?(dholbert)
Attached patch part 2Splinter Review
Attachment #8575519 - Flags: review?(dholbert)
Attached patch part 3Splinter Review
Attachment #8575520 - Flags: review?(dholbert)
Attached patch part 4Splinter Review
Attachment #8575521 - Flags: review?(dholbert)
Attached patch part 5Splinter Review
Attachment #8575522 - Flags: review?(dholbert)
Attachment #8575517 - Flags: review?(dholbert) → review+
Attachment #8575519 - Flags: review?(dholbert) → review+
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+
Attachment #8575521 - Flags: review?(dholbert) → review+
Attachment #8575522 - Flags: review?(dholbert) → review+
Attached patch part 6 (obsolete) — Splinter Review
Attachment #8575555 - Flags: review?(dholbert)
Attached patch part 6Splinter Review
Updated per IRC discussion.
Attachment #8575555 - Attachment is obsolete: true
Attachment #8575555 - Flags: review?(dholbert)
Attachment #8575576 - Flags: review?(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.
Attachment #8575576 - Flags: review?(dholbert) → review+
Attached patch part 8Splinter Review
Attachment #8575594 - Flags: review?(dholbert)
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+
(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.)
(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.
Attachment #8575625 - Flags: review+
Attachment #8575594 - Attachment description: part 7 → part 8
Attachment #8575626 - Flags: review+
Not this bugs fault.
Flags: needinfo?(jwatt)
Attached patch part 10Splinter Review
Attachment #8575767 - Flags: review?(bbirtles)
Attached patch part 11Splinter Review
Attachment #8575768 - Flags: review?(bbirtles)
Attached patch part 12Splinter Review
Attachment #8575779 - Flags: review?(bbirtles)
Attachment #8575767 - Flags: review?(bbirtles) → review+
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+
Attachment #8575779 - Flags: review?(bbirtles) → review+
(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.
Attached patch part 13Splinter Review
Attachment #8575781 - Flags: review?(bbirtles)
Comment on attachment 8575781 [details] [diff] [review]
part 13

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

Beautiful!
Attachment #8575781 - Flags: review?(bbirtles) → review+
Attached patch part 14Splinter Review
Attachment #8575784 - Flags: review?(bbirtles)
Attachment #8575784 - Flags: review?(bbirtles) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.