Bug 170534 - Throttle requestAnimationFrame in cross-origin iframes to 30fps
Summary: Throttle requestAnimationFrame in cross-origin iframes to 30fps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-05 18:23 PDT by Simon Fraser (smfr)
Modified: 2020-07-31 06:31 PDT (History)
10 users (show)

See Also:


Attachments
Patch (16.97 KB, patch)
2017-04-05 18:27 PDT, Simon Fraser (smfr)
dbates: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (13.44 MB, application/zip)
2017-04-05 20:55 PDT, Build Bot
no flags Details
Patch (16.86 KB, patch)
2017-04-06 13:53 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-04-05 18:23:00 PDT
Throttle requestAnimationFrame in cross-origin iframes to 30fps
Comment 1 Simon Fraser (smfr) 2017-04-05 18:27:22 PDT
Created attachment 306353 [details]
Patch
Comment 2 Daniel Bates 2017-04-05 20:33:38 PDT
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.
Comment 3 Daniel Bates 2017-04-05 20:55:14 PDT
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?
Comment 4 Build Bot 2017-04-05 20:55:42 PDT
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
Comment 5 Build Bot 2017-04-05 20:55:44 PDT
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
Comment 6 Simon Fraser (smfr) 2017-04-06 13:53:24 PDT
Created attachment 306424 [details]
Patch
Comment 7 Chris Dumez 2017-04-06 15:49:05 PDT
Comment on attachment 306424 [details]
Patch

nice, LGTM2.
Comment 8 Daniel Bates 2017-04-06 16:54:58 PDT
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
Comment 9 Simon Fraser (smfr) 2017-04-06 16:57:48 PDT
(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?
Comment 10 Simon Fraser (smfr) 2017-04-06 17:05:16 PDT
https://trac.webkit.org/r215070

rdar://problem/28812757
Comment 11 Daniel Bates 2017-04-06 20:43:26 PDT
(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.
Comment 12 Ryan Haddad 2017-04-07 11:13:57 PDT
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
Comment 13 mdrejhon 2017-06-15 13:42:20 PDT
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.