Bug 111244 – Animations fail to start on http://www.google.com/insidesearch/howsearchworks/thestory/

• Component: New Bugs
• Status: RESOLVED CONFIGURATION CHANGED

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/131131 seems to have broken http://www.google.com/insidesearch/howsearchworks/thestory/

The page loads, but the animations don't start.

[Simon Fraser (smfr)]

<rdar://problem/13330885>

[Simon Fraser (smfr)]

[e-mail], do you plan to look at this?

[Nat Duca]

Iirw, we tweaked that patch such that webkitRequestAnimFrame delivers a DOMTimestamp [old style] and the unprefixed requestAnimationFrame delivers a DOMHighResolutionTimestamp. @simonjam can confirm...

[James Simonsen]

Exactly right.

https://bugs.webkit.org/show_bug.cgi?id=106697

We'd love to take out the old behavior though. We should track down whoever owns that and ask them to update.

[Andy Estes]

(In reply to comment #4)
> Exactly right.
> 
> https://bugs.webkit.org/show_bug.cgi?id=106697
> 
> We'd love to take out the old behavior though. We should track down whoever owns that and ask them to update.

Are you saying that r139509 should have fixed this bug, too? It still looks broken to me in ToT.

[James Simonsen]

It looks like r139509 won't help here. This site has a proper polyfill, so it should pick up the unprefixed requestAnimationFrame() first, which is based on performance.now() values.

Does ToT have window.performance.now() defined? Without that, it'd definitely be broken.

If that's the case, we need to either enable performance.now(), or disable requestAnimationFrame() and only provide webkitRequestAnimationFrame() if now() isn't available.

[zalan]

Created attachment 199877 [details]
Patch

[zalan]

Comment on attachment 199877 [details]
Patch

missing testcase

[zalan]

Created attachment 200004 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 200004 [details]
Patch

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

r=me, but consider the feedback.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:183
> +ENABLE_WEB_TIMING_MINIMAL = ENABLE_WEB_TIMING_MINIMAL;
>  ENABLE_WEB_TIMING = ;

Nit:  I would put ENABLE_WEB_TIMING_MINIMAL after ENABLE_WEB_TIMING (to match FEATURE_DEFINES below).  If you change this, please remember to make the same change to all FeatureDefines.xcconfig files.

> Source/WTF/wtf/FeatureDefines.h:825
> +#if !defined(ENABLE_WEB_TIMING_MINIMAL)
> +#define ENABLE_WEB_TIMING_MINIMAL 0
> +#endif
> +
>  #if !defined(ENABLE_WEB_TIMING)
>  #define ENABLE_WEB_TIMING 0
>  #endif

Should we add an #error if both WEB_TIMING and WEB_TIMING_MINIMAL are enabled?

Nit:  I would reverse the order of these as well.

[zalan]

Created attachment 200090 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 200090 [details]
Patch

Clearing flags on attachment: 200090

Committed r149359: <http://trac.webkit.org/changeset/149359>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Re-opening since most of this change was rolled out for Bug 115601 in r149999:
<http://trac.webkit.org/changeset/149999>

[Ahmad Saleem]

Seems to work fine now.