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

Side by Side 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 unified diff | 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')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <script src="/js-test-resources/js-test.js"></script>	2 <script src="/js-test-resources/js-test.js"></script>

3 <div style="width:100%; height:700px;"></div>	3 <div style="width:100%; height:700px;"></div>

4 <iframe id="target-iframe" src="http://localhost:8080/intersection-observer/reso urces/cross-origin-subframe.html" style="height: 100px; overflow-y: scroll"></if rame>	4 <iframe id="target-iframe" src="http://localhost:8080/intersection-observer/reso urces/cross-origin-subframe.html" style="height: 100px; overflow-y: scroll"></if rame>

5 <div style="width:100%; height:700px;"></div>	5 <div style="width:100%; height:700px;"></div>

6	6

7 <script>	7 <script>

8 setPrintTestResultsLazily();	8 setPrintTestResultsLazily();

9 self.jsTestIsAsync = true;	9 self.jsTestIsAsync = true;

10	10

(...skipping 43 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
54 checkData(event.data.actual, event.data.expected);	54 checkData(event.data.actual, event.data.expected);

55 } else if (event.data.hasOwnProperty('DONE')) {	55 } else if (event.data.hasOwnProperty('DONE')) {

56 finishJSTest();	56 finishJSTest();

57 document.scrollingElement.scrollTop = 0;	57 document.scrollingElement.scrollTop = 0;

58 } else {	58 } else {

59 requestAnimationFrame(function () { iframe.contentWindow.postMessage("", "*" ) });	59 requestAnimationFrame(function () { iframe.contentWindow.postMessage("", "*" ) });

60 }	60 }

61 }	61 }

62	62

63 window.addEventListener("message", handleMessage);	63 window.addEventListener("message", handleMessage);

	64

64 iframe.onload = function() {	65 iframe.onload = function() {

65 iframe.contentWindow.postMessage("", "*")	66 requestAnimationFrame(() => {

66 };	67 requestAnimationFrame(() => { iframe.contentWindow.postMessage("", "*") })
	ojan 2016/03/08 01:37:11 Why do you need 2 rafs here? Why do you need 2 rafs here? Sami 2016/03/08 17:32:41 Right, this pattern seems to be repeated in a bunc Show quoted text 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. szager1 2016/03/08 18:51:32 In general, the double-RAF pattern is necessary be Show quoted text 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. Sami 2016/03/09 18:29:55 I thought IO runs after layout, so the notificatio Show quoted text 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? szager1 2016/03/09 20:45:25 Actually, my explanation was totally wrong. I add Show quoted text 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.
	68 });

	69 }

67 </script>	70 </script>

OLD	NEW