third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html - Issue 1776493002: IntersectionObserver: use an idle callback to send notifications.

Unified Diff: third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html

Issue 1776493002: IntersectionObserver: use an idle callback to send notifications. (Closed) Base URL: https://chromium.googlesource.com/chromium/src@master

Patch Set: Created 4 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « no previous file | third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/cross-origin-subframe.html » ('j') | third_party/WebKit/LayoutTests/intersection-observer/observer-without-js-reference.html » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html

diff --git a/third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html b/third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html

index 3324515a453456a0c2648cce43c58f7226ac0b4f..bfb48f37c4fae814c7bb81b78f7a8cda77d5b718 100644

--- a/third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html

+++ b/third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html

@@ -61,7 +61,10 @@ function handleMessage(event) {

}

window.addEventListener("message", handleMessage);

iframe.onload = function() {

- iframe.contentWindow.postMessage("", "*")

-};

+ requestAnimationFrame(() => {

+ requestAnimationFrame(() => { iframe.contentWindow.postMessage("", "*") })

ojan 2016/03/08 01:37:11 Why do you need 2 rafs here?

Sami 2016/03/08 17:32:41 Right, this pattern seems to be repeated in a bunc

szager1 2016/03/08 18:51:32 In general, the double-RAF pattern is necessary be

Sami 2016/03/09 18:29:55 I thought IO runs after layout, so the notificatio

I thought IO runs after layout, so the notifications are based on what is currently being rendered, not the previous layout? Or is the first frame a special case since we don't have anything to compare against? In any case, could you add that as a comment here to make it more obvious that this is deliberate?

szager1 2016/03/09 20:45:25 Actually, my explanation was totally wrong. I add

On 2016/03/09 18:29:55, Sami wrote: > On 2016/03/08 18:51:32, szager1 wrote: > > On 2016/03/08 17:32:41, Sami wrote: > > > On 2016/03/08 01:37:11, ojan wrote: > > > > Why do you need 2 rafs here? > > > > > > Right, this pattern seems to be repeated in a bunch of the other tests too. > > > Maybe reduce it to one if you can or add a common helper function that > > > encapsulates the logic? > > > > > > Since the idle callbacks are scheduled in the lifecycle update, they should > > get > > > serviced within the same frame assuming there is idle time. This might get > > flaky > > > if there isn't idle time, so maybe we could use some explicit callback > > instead? > > > FWIW requestIdleCallbacks run in the requested order, but I guess the spec > > > doesn't really say whether those have any relation to intersection observer > > > notifications. > > > > In general, the double-RAF pattern is necessary because the order of events is > > something like this: > > > > - onload happens, possibly after a frame has been generated. Layout is > changed > > in onload, but the changes won't get rendered until the next frame. > > > > - On the next frame, IntersectionObserver runs its algorithm based on the > > previous layout, so no notifications are generated. The layout change from > > onload is rendered. > > > > - One the next frame, IntersectionObserver runs its algorithm based on the new > > layout, and generated the expected notifications. > > > > It's racy based on when onload runs, but the double-RAF should avoid any > > problems. All subsequent test steps used a single RAF. > > > > It's possible that for this particular test, because it uses postMessage, the > > double-RAF is unnecessary. I could take it out and see if the test gets > flaky. > > I could also just leave it in. > > I thought IO runs after layout, so the notifications are based on what is > currently being rendered, not the previous layout? Or is the first frame a > special case since we don't have anything to compare against? > > In any case, could you add that as a comment here to make it more obvious that > this is deliberate?

Actually, my explanation was totally wrong. I added LayoutTests/intersection-observer/README with a full explanation, and added comments referring to the README in each test.

+ });

</script>