1612106 - Add a separate pref for read-only Animation.timeline

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Boris Chiou [:boris]]

This also makes setting timelines a separate pref. Webkit is also working on this now, so it'd be great if we can ship read-only animation.timeline sooner.

Comment 1

[Brian Birtles (:birtles)]

We might also want to mark timeline as [SecureContext] depending on what Safari ships.

Comment 2

[Boris Chiou [:boris]]

I'm thinking what is our expectation for this bug. The current spec defines timeline as a readonly attribute, and Gecko supports writable timeline with the function: Document::AreWebAnimationsTimelinesEnabled(). It seems there is no easy way to add a pref for Setter-only or Getter-only in WebIDL. So perhaps there are some options we can do:

• Option 1: Add a new pref, e.g. dom.animations-api.writable-timelines.enabled, and check it in Animation::SetTimeline(). We throw an error or just do nothing (i.e. no-op) if the pref is off. Therefore, we can just enable dom.animations-api.timelines.enabled for the read-only animation timeline. The benefit is: it seems we have to enable this pref for all the timeline related attributes, so this may make the patch cleaner.

  • Or maybe we just rename dom.animations-api.timelines.enabled to something like dom.animations-api.readonly-timelines.enabled, and keep the new pref as dom.animations-api.writable-timelines.enabled. And then let Document::AreWebAnimationsTimelinesEnabled() return true if we set any of the two prefs to true. However, this still exposes the no-op SetTimeline() if dom.animations-api.writable-timelines.enabled is false.

• Option 2: Change the attribute of timeline in Animation to readonly attribute AnimationTimeline? timeline, and drop the setter. It seems the setter is pretty simple, so we can add it back later. However, this needs to update wpt.

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1612106 - Use NIGHTLY build flag to distinguish the writable/readonly Animation.timeline.

Comment 4

[Brian Birtles (:birtles)]

I think we want to keep the writeable timeline attribute available behind a pref so we can iterate on that implementation (which we will need for implementing ScrollTimeline in the next few months).

As for how to best hide the timeline setter via a pref while, I wonder if bz might have some ideas?

I think we want to end up with two prefs:

• dom.animations-api.timelines.enabled -- Enables the Animation.timeline getter, Document.timeline, and the DocumentTimeline and AnimationTimeline interfaces. We will turn this on by default in 75 but we still need it in case we need to disable this feature.
• dom.animations-api.writeable-timeline.enabled -- Enables the Animation.timeline setter. Turned on by default in Nightly only.

If dom.animations-api.writeable-timeline.enabled is set, but dom.animations-api.timelines.enabled is not, what should happen? I think it would be ok to do whatever is easiest to implement.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Without changes to codegen, the best we can do is have the attribute writable in the IDL and then have the setter in the C++ throw or no-op. Neither one is perfect, unfortunately. The behavior of an actual readonly attribute is to throw on set in strict mode and no-op in non-strict, but we don't really have access to the strict mode here, I don't think. So no-op may be safest... But you'd only reach this code if the passed-in value is an AnimationTimeline or null; everything else would throw.

With changes to the codegen... we could somewhat easily throw or no-op in the IDL setter before doing the argument coercion. That would minimize the risk of unexpected exceptions, at least.

Doing the complete "setter is undefined" thing via pref would require quite a bit of codegen surgery. Right now, codegen basically just passes a data table to SpiderMonkey, which SpiderMonkey then uses to create the properties on the prototype. We can't really inject pref checks into that process. We could change that mechanism around in various ways, but that's pretty nontrivial work.

One relevant question: how important is it to have this be a pref? Doing a build-time ifdef for this is easy, if that helps at all...

Comment 6

[Brian Birtles (:birtles)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)

One relevant question: how important is it to have this be a pref? Doing a build-time ifdef for this is easy, if that helps at all...

Maybe that would be ok. Provided we can #define it for Nightly builds only I think that would still give us the necessary test coverage for WPT particularly as we come to work on ScrollTimelines. I believe WPT test expectations can be set per release stage too so that should be ok.

(https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests says, "The available set of condition variables is that provided by mozinfo plus a boolean variable e10s that is true when e10s is enabled" and links to https://firefox-source-docs.mozilla.org/mozbase/mozinfo.html which simply says "The current implementation exposes relevant keys and values such as: os, version, bits, and processor. Additionally, the service pack in use is available on the windows platform." All outgoing links from there 404 but looking in mozilla-central I can see a few not nightly_build annotations (e.g. [1]) so I'm hopeful this is possible.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Provided we can #define it for Nightly builds only

That's easy enough. https://searchfox.org/mozilla-central/rev/5a10be606f2d76ef22f1f44565749490de991d35/dom/webidl/Window.webidl#513-515 is an example.

And yes, you can set wpt expectatons based on channel.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/922d1a0e3090
Use NIGHTLY build flag to distinguish the writable/readonly Animation.timeline. r=birtles,bzbarsky

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/922d1a0e3090