[webkit-reviews] review granted: [Bug 170534] Throttle requestAnimationFrame in cross-origin iframes to 30fps : [Attachment 306353] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 170534: Throttle requestAnimationFrame in cross-origin iframes to 30fps
https://bugs.webkit.org/show_bug.cgi?id=170534

Attachment 306353: Patch

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




--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 306353
  --> https://bugs.webkit.org/attachment.cgi?id=306353
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.h
tml: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.h
tml: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.h
tml: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.h
tml. 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.h
tml: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.h
tml:78
> +

Is this empty line intentional?

>
LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.h
tml: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.ht
ml: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.ht
ml: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.h
tml. Could we combine the two tests into one? Or share more code?

>
LayoutTests/http/tests/frame-throttling/resources/requestAnimationFrame-frame.h
tml: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.