Make timeline argument of Animation constructor default to the document timeline of the active document

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: birtles, Assigned: daisuke)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

I think that we should modify some tests.
Some test expected that timeline will be null when specified one parameter only.
e.g. 
  web-animation/timing-model/animations/set-the-an-animation-start-time.html
  This test isn't setting the second parameters. then this test expect that timeline will be null.
(Assignee)

Updated

2 years ago
Assignee: nobody → daisuke
(Assignee)

Comment 4

2 years ago
Created attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

Review commit: https://reviewboard.mozilla.org/r/55028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55028/
Attachment #8756213 - Flags: review?(bbirtles)
Attachment #8756214 - Flags: review?(bbirtles)
(Assignee)

Comment 5

2 years ago
Created attachment 8756214 [details]
MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r=birtles

Review commit: https://reviewboard.mozilla.org/r/55030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55030/
(Reporter)

Updated

2 years ago
Attachment #8756213 - Flags: review?(bbirtles)
(Reporter)

Comment 6

2 years ago
Comment on attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

https://reviewboard.mozilla.org/r/55028/#review51700

Looks fine but I think we can use just a single constructor? I think we'd only need to update Element::Animate?

::: dom/animation/Animation.h:93
(Diff revision 1)
>    static already_AddRefed<Animation>
>    Constructor(const GlobalObject& aGlobal,
>                KeyframeEffectReadOnly* aEffect,
> +              const Optional<AnimationTimeline*>& aTimeline,
> +              ErrorResult& aRv);
> +  static already_AddRefed<Animation>
> +  Constructor(const GlobalObject& aGlobal,
> +              KeyframeEffectReadOnly* aEffect,
>                AnimationTimeline* aTimeline,
>                ErrorResult& aRv);

It seems like it would be easier to follow this code if we just had the one constructor?

How much other code would we have to change?

::: dom/animation/Animation.cpp:96
(Diff revision 1)
> +  nsIDocument* document =
> +    AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> +  AnimationTimeline* timeline =
> +    document ? (AnimationTimeline*) document->Timeline() : nullptr;

I think we should probably just throw if document is null since we don't expect that to be the case.

(Also we shouldn't need to cast document->Timeline to AnimationTimeline*)

::: dom/webidl/Animation.webidl:30
(Diff revision 1)
>    [BinaryName="startTimeAsDouble"]
>    attribute double? startTime;
>    [SetterThrows, BinaryName="currentTimeAsDouble"]
>    attribute double? currentTime;
>  
> -           attribute double             playbackRate;
> +  attribute double             playbackRate;

I think we should just leave this line.
(Assignee)

Comment 8

2 years ago
Comment on attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55028/diff/1-2/
Attachment #8756213 - Flags: review?(bbirtles)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8756214 [details]
MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55030/diff/1-2/
(Reporter)

Comment 10

2 years ago
Comment on attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

https://reviewboard.mozilla.org/r/55028/#review52106

r=me with the following changes

::: dom/animation/Animation.cpp:110
(Diff revision 2)
> +    timeline = aTimeline.Value();
> +  } else {
> +    nsIDocument* document =
> +      AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
> +    if (!document) {
> +      aRv.Throw(NS_ERROR_DOM_ANIM_NO_ACTIVE_DOCUMENT_ERR);

Are we expecting this to happen? I think elsewhere we just throw NS_ERROR_FAILURE when this is false.

(If we are concerned about this being called from chrome and that we should do something sensible in that case, then we should fix that everywhere in a separate bug.)

::: dom/base/domerr.msg:122
(Diff revision 2)
>  
>  /* Web Animations errors */
>  
>  DOM4_MSG_DEF(NotSupportedError, "Animation to or from an underlying value is not yet supported.", NS_ERROR_DOM_ANIM_MISSING_PROPS_ERR)
>  DOM4_MSG_DEF(NotSupportedError, "Animation with no effect is not yet supported.", NS_ERROR_DOM_ANIM_NO_EFFECT_ERR)
> +DOM4_MSG_DEF(NotSupportedError, "Timeline could not get since no active document.", NS_ERROR_DOM_ANIM_NO_ACTIVE_DOCUMENT_ERR)

(i.e. I don't think we need this)

::: dom/webidl/Animation.webidl:17
(Diff revision 2)
>  
>  enum AnimationPlayState { "idle", "pending", "running", "paused", "finished" };
>  
>  [Func="nsDocument::IsElementAnimateEnabled",
>   Constructor (optional KeyframeEffectReadOnly? effect = null,
> -              optional AnimationTimeline? timeline = null)]
> +              optional AnimationTimeline? timeline)]

(This will need DOM peer review.)

::: xpcom/base/ErrorList.h:957
(Diff revision 2)
>    /* 39: NS_ERROR_MODULE_DOM_ANIM */
>    /* ======================================================================= */
>  #define MODULE NS_ERROR_MODULE_DOM_ANIM
>    ERROR(NS_ERROR_DOM_ANIM_MISSING_PROPS_ERR,              FAILURE(1)),
>    ERROR(NS_ERROR_DOM_ANIM_NO_EFFECT_ERR,                  FAILURE(2)),
> +  ERROR(NS_ERROR_DOM_ANIM_NO_ACTIVE_DOCUMENT_ERR,         FAILURE(3)),

(We won't need this either)
Attachment #8756213 - Flags: review?(bbirtles) → review+
(Reporter)

Updated

2 years ago
Attachment #8756214 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 11

2 years ago
Comment on attachment 8756214 [details]
MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r=birtles

https://reviewboard.mozilla.org/r/55030/#review52110

r=me with the following changes.

Also, we should probably add a test that we are getting the timeline for the active document for the context the script is running in, as opposed to say, the owner document of the effect's target element. Maybe that's unlikely since effect can be null?

See [1] for an example although in this case I guess we'd want to create an iframe, set up a keyframe effect that targets an element in the iframe, create an Animation using the effect, then assert that animation.timeline is NOT equal to animation.effect.target.ownerDocument.timeline ? (i.e. not equal to iframe.contentDocument.timeline)

[1] https://dxr.mozilla.org/mozilla-central/rev/d6d4e8417d2fd71fdf47c319b7a217f6ace9d5a5/testing/web-platform/tests/web-animations/interfaces/Animatable/animate.html#96

::: testing/web-platform/tests/web-animations/interfaces/Animation/constructor.html:63
(Diff revision 2)
> -    var animation = new Animation(args.effect, args.timeline);
> +    var animation = args.timeline === undefined
> +                    ? new Animation(args.effect)
> +                    : new Animation(args.effect, args.timeline);

We don't need this change. The following are equivalent:

  new Animation(args.effect, undefined)
  new Animation(args.effect)

::: testing/web-platform/tests/web-animations/interfaces/Animation/constructor.html:73
(Diff revision 2)
> -                  "Animation returns the same timeline passed to " +
> +                  "Animation returns " + args.expectedTimelineDescription);
> -                  "the Constructor");

If this fails, we'll see something like:

  "Animation returns null"

It would be better if it said something like:

  "Animation timeline should be null"
(Assignee)

Comment 12

2 years ago
Comment on attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55028/diff/2-3/
Attachment #8756213 - Attachment description: MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r?birtles → MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r?smaug
Attachment #8756214 - Attachment description: MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r?birtles → MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r=birtles
Attachment #8756213 - Flags: review?(bugs)
(Assignee)

Comment 13

2 years ago
Comment on attachment 8756214 [details]
MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55030/diff/2-3/
(Assignee)

Comment 14

2 years ago
Hello Olli,
Could you review dom/webidl/Animation.webidl?
https://reviewboard.mozilla.org/r/55028/diff/3#3

Thanks!
Comment on attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

https://reviewboard.mozilla.org/r/55028/#review52632
Attachment #8756213 - Flags: review?(bugs) → review+
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 16

2 years ago
Comment on attachment 8756213 [details]
MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55028/diff/3-4/
Attachment #8756213 - Attachment description: MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r?smaug → MozReview Request: Bug 1272211 - Part 1: Make timeline argument of Animation constructor default to the document timeline of the active document. r=birtles, r=smaug
(Assignee)

Comment 17

2 years ago
Comment on attachment 8756214 [details]
MozReview Request: Bug 1272211 - Part 2: Modified and append tests. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55030/diff/3-4/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cdeb875c776
https://hg.mozilla.org/mozilla-central/rev/8acb4ca03d6a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.