Bug 170534 – Throttle requestAnimationFrame in cross-origin iframes to 30fps

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

[Simon Fraser (smfr)]

Throttle requestAnimationFrame in cross-origin iframes to 30fps

[Simon Fraser (smfr)]

Created attachment 306353 [details]
Patch

[Daniel Bates]

Comment on attachment 306353 [details]
Patch

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

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:23
> +        eventSender.mouseMoveTo(testFrame.offsetLeft + 5, testFrame.offsetTop + 5);
> +        eventSender.mouseDown();
> +        eventSender.mouseUp();

This will not work when running this test using WebKit2 in iOS Simulator. One way to fix this is to write this logic using the convenience functions in LayoutTests/resources/ui-helper.js.

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:58
> +        debug('Received message: ' + message.data);

The notation used for the string literal on this line is inconsistent with the notation used for string literal above. That is, this line used single quotes where as double quotes are used for all string literals above this line. I suggest that we use double quotes for the string literal in this line. Regardless, we should pick one quoting style and stick with it throughout this test unless we can avoid the need to escape a character by picking a particular quoting style.

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:64
> +        var re = /throttled\: (true|false)/;

It is unnecessary to escape the ':'. This regular is reasonable given the way the message is formatted in LayoutTests/http/tests/frame-throttling/resources/requestAnimationFrame-frame.html. We could strengthen it by matching the start and end of the line by using the ^ and $ metacharacters.

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:72
> +        debug('Failed to handle message '+ message.data);

The notation used for the string literal on this line is inconsistent with the notation used for string literal above. That is, this line used single quotes where as double quotes are used for all string literals above this line. I suggest that we use double quotes for the string literal in this line. Regardless, we should pick one quoting style and stick with it throughout this test unless we can avoid the need to escape a character by picking a particular quoting style.

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:78
> +

Is this empty line intentional?

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:79
> +<script src="../../resources/js-test-post.js"></script>

The indentation of this line is inconsistent with the indentation of line 77. For your consideration I suggest indenting this line such that it is left aligned with the '<' on line 77.

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-same-origin-subframe.html:12
> +    description("Tests that requestAnimationFrame is throttled in subframes that are cross-origin");

cross-origin => same-origin

> LayoutTests/http/tests/frame-throttling/raf-throttle-in-same-origin-subframe.html:33
> +    function clickSubframe()
> +    {
> +        eventSender.mouseMoveTo(testFrame.offsetLeft + 5, testFrame.offsetTop + 5);
> +        eventSender.mouseDown();
> +        eventSender.mouseUp();
> +    }
> +
> +    function interactWithSubframe()
> +    {
> +        debug("Simulating user interaction.");
> +        clickSubframe();
> +
> +        messageHandler = checkUnthrottledAfterInteraction;
> +        testFrame.contentWindow.postMessage("report-throttle", "*")
> +    }

This file duplicates much of the functionality of LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html. Could we combine the two tests into one? Or share more code?

> LayoutTests/http/tests/frame-throttling/resources/requestAnimationFrame-frame.html:21
> +        window.addEventListener('load', function() {

The notation used for the string literal on this line is inconsistent with the notation used for string literal above. That is, this line used single quotes where as double quotes are used for all string literals above this line. I suggest that we use double quotes for the string literal in this line. Regardless, we should pick one quoting style and stick with it throughout this test unless we can avoid the need to escape a character by picking a particular quoting style.

[Daniel Bates]

Comment on attachment 306353 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6325
> +    if (static_cast<bool>(time) && m_scriptedAnimationController) {

Is the static_cast<bool> necessary here?

[Build Bot]

Comment on attachment 306353 [details]
Patch

Attachment 306353 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3482374

New failing tests:
http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html

[Build Bot]

Created attachment 306362 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Simon Fraser (smfr)]

Created attachment 306424 [details]
Patch

[Chris Dumez]

Comment on attachment 306424 [details]
Patch

nice, LGTM2.

[Daniel Bates]

Comment on attachment 306424 [details]
Patch

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

> LayoutTests/http/tests/resources/ui-helper.js:2
> +window.UIHelper = class UIHelper {

Is it necessary to make a copy of this file? I thought we map /js-test-resources/ to LayoutTests/resources such that we can reference this file in LayoutTests/resources/ui-helper.js using URL /js-test-resources/ui-helper.js

[Simon Fraser (smfr)]

(In reply to Daniel Bates from comment #8)
> Comment on attachment 306424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306424&action=review
> 
> > LayoutTests/http/tests/resources/ui-helper.js:2
> > +window.UIHelper = class UIHelper {
> 
> Is it necessary to make a copy of this file? I thought we map
> /js-test-resources/ to LayoutTests/resources such that we can reference this
> file in LayoutTests/resources/ui-helper.js using URL
> /js-test-resources/ui-helper.js

You're right. So why do we have copies of js-test-pre.js and js-test-post.js under http tests?

[Simon Fraser (smfr)]

https://trac.webkit.org/r215070

rdar://problem/28812757

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #9)
> (In reply to Daniel Bates from comment #8)
> > Comment on attachment 306424 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306424&action=review
> > 
> > > LayoutTests/http/tests/resources/ui-helper.js:2
> > > +window.UIHelper = class UIHelper {
> > 
> > Is it necessary to make a copy of this file? I thought we map
> > /js-test-resources/ to LayoutTests/resources such that we can reference this
> > file in LayoutTests/resources/ui-helper.js using URL
> > /js-test-resources/ui-helper.js
> 
> You're right. So why do we have copies of js-test-pre.js and js-test-post.js
> under http tests?

I suspect these files exist for historical reasons. We need to update existing tests to reference the files in /js-test-resources/ and then we can remove them.

[Ryan Haddad]

The LayoutTest for this change is failing on ios-simulator:

https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/r215094%20(497)/results.html

[mdrejhon]

An additional consideration I'd like to add is embedded scientific animations (e.g. frame rate comparisons) such as http://www.testufo.com which supports being embedded.

- Embeds in forum threads (example http://forums.blurbusters.com/viewtopic.php?f=19&t=143&p=4245&hilit=TestUFO+embed ) 
- Embeds in web pages (example http://www.blurbusters.com/gsync/preview ) 

These are often 120fps-versus-60fps-versus-30fps web animations, which are extremely educational in explaining to users of the benefits of of increased refresh rates & variable refresh rates.

Both FireFox and Chrome supports full-framerate requestAnimationFrame() embeds.  TestUFO permits other third party websites to embed TestUFO animations for demonstration purposes, much like a 60fps video.