Bug 99116 – [RequestAnimationFrame] Remove vendor prefix

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Simonsen]

[RequestAnimationFrame] Remove vendor prefix

[James Simonsen]

Created attachment 168312 [details]
Patch

[James Robinson]

Comment on attachment 168312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168312&action=review

R=me

> Source/WebCore/page/DOMWindow.idl:218
>          // WebKit animation extensions, being standardized in the WebPerf WG

probably worth updating this comment

[Adam Barth]

Comment on attachment 168312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168312&action=review

> Source/WebCore/page/DOMWindow.idl:-221
> -        long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
> -        void webkitCancelAnimationFrame(in long id);
> -        void webkitCancelRequestAnimationFrame(in long id); // This is a deprecated alias for webkitCancelAnimationFrame(). Remove this when removing vendor prefix.

Do we want to expose both sets of APIs for a while?

[Adam Barth]

Comment on attachment 168312 [details]
Patch

This patch seems a bit aggressive for this API.  Have we measured the usage of the prefixed versions?  Will this patch break sites?

[Adam Barth]

IMHO, we should expose both versions for a while and use V8MeasureAs to measure how quickly web sites are transitioning from the prefixed to the unprefixed versions.

[Adam Barth]

See http://lists.webkit.org/pipermail/webkit-dev/2012-September/022239.html for instructions about how to use V8MeasureAs and FeatureObserver.

[James Simonsen]

There were a couple of reasons for making the transition.

1. Everyone else is already (or in the process of) shipping unprefixed.
2. We've changed our semantics to match the spec. So the old webkit* functions aren't compatible.
3. The major toolkits I checked already use the unprefixed version.

[Adam Barth]

Would you be willing to email webkit-dev with this information?  Different folks in the community have different risk tolerances for these sorts of changes, and it's better to talk things through so that folks don't feel railroaded in one direction or another.

Here's a wiki page we wrote up based on previous discussions of similar changes:

https://trac.webkit.org/wiki/DeprecatingFeatures

[James Simonsen]

Created attachment 168445 [details]
Patch

[Adam Barth]

Comment on attachment 168445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168445&action=review

> Source/WebCore/page/DOMWindow.h:270
> +        void webkitCancelAnimationFrame(int id) { cancelAnimationFrame(id); }
> +        void webkitCancelRequestAnimationFrame(int id) { cancelAnimationFrame(id); }

You can just use ImplementedAs in the IDL rather than having these stub functions here.

> Source/WebCore/page/DOMWindow.idl:216
> +    // W3C Web Performance WG Request Animation Frame standard.

I would skip this comment.  It's not adding any information.

> Source/WebCore/page/DOMWindow.idl:221
> +    long requestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
> +    void cancelAnimationFrame(in long id);
>      long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
>      void webkitCancelAnimationFrame(in long id);
>      void webkitCancelRequestAnimationFrame(in long id); // This is a deprecated alias for webkitCancelAnimationFrame(). Remove this when removing vendor prefix.

Please add V8MeasureAs attributes so that we can measure how often the prefixed versions are used.  That way we'll know have more information about when we can remove them.

[James Simonsen]

Created attachment 168457 [details]
Patch

[Adam Barth]

Comment on attachment 168457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168457&action=review

> Source/WebCore/page/DOMWindow.idl:220
> +    long requestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
> +    void cancelAnimationFrame(in long id);
> +    [ImplementedAs=requestAnimationFrame, V8MeasureAs=PrefixedRequestAnimationFrame] long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
> +    [ImplementedAs=cancelAnimationFrame] void webkitCancelAnimationFrame(in long id);
> +    [ImplementedAs=cancelAnimationFrame] void webkitCancelRequestAnimationFrame(in long id); // This is a deprecated alias for webkitCancelAnimationFrame(). Remove this when removing vendor prefix.

Should we measure the unprefixed one as well, so we can compare, like we do for IndexedDB?

[James Simonsen]

Created attachment 168460 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168460 [details]
Patch for landing

Clearing flags on attachment: 168460

Committed r131214: <http://trac.webkit.org/changeset/131214>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pablo Flouret]

Breaks gtk? http://queues.webkit.org/results/14267238

[Adam Barth]

Sounds like the CPP bindings need to understand ImplementedAs.

DerivedSources/webkit/WebKitDOMDOMWindow.cpp: In function 'void webkit_dom_dom_window_webkit_cancel_animation_frame(WebKitDOMDOMWindow*, glong)':
DerivedSources/webkit/WebKitDOMDOMWindow.cpp:1116:11: error: 'class WebCore::DOMWindow' has no member named 'webkitCancelAnimationFrame'
DerivedSources/webkit/WebKitDOMDOMWindow.cpp: In function 'void webkit_dom_dom_window_webkit_cancel_request_animation_frame(WebKitDOMDOMWindow*, glong)':
DerivedSources/webkit/WebKitDOMDOMWindow.cpp:1125:11: error: 'class WebCore::DOMWindow' has no member named 'webkitCancelRequestAnimationFrame'

[James Simonsen]

(In reply to comment #17)
> Sounds like the CPP bindings need to understand ImplementedAs.

Is someone going to do that or should I go back to the old way of forwarding?

[Pablo Flouret]

(In reply to comment #18)
> (In reply to comment #17)
> > Sounds like the CPP bindings need to understand ImplementedAs.
> 
> Is someone going to do that or should I go back to the old way of forwarding?

I have a patch almost ready. Filed bug 99214.

[Simon Fraser (smfr)]

The changelog and commit message don't say if the prefixed version was retained. Consensus on webkit-dev was to keep both prefixed and unprefixed.

[Simon Fraser (smfr)]

This patch should have been r- because of the lack of details in the changlog.

[Adam Barth]

> This patch should have been r- because of the lack of details in the changlog.

Thanks for the reminder.  I was somewhat distracted today with other disasters.  The code in the patch should be correct, however.