IntersectionObserver: use an idle callback to send notifications.

IntersectionObserver: use an idle callback to send notifications.

Re-landing after original:

https://codereview.chromium.org/1780163002/

... was reverted by:

https://codereview.chromium.org/1776493002/

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.

BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com

Committed: https://crrev.com/4cf47cb23a655b02d36e64ea3f986ff2900f5bdd
Cr-Commit-Position: refs/heads/master@{#385021}

[szager1, 2016-03-16 21:52:40]

Description was changed from

==========
IntersectionObserver: use an idle callback to send notifications.

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.
BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com
==========

to

==========
IntersectionObserver: use an idle callback to send notifications.

Re-landing after original:

https://codereview.chromium.org/1780163002/

... was reverted by:

https://codereview.chromium.org/1776493002/

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.

BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com
==========

[szager1, 2016-03-16 21:53:06]

szager@chromium.org changed reviewers:
+ esprehn@chromium.org

[sof, 2016-03-17 07:14:58]

Thanks for determining the underlying leak condition.

https://codereview.chromium.org/1806133002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html
(right):

https://codereview.chromium.org/1806133002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html:57:
setTimeout(finishJSTest, 150);
Do you need this with the leak exemptions and once the fix for 595155 lands? For
this & all other instances of it in this CL.

[haraken, 2016-03-17 10:10:53]

core/ LGTM

[szager1, 2016-03-17 17:43:22]

https://codereview.chromium.org/1806133002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html
(right):

https://codereview.chromium.org/1806133002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html:57:
setTimeout(finishJSTest, 150);
On 2016/03/17 07:14:58, sof wrote:
> Do you need this with the leak exemptions and once the fix for 595155 lands?
For
> this & all other instances of it in this CL.

The fix for 595155 will make it possible to remove the lines from
LayoutTests/LeakExpectations, but only if the tests continue to use setTimeout
here.  To avoid leaking, the test code must be certain that all idle tasks have
completed before exiting.

Issue 595152 shows that the idle tasks scheduled by IntersectionObserver will
only run when their 100ms timeout expires, hence the slightly conservative 150ms
timeout here.  And even were 595152 fixed, we'd probably *still* need a
setTimeout here, because there's no programmatic way to ascertain whether the
idle task actually ran.

[esprehn, 2016-03-17 17:45:58]

I don't think we can do this, you can have tests with 150ms waits in them. The
tests needs to be fast.

[esprehn, 2016-03-17 17:46:14]

On 2016/03/17 at 17:45:58, esprehn wrote:
> I don't think we can do this, you can have tests with 150ms waits in them. The
tests needs to be fast.

Can't*

[ojan, 2016-03-17 17:48:03]

https://codereview.chromium.org/1806133002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html
(right):

https://codereview.chromium.org/1806133002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html:57:
setTimeout(finishJSTest, 150);
On 2016/03/17 at 17:43:22, szager1 wrote:
> On 2016/03/17 07:14:58, sof wrote:
> > Do you need this with the leak exemptions and once the fix for 595155 lands?
For
> > this & all other instances of it in this CL.
> 
> The fix for 595155 will make it possible to remove the lines from
LayoutTests/LeakExpectations, but only if the tests continue to use setTimeout
here.  To avoid leaking, the test code must be certain that all idle tasks have
completed before exiting.
> 
> Issue 595152 shows that the idle tasks scheduled by IntersectionObserver will
only run when their 100ms timeout expires, hence the slightly conservative 150ms
timeout here.  And even were 595152 fixed, we'd probably *still* need a
setTimeout here, because there's no programmatic way to ascertain whether the
idle task actually ran.

Can you instead add a callback to the IntersectionObserver and finishJSTest
inside the callback?

[szager1, 2016-03-17 20:05:24]

I switched the tests to use:

requestIdleCallback(finishJSTest, {timeout:100})

I also added an explanation to the README about why that's necessary.  As far as
I can tell, there's no way to avoid the 100ms pause.

[szager1, 2016-03-21 18:14:51]

ping

[ojan, 2016-03-21 18:19:39]

lgtm

https://codereview.chromium.org/1806133002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/README (right):

https://codereview.chromium.org/1806133002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/README:43: crbug.com/595155
explains that when one of those tasks runs, it does not release
Somewhere in here should be a TODO that we can remove the timeout:100 once
crbug.com/595155 is fixed, right?

[esprehn, 2016-03-21 18:27:03]

We should put this test in the virtual test suite where idle callbacks
actually run. I don't think we should add a test that has a 100ms delay in
it.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-03-21 18:27:04]

We should put this test in the virtual test suite where idle callbacks
actually run. I don't think we should add a test that has a 100ms delay in
it.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[szager1, 2016-03-21 18:27:49]

https://codereview.chromium.org/1806133002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/README (right):

https://codereview.chromium.org/1806133002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/README:43: crbug.com/595155
explains that when one of those tasks runs, it does not release
On 2016/03/21 18:19:39, ojan wrote:
> Somewhere in here should be a TODO that we can remove the timeout:100 once
> crbug.com/595155 is fixed, right?

It's the other bug that needs to be fixed; added a TODO.

[szager1, 2016-03-21 18:28:11]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-03-21 18:28:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/1806133002/#ps60001
(title: "Added TODO about remove requestIdleCallback hack")

[commit-bot: I haz the power, 2016-03-21 18:28:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806133002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1806133002/60001

[szager1, 2016-03-21 18:29:15]

The CQ bit was unchecked by szager@chromium.org

[szager1, 2016-03-21 18:29:53]

On 2016/03/21 18:27:04, esprehn wrote:
> We should put this test in the virtual test suite where idle callbacks
> actually run. I don't think we should add a test that has a 100ms delay in
> it.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[szager1, 2016-03-21 18:30:39]

On 2016/03/21 18:27:04, esprehn wrote:
> We should put this test in the virtual test suite where idle callbacks
> actually run. I don't think we should add a test that has a 100ms delay in
> it.

I will let ojan and esprehn duke it out, let me know what you decide.

[ojan, 2016-03-22 01:13:51]

On 2016/03/21 at 18:30:39, szager wrote:
> On 2016/03/21 18:27:04, esprehn wrote:
> > We should put this test in the virtual test suite where idle callbacks
> > actually run. I don't think we should add a test that has a 100ms delay in
> > it.
> 
> I will let ojan and esprehn duke it out, let me know what you decide.

I was reluctant to block on this when it would require a ton of work, but now
that crbug.com/595152 has the idea of just adding an internals API, I think we
should probably go with that since it's not a lot of work. That is probably
better than committing slow tests since it's unlikely that we'll remember to
come back through and fix these.

Also, that would mean this patch could be a lot simpler. Instead of using
takeRecords, you could s/setTimeout/requestIdleCallback/.

That sound OK?

[szager1, 2016-03-25 09:12:28]

On 2016/03/22 01:13:51, ojan wrote:
> On 2016/03/21 at 18:30:39, szager wrote:
> > On 2016/03/21 18:27:04, esprehn wrote:
> > > We should put this test in the virtual test suite where idle callbacks
> > > actually run. I don't think we should add a test that has a 100ms delay in
> > > it.
> > 
> > I will let ojan and esprehn duke it out, let me know what you decide.
> 
> I was reluctant to block on this when it would require a ton of work, but now
> that crbug.com/595152 has the idea of just adding an internals API, I think we
> should probably go with that since it's not a lot of work. That is probably
> better than committing slow tests since it's unlikely that we'll remember to
> come back through and fix these.
> 
> Also, that would mean this patch could be a lot simpler. Instead of using
> takeRecords, you could s/setTimeout/requestIdleCallback/.
> 
> That sound OK?

[szager1, 2016-03-25 09:22:22]

On 2016/03/25 09:12:28, szager1 wrote:
> On 2016/03/22 01:13:51, ojan wrote:
> > On 2016/03/21 at 18:30:39, szager wrote:
> > > On 2016/03/21 18:27:04, esprehn wrote:
> > > > We should put this test in the virtual test suite where idle callbacks
> > > > actually run. I don't think we should add a test that has a 100ms delay
in
> > > > it.
> > > 
> > > I will let ojan and esprehn duke it out, let me know what you decide.
> > 
> > I was reluctant to block on this when it would require a ton of work, but
now
> > that crbug.com/595152 has the idea of just adding an internals API, I think
we
> > should probably go with that since it's not a lot of work. That is probably
> > better than committing slow tests since it's unlikely that we'll remember to
> > come back through and fix these.
> > 
> > Also, that would mean this patch could be a lot simpler. Instead of using
> > takeRecords, you could s/setTimeout/requestIdleCallback/.
> > 
> > That sound OK?

I have a patch out to add testRunner.runIdleTasks(), and with the patch these
the tests pass and don't leak, but the reviewers are OOO, and I'm going on
vacation:

https://codereview.chromium.org/1832673002/

I could move the tests into a virtual test suite that uses the threaded
compositor, but that has a couple of drawbacks:

- Even with the threaded compositor, there is no guarantee that idle tasks will
have run by the time the test finishes.  Even setTimeout(finishJSTest) won't
achieve that, since setTimeout runs with higher priority than idle tasks.

- Tests that require the test server (http/tests/*) can't be moved into virtual/
because the test runner doesn't recognize virtual/*/https/tests/* as tests that
need the test server.

So, for now, I'm going to just remove the requestIdleCallback(finishJSTest,
{timeout:100}) statements and add all the tests to LeakExpectations.  When
testRunner.runIdleTasks() is available, these tests can be converted to end
with:

if (window.testRunner)
  testRunner.runIdleTasks(finishJSTest);

... and can be removed from LeakExpectations.

[szager1, 2016-03-25 09:46:15]

PTAL

Note that there are still two slow tests:

- timestamp.html, because it needs to introduce a clock skew between the top
frame and the iframe.

- observer-without-js-reference.html, because it can't use takeRecords, so it
has to wait for the observer's idle task to run.

[esprehn, 2016-03-25 10:10:37]

In general I'm a bit worried that so many of the tests use takeRecords() and
don't use a callback. We should probably have a mix of both to get decent
coverage. :/

[szager1, 2016-03-25 14:32:36]

On 2016/03/25 10:10:37, esprehn wrote:
> In general I'm a bit worried that so many of the tests use takeRecords() and
> don't use a callback. We should probably have a mix of both to get decent
> coverage. :/

What if I put just a couple of tests into a virtual test suite that uses the
threaded compositor?  Those tests can *probably* avoid takeRecords, but might be
flaky, which is something we can deal with if it comes up.

[szager1, 2016-03-25 14:43:19]

On 2016/03/25 14:32:36, szager1 wrote:
> On 2016/03/25 10:10:37, esprehn wrote:
> > In general I'm a bit worried that so many of the tests use takeRecords() and
> > don't use a callback. We should probably have a mix of both to get decent
> > coverage. :/
> 
> What if I put just a couple of tests into a virtual test suite that uses the
> threaded compositor?  Those tests can *probably* avoid takeRecords, but might
be
> flaky, which is something we can deal with if it comes up.

I think it would be best to land the testRunner.runIdleTasks patch before this
one, and then edit the tests accordingly.  But that's not going to happen today,
and I'm on vacation next week.  I'll be back on April 4, which is four days
before the branch cut, and that's cutting it close.

This patch is a fairly significant change, and I think it's best if it lands and
gets some exposure next week.  If it's broken, I wouldn't want to wait until a
couple of days before the branch cut to find out.

[szager1, 2016-03-25 14:45:31]

On 2016/03/25 14:32:36, szager1 wrote:
> On 2016/03/25 10:10:37, esprehn wrote:
> > In general I'm a bit worried that so many of the tests use takeRecords() and
> > don't use a callback. We should probably have a mix of both to get decent
> > coverage. :/
> 
> What if I put just a couple of tests into a virtual test suite that uses the
> threaded compositor?  Those tests can *probably* avoid takeRecords, but might
be
> flaky, which is something we can deal with if it comes up.

There is one test that doesn't use takeRecords:
observer-without-js-reference.html.

Anyways, long term there's always going to be *something* unnatural in every
test; it's either going to be takeRecords or testRunner.runIdleTasks.  There's
no way around it.

[esprehn, 2016-03-25 20:12:43]

On 2016/03/25 at 14:45:31, szager wrote:
> On 2016/03/25 14:32:36, szager1 wrote:
> > On 2016/03/25 10:10:37, esprehn wrote:
> > > In general I'm a bit worried that so many of the tests use takeRecords()
and
> > > don't use a callback. We should probably have a mix of both to get decent
> > > coverage. :/
> > 
> > What if I put just a couple of tests into a virtual test suite that uses the
> > threaded compositor?  Those tests can *probably* avoid takeRecords, but
might be
> > flaky, which is something we can deal with if it comes up.
> 
> There is one test that doesn't use takeRecords:
observer-without-js-reference.html.
> 
> Anyways, long term there's always going to be *something* unnatural in every
test; it's either going to be takeRecords or testRunner.runIdleTasks.  There's
no way around it.

Yeah that's fine, takeRecords() is more concerning to me since it's an actual
web API. We could be getting the list of records to the callback wrong half the
time and we wouldn't notice. In general takeRecords() is probably an exceptional
case on the web too, look at the MutationObserver tests. It's mostly callbacks
and then a few takeRecords() tests.

[eae, 2016-03-30 17:39:16]

On 2016/03/25 20:12:43, esprehn wrote:
> On 2016/03/25 at 14:45:31, szager wrote:
> > On 2016/03/25 14:32:36, szager1 wrote:
> > > On 2016/03/25 10:10:37, esprehn wrote:
> > > > In general I'm a bit worried that so many of the tests use takeRecords()
> and
> > > > don't use a callback. We should probably have a mix of both to get
decent
> > > > coverage. :/
> > > 
> > > What if I put just a couple of tests into a virtual test suite that uses
the
> > > threaded compositor?  Those tests can *probably* avoid takeRecords, but
> might be
> > > flaky, which is something we can deal with if it comes up.
> > 
> > There is one test that doesn't use takeRecords:
> observer-without-js-reference.html.
> > 
> > Anyways, long term there's always going to be *something* unnatural in every
> test; it's either going to be takeRecords or testRunner.runIdleTasks.  There's
> no way around it.
> 
> Yeah that's fine, takeRecords() is more concerning to me since it's an actual
> web API. We could be getting the list of records to the callback wrong half
the
> time and we wouldn't notice. In general takeRecords() is probably an
exceptional
> case on the web too, look at the MutationObserver tests. It's mostly callbacks
> and then a few takeRecords() tests.

Looks like the timeout issue has been resolved which speeds up the tests. What
would it take in terms of test coverage to make you happy Elliot? Is there a way
to write a test that doesn't use takeRecords nor a timeout? Would one test like
that be sufficient or are you looking for a better distribution?
I'm trying to figure out if we can get this in before the branch point and if so
what it would take.
Thanks.

[eae, 2016-04-01 00:28:50]

Elliott?

[esprehn, 2016-04-01 00:31:51]

On 2016/04/01 at 00:28:50, eae wrote:
> Elliott?

I think we should move forward here. In general I think our tests should use the
API like developers would though. So we should probably add more callback tests.

[szager1, 2016-04-04 18:34:59]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-04-04 18:34:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/1806133002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-04 18:35:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806133002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1806133002/100001

[commit-bot: I haz the power, 2016-04-04 21:53:51]

Description was changed from

==========
IntersectionObserver: use an idle callback to send notifications.

Re-landing after original:

https://codereview.chromium.org/1780163002/

... was reverted by:

https://codereview.chromium.org/1776493002/

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.

BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com
==========

to

==========
IntersectionObserver: use an idle callback to send notifications.

Re-landing after original:

https://codereview.chromium.org/1780163002/

... was reverted by:

https://codereview.chromium.org/1776493002/

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.

BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com
==========

[commit-bot: I haz the power, 2016-04-04 21:53:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-04 21:55:40]

Description was changed from

==========
IntersectionObserver: use an idle callback to send notifications.

Re-landing after original:

https://codereview.chromium.org/1780163002/

... was reverted by:

https://codereview.chromium.org/1776493002/

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.

BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com
==========

to

==========
IntersectionObserver: use an idle callback to send notifications.

Re-landing after original:

https://codereview.chromium.org/1780163002/

... was reverted by:

https://codereview.chromium.org/1776493002/

With this change, the tests can no longer use setTimeout(0) to wait
for notifications to be delivered.  Instead, use takeRecords() to
proactively grab notifications right after they are generated
(typically in a RAF right after a layout change).

These tests tickle a bug in ScriptedIdleTaskController, hence the
LeakExpectations entries.

BUG=540528
R=ojan@chromium.org,haraken@chromium.org,sigbjornf@opera.com

Committed: https://crrev.com/4cf47cb23a655b02d36e64ea3f986ff2900f5bdd
Cr-Commit-Position: refs/heads/master@{#385021}
==========

[commit-bot: I haz the power, 2016-04-04 21:55:42]

Patchset 6 (id:??) landed as
https://crrev.com/4cf47cb23a655b02d36e64ea3f986ff2900f5bdd
Cr-Commit-Position: refs/heads/master@{#385021}