Use intersection observer to control frame throttling

Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519, 647393, 661182
Committed: https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e
Cr-Commit-Position: refs/heads/master@{#429046}

[Sami, 2016-08-24 17:53:12]

Description was changed from

==========
Use intersection observer to control frame throttling

(WIP)
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

Because the built-in 100ms slack time for intersection observer delivery
results in too much checkerboarding, this patch also introduces a low
latency intersection observer. Low latency notifications are delivered
immediately with a post task instead of waiting for the next idle period.

BUG=616519
==========

[Sami, 2016-08-24 17:53:12]

skyostil@chromium.org changed reviewers:
+ ojan@chromium.org, szager@chromium.org

[Sami, 2016-08-24 17:56:31]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[Sami, 2016-08-24 17:56:32]

PTAL.

[commit-bot: I haz the power, 2016-08-24 17:57:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 18:35:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:35:01]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sami, 2016-08-25 09:42:48]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 09:43:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 10:21:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 10:22:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sami, 2016-08-25 15:01:24]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 15:02:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 15:40:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 15:40:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sami, 2016-08-25 16:32:14]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 16:32:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 18:03:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 18:03:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2016-08-26 16:47:27]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 16:48:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 17:42:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 17:42:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sami, 2016-08-30 11:08:35]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 11:08:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 12:12:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 12:12:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2016-08-30 18:45:22]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 18:46:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 20:13:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 20:13:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[szager1, 2016-08-30 23:57:12]

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp
(right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp:27: ,
m_lowLatencyCallbackFiredWhileSuspended(false)
I don't think you need m_lowLatencyCallbacksFiredWhileSuspended.  Instead,
resume() should call deliverLowLatencyNotifications() unconditionally, or post a
task to do so.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp:35: if
(observer.lowLatency()) {
I think this would read better if you refactor it to call into one of two new
methods:

void
IntersectionObserverController::scheduleIntersectionObserverForDelivery(IntersectionObserver&
observer)
{
  if (observer.lowLatency())
    scheduleLowLatencyNotification(observer);
  else
    scheduleIdleNotification(observer);
}

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:286: 
if (frame().isLocalRoot())
  return;

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:288: // Intersection observer
does not currently support remote frames. As a
Is there a nice way to combine this with the static method getRootNode() in
IntersectionObserver.cpp?

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4299: :
m_frameView(std::move(frameView))
Why std::move?

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4307: for (const auto& entry
: entries)
if (entries.length())
  isHidden = !entries.last()->intersectionRatio();

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4310:
m_frameView->updateRenderThrottlingStatus(isHidden,
m_frameView->m_subtreeThrottled);
I would expect that you need a friend declaration somewhere to access
m_frameView->m_subtreeThrottled, no?

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4315: return
m_frameView->frame().document();
I'm not sure it matters, which ExecutionContext is passed to postTask, but I
think this should be the same document as the root node of the observer, i.e.:

  return &m_frameView->m_intersectionObserver->rootNode()->document();

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:684: Member<FrameView>
m_frameView;
I think this should be WeakMember, to avoid a reference cycle:

FrameView->IntersectionObserver->IntersectionObserverCallback->FrameView.

[ojan, 2016-08-31 01:11:08]

I didn't look closely at the patch, but I think if we are adding a way to
postTask with IntersectionObserver, we need to either make that how IO works, or
expose an API to authors to do the same. If we need it, authors will need it
too.

[Sami, 2016-08-31 11:07:15]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 11:07:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-08-31 11:08:56]

> I didn't look closely at the patch, but I think if we are adding a way to
> postTask with IntersectionObserver, we need to either make that how IO works,
or
> expose an API to authors to do the same. If we need it, authors will need it
> too.

You mean exposing some kind of low latency flag through the API? I think that
would be fine, as long as it's not the default. We probably want to do that in a
different patch though.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp
(right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp:27: ,
m_lowLatencyCallbackFiredWhileSuspended(false)
On 2016/08/30 23:57:11, szager1 wrote:
> I don't think you need m_lowLatencyCallbacksFiredWhileSuspended.  Instead,
> resume() should call deliverLowLatencyNotifications() unconditionally, or post
a
> task to do so.

Done.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp:35: if
(observer.lowLatency()) {
On 2016/08/30 23:57:11, szager1 wrote:
> I think this would read better if you refactor it to call into one of two new
> methods:
> 
> void
>
IntersectionObserverController::scheduleIntersectionObserverForDelivery(IntersectionObserver&
> observer)
> {
>   if (observer.lowLatency())
>     scheduleLowLatencyNotification(observer);
>   else
>     scheduleIdleNotification(observer);
> }

Good idea, done.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:286: 
On 2016/08/30 23:57:12, szager1 wrote:
> if (frame().isLocalRoot())
>   return;

Any particular reason why? That would mean we don't get events for the root
frame. There's a test checking that a 0x0 root frame becomes throttled, and it
seems we want to keep that property.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:288: // Intersection observer
does not currently support remote frames. As a
On 2016/08/30 23:57:12, szager1 wrote:
> Is there a nice way to combine this with the static method getRootNode() in
> IntersectionObserver.cpp?

I gave this a try but couldn't find a good way to do it. The problem is that
IntersectionObserverInit wants the root as an Element, but we actually need to
use the Document which is just a Node. I tried passing in the document element
instead, but that seemed to break everything (did not get observations anymore).

The other alternative I think is to make the observer turn a document element
into a document node, but that would be a web-visible change and probably
doesn't make much sense either.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4299: :
m_frameView(std::move(frameView))
On 2016/08/30 23:57:12, szager1 wrote:
> Why std::move?

Oops, looks like I'm typing it out of habit already :)

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4307: for (const auto& entry
: entries)
On 2016/08/30 23:57:11, szager1 wrote:
> if (entries.length())
>   isHidden = !entries.last()->intersectionRatio();

Neat, thanks!

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4310:
m_frameView->updateRenderThrottlingStatus(isHidden,
m_frameView->m_subtreeThrottled);
On 2016/08/30 23:57:12, szager1 wrote:
> I would expect that you need a friend declaration somewhere to access
> m_frameView->m_subtreeThrottled, no?

This is an inner class and since C++11 it's automatically a friend.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4315: return
m_frameView->frame().document();
On 2016/08/30 23:57:12, szager1 wrote:
> I'm not sure it matters, which ExecutionContext is passed to postTask, but I
> think this should be the same document as the root node of the observer, i.e.:
> 
>   return &m_frameView->m_intersectionObserver->rootNode()->document();

I wasn't sure about that either. I've now made it consistent with the root,
although it needed a little more plumbing than your suggestion since the
intersection observer construction already requires a valid context from this
getter.

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:684: Member<FrameView>
m_frameView;
On 2016/08/30 23:57:12, szager1 wrote:
> I think this should be WeakMember, to avoid a reference cycle:
> 
> FrameView->IntersectionObserver->IntersectionObserverCallback->FrameView.

I guess I was thinking that with Oilpan in-heap reference cycles aren't an issue
anymore, but maybe a WeakMember is still more appropriate? Done.

[commit-bot: I haz the power, 2016-08-31 12:43:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 12:43:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[szager1, 2016-08-31 20:54:27]

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:286: 
On 2016/08/31 11:08:56, Sami wrote:
> On 2016/08/30 23:57:12, szager1 wrote:
> > if (frame().isLocalRoot())
> >   return;
> 
> Any particular reason why? That would mean we don't get events for the root
> frame. There's a test checking that a 0x0 root frame becomes throttled, and it
> seems we want to keep that property.

OK; that's a weird scenario, but if that's the behavior you want, then forget my
comment.

https://codereview.chromium.org/2272773002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:295: }
Shouldn't this be:

} else {
  context = mainFrame->document();
}

?

[ojan, 2016-09-01 05:04:30]

On 2016/08/31 at 11:08:56, skyostil wrote:
> > I didn't look closely at the patch, but I think if we are adding a way to
> > postTask with IntersectionObserver, we need to either make that how IO
works, or
> > expose an API to authors to do the same. If we need it, authors will need it
> > too.
> 
> You mean exposing some kind of low latency flag through the API? I think that
would be fine, as long as it's not the default. We probably want to do that in a
different patch though.

There's a few options on the table. One would be to stop using an idle callback
entirely. Another would be to give an option for low latency. Yet another would
be to allow you to control the deadline.

I agree you don't want to change that in this patch, but I would like for us to
prioritize fixing this. I've talked to szager and he's going to do some
investigation.

This should have at TODO to remove low latency mode and replace it with whatever
we decide to do for web authors.

[Sami, 2016-09-01 11:11:35]

> There's a few options on the table. One would be to stop using an idle
callback
> entirely. Another would be to give an option for low latency. Yet another
would
> be to allow you to control the deadline.

Agreed. FWIW I tried to expose the deadline control for this patch, but the
problem was that a deadline of 0 currently means no deadline.

> I agree you don't want to change that in this patch, but I would like for us
to
> prioritize fixing this. I've talked to szager and he's going to do some
> investigation.
> 
> This should have at TODO to remove low latency mode and replace it with
whatever
> we decide to do for web authors.

Great. I've added a TODO.

https://codereview.chromium.org/2272773002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:295: }
On 2016/08/31 20:54:27, szager1 wrote:
> Shouldn't this be:
> 
> } else {
>   context = mainFrame->document();
> }
> 
> ?

Ah, well spotted, thanks.

[Sami, 2016-09-01 11:11:48]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 11:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 12:37:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 12:37:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ojan, 2016-09-06 21:16:35]

lgtm

https://codereview.chromium.org/2272773002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2272773002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:65: // authors and
adjust/remove this API accordingly.
FWIW, it looks like we're just going to change it to always postTask, but you
don't need to be on the hook for that, so a TODO is still good. Me or Stefan
will fix.

Or, if you're willing to wait, you can wait till we fix it. Either way is fine w
me.

[szager1, 2016-09-06 22:09:05]

lgtm

[ojan, 2016-09-10 02:18:27]

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp
(right):

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp:59:
TaskRunnerHelper::get(TaskType::Unthrottled,
getExecutionContext())->postTask(BLINK_FROM_HERE,
m_lowLatencyNotificationTask->cancelAndCreate());
In retrospect, I wonder if unthrottled task runner is really necessary. Maybe it
was a mistake to use postTask at all for throttling frames. We could just use
takeRecords on the IntersectionObserver at the end of updateAllLifeCyclePhases.
It's doing something that web content can't do with current API, which is a bit
of a bummer, but so is the unthrottled task running. I think it's reasonably to
say that the code that actually does the platform-level throttling of frames can
be a bit special. But also, we have some API proposals that would actually let
web content hook in at this point in the pipeline as well. Lastly, we still
share all the code for computing intersection rects and everything.

Let me know what you think about this. If we were to change that, then we could
change https://codereview.chromium.org/2322143002 to just use a Timer instead of
needing it's own weakPtrFactory and whatnot and I think we could get rid of the
concept of unthrottled tasks?

[haraken, 2016-09-12 00:46:32]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-09-12 00:46:33]

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp
(right):

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp:59:
TaskRunnerHelper::get(TaskType::Unthrottled,
getExecutionContext())->postTask(BLINK_FROM_HERE,
m_lowLatencyNotificationTask->cancelAndCreate());
On 2016/09/10 02:18:27, ojan wrote:
> In retrospect, I wonder if unthrottled task runner is really necessary. Maybe
it
> was a mistake to use postTask at all for throttling frames. We could just use
> takeRecords on the IntersectionObserver at the end of
updateAllLifeCyclePhases.
> It's doing something that web content can't do with current API, which is a
bit
> of a bummer, but so is the unthrottled task running. I think it's reasonably
to
> say that the code that actually does the platform-level throttling of frames
can
> be a bit special. But also, we have some API proposals that would actually let
> web content hook in at this point in the pipeline as well. Lastly, we still
> share all the code for computing intersection rects and everything.
> 
> Let me know what you think about this. If we were to change that, then we
could
> change https://codereview.chromium.org/2322143002 to just use a Timer instead
of
> needing it's own weakPtrFactory and whatnot and I think we could get rid of
the
> concept of unthrottled tasks?

Just help me understand: IIUC, frame throttling is enabled on cross-origin
frames. It means that IntersectionObservers may be throttled only on
cross-origin frames. I was thinking that it's totally okay, but is your
intention for this CL that you don't want to throttle IntersectionObservers even
on cross-origin frames?

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserverController.h
(right):

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserverController.h:53:
std::unique_ptr<CancellableTaskFactory> m_lowLatencyNotificationTask;

IIUC, we've decided to use WeakPtrFactory to post a cancellable task, right?

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:293:
observerInit.setRoot(frame().localFrameRoot()->document()->documentElement());

This looks a bit too weird to me. Maybe can we just ignore remote frames until
we support them?

[dgrogan, 2016-09-15 23:47:06]

dgrogan@chromium.org changed reviewers:
+ dgrogan@chromium.org

[dgrogan, 2016-09-15 23:47:08]

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4299:
frame().document()->onVisibilityMaybeChanged(!m_hiddenForThrottling);
If this was unintentionally dropped in the new code can it get reinserted?

[dgrogan, 2016-09-21 17:55:56]

What's the status/plan for this CL? I'm looking to piggy-back some more
data-gathering in your new IntersectionObserverCallback::handleEvent
implementation.

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2272773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4889:
frame->view()->setupRenderThrottling();
Just for my understanding, why call this here in addition to
FrameView::setParent? Won't setParent cover all non-top-level frames?

[Sami, 2016-09-22 10:49:29]

> What's the status/plan for this CL? I'm looking to piggy-back some more
> data-gathering in your new IntersectionObserverCallback::handleEvent
> implementation.

It's on hold while I investigate some frame timer throttling related breakage.
I'll return to it once I have some time again.

[Sami, 2016-10-07 18:35:36]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 18:36:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 19:37:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 19:37:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sami, 2016-10-11 00:55:07]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 00:55:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 02:04:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 02:04:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2016-10-11 05:55:37]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 05:55:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 07:05:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 07:05:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2016-10-11 08:52:32]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

Because the built-in 100ms slack time for intersection observer delivery
results in too much checkerboarding, this patch also introduces a low
latency intersection observer. Low latency notifications are delivered
immediately with a post task instead of waiting for the next idle period.

BUG=616519
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519
==========

[Sami, 2016-10-11 09:25:16]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 09:25:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 10:35:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 10:35:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2016-10-12 07:11:57]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 07:12:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 08:38:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 08:38:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2016-10-13 05:08:25]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 05:08:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 06:43:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 06:43:25]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-10-13 07:02:06]

Alright, I've rebased the patch and the trybots seem content with it, so I'd
like to try landing this now. I haven't dug into the Gmail bug with timer
throttling yet, but since this makes many aspects of throttling more rational I
think it makes sense to get this in first.

Ojan/Stefan, would you mind checking the small tweak I made to the SVG code?

[szager1, 2016-10-13 19:12:30]

On 2016/10/13 07:02:06, Sami (slow -- traveling) wrote:
> Alright, I've rebased the patch and the trybots seem content with it, so I'd
> like to try landing this now. I haven't dug into the Gmail bug with timer
> throttling yet, but since this makes many aspects of throttling more rational
I
> think it makes sense to get this in first.
> 
> Ojan/Stefan, would you mind checking the small tweak I made to the SVG code?

It looks like the SVG changes are just fixing errors in the rebase, right? 
You're not actually changing SVG code with this patch?

[Sami, 2016-10-14 00:51:13]

On 2016/10/13 19:12:30, szager1 wrote:
> On 2016/10/13 07:02:06, Sami (slow -- traveling) wrote:
> > Alright, I've rebased the patch and the trybots seem content with it, so I'd
> > like to try landing this now. I haven't dug into the Gmail bug with timer
> > throttling yet, but since this makes many aspects of throttling more
rational
> I
> > think it makes sense to get this in first.
> > 
> > Ojan/Stefan, would you mind checking the small tweak I made to the SVG code?
> 
> It looks like the SVG changes are just fixing errors in the rebase, right? 
> You're not actually changing SVG code with this patch?

I'm actually making LayoutSVGBlock::mapAncestorToLocal and
LayoutSVGModelObject::mapAncestorToLocal respect the |flags| argument. I'm not
sure why they didn't do that before.

[ojan, 2016-10-14 02:25:48]

I think you should use ElementVisibilityObserver if you can. Is there a reason
you can't? If it's not exposing something you need from IntersectionObserver, I
think it'd be find to expand the api surface.

https://codereview.chromium.org/2272773002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:313:
observation->setLastThresholdIndex(std::numeric_limits<unsigned>::max());
This is digging into the guts of IntersectionObserver algorithm too much.
Instead, we should probably add a first-class feature that lets you declare that
you want to assume targets are visible by default.

I don't know if that should be a value on the IntersectionObserver constructor
or on the observe call. I think for consistency with other configuration bits,
it should probably be on the constructor. I filed a bug at
https://github.com/WICG/IntersectionObserver/issues/163 to track changing the
web exposed API, but in the meantime, we can implement what we need and then
rejigger it to match the web exposed API once there's agreement on what that
should look like.

That sound OK?

[Sami, 2016-10-25 11:49:44]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 11:49:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-25 12:12:21]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 12:12:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 13:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 13:19:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2016-10-25 17:11:51]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 17:12:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-25 17:45:53]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 17:46:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-25 17:47:00]

On 2016/10/14 02:25:48, ojan wrote:
> I think you should use ElementVisibilityObserver if you can. Is there a reason
> you can't? If it's not exposing something you need from IntersectionObserver,
I
> think it'd be find to expand the api surface.

The only reason I didn't use it because this patch predates that class. Anyway,
I'm using it now -- PTAL :)

[Sami, 2016-10-25 17:47:12]

https://codereview.chromium.org/2272773002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:313:
observation->setLastThresholdIndex(std::numeric_limits<unsigned>::max());
On 2016/10/14 02:25:47, ojan wrote:
> This is digging into the guts of IntersectionObserver algorithm too much.
> Instead, we should probably add a first-class feature that lets you declare
that
> you want to assume targets are visible by default.
> 
> I don't know if that should be a value on the IntersectionObserver constructor
> or on the observe call. I think for consistency with other configuration bits,
> it should probably be on the constructor. I filed a bug at
> https://github.com/WICG/IntersectionObserver/issues/163 to track changing the
> web exposed API, but in the meantime, we can implement what we need and then
> rejigger it to match the web exposed API once there's agreement on what that
> should look like.
> 
> That sound OK?

Yeah, sounds good. I've added an explicit method for this now since I couldn't
figure out how to add something to IntersectionObserverInit without also
exposing it to the web.

[commit-bot: I haz the power, 2016-10-25 21:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 21:00:45]

Dry run: This issue passed the CQ dry run.

[dgrogan, 2016-10-25 22:29:37]

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4406: if
(frame().document()->frame()) {
Removing this doesn't cause any of the DeferredLoading tests to fail. What's it
checking for? I'll add a test for it.

[szager1, 2016-10-25 22:39:35]

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h (right):

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:39: void
deliverObservationsForTesting();
Can we avoid this by adding calls to runPendingTasks() in the test code?

[Sami, 2016-10-26 17:58:48]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 17:59:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-26 17:59:35]

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h (right):

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:39: void
deliverObservationsForTesting();
On 2016/10/25 22:39:35, szager1 wrote:
> Can we avoid this by adding calls to runPendingTasks() in the test code?

Unfortunately FrameThrottlingTest::ThrottleSubtreeAtomically needs the ability
to trigger specific observers to make sure we're not relying on the order in
which they are delivered.

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4406: if
(frame().document()->frame()) {
On 2016/10/25 22:29:37, dgrogan wrote:
> Removing this doesn't cause any of the DeferredLoading tests to fail. What's
it
> checking for? I'll add a test for it.

I think I saw some unrelated tests failing because of this, probably because the
document was in a bad state at the time. Let me give this another pass through
the bots to see if I can spot the problem again.

[commit-bot: I haz the power, 2016-10-26 18:31:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 18:31:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[szager1, 2016-10-26 19:55:13]

lgtm

[Sami, 2016-10-27 09:10:04]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 09:10:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2016-10-27 09:11:10]

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2272773002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4406: if
(frame().document()->frame()) {
On 2016/10/26 17:59:35, Sami wrote:
> On 2016/10/25 22:29:37, dgrogan wrote:
> > Removing this doesn't cause any of the DeferredLoading tests to fail. What's
> it
> > checking for? I'll add a test for it.
> 
> I think I saw some unrelated tests failing because of this, probably because
the
> document was in a bad state at the time. Let me give this another pass through
> the bots to see if I can spot the problem again.

Yeah, turns out we do need this -- else we get crashes like this (from
WebFrameSwapTest for example):

[28576:28576:1027/100709:5010231153622:FATAL:Document.cpp(6373)] Check failed:
frame(). 
#0 0x7f5fe7c724ce base::debug::StackTrace::StackTrace()
#1 0x7f5fe7ce19ff logging::LogMessage::~LogMessage()
#2 0x7f5fe340ea4e blink::Document::maybeRecordLoadReason()
#3 0x7f5fe382f6d6 blink::FrameView::updateRenderThrottlingStatus()
#4 0x7f5fe38304ea blink::FrameView::setupRenderThrottling()::$_0::operator()()
#5 0x7f5fe38304a2
_ZN4base8internal13FunctorTraitsIZN5blink9FrameView21setupRenderThrottlingEvE3$_0vE6InvokeIJRKNS2_14WeakPersistentIS3_EEbEEEvRKS4_DpOT_
#6 0x7f5fe3830437
_ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKZN5blink9FrameView21setupRenderThrottlingEvE3$_0JRKNS4_14WeakPersistentIS5_EEbEEEvOT_DpOT0_
#7 0x7f5fe38303e7
_ZN4base8internal7InvokerINS0_9BindStateIZN5blink9FrameView21setupRenderThrottlingEvE3$_0JNS3_14WeakPersistentIS4_EEEEEFvbEE7RunImplIRKS5_RKSt5tupleIJS7_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEEOb
#8 0x7f5fe383031c
_ZN4base8internal7InvokerINS0_9BindStateIZN5blink9FrameView21setupRenderThrottlingEvE3$_0JNS3_14WeakPersistentIS4_EEEEEFvbEE3RunEPNS0_13BindStateBaseEOb
#9 0x7f5fe34998a4 base::internal::RunMixin<>::Run()
#10 0x7f5fe3497b4c WTF::Function<>::operator()()
#11 0x7f5fe3497362 blink::ElementVisibilityObserver::onVisibilityChanged()
#12 0x7f5fe34989ff
_ZN4base8internal13FunctorTraitsIMN5blink25ElementVisibilityObserverEFvRKNS2_10HeapVectorINS2_6MemberINS2_25IntersectionObserverEntryEEELm0EEEEvE6InvokeIRKNS2_14WeakPersistentIS3_EEJSA_EEEvSC_OT_DpOT0_
#13 0x7f5fe3498932
_ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink25ElementVisibilityObserverEFvRKNS4_10HeapVectorINS4_6MemberINS4_25IntersectionObserverEntryEEELm0EEEERKNS4_14WeakPersistentIS5_EEJSC_EEEvOT_OT0_DpOT1_
#14 0x7f5fe34988a7
_ZN4base8internal7InvokerINS0_9BindStateIMN5blink25ElementVisibilityObserverEFvRKNS3_10HeapVectorINS3_6MemberINS3_25IntersectionObserverEntryEEELm0EEEEJNS3_14WeakPersistentIS4_EEEEEFvSB_EE7RunImplIRKSD_RKSt5tupleIJSF_EEJLm0EEEEvOT_OT0_NS_13IndexSequenc
eIJXspT1_EEEESB_
#15 0x7f5fe34987dc
_ZN4base8internal7InvokerINS0_9BindStateIMN5blink25ElementVisibilityObserverEFvRKNS3_10HeapVectorINS3_6MemberINS3_25IntersectionObserverEntryEEELm0EEEEJNS3_14WeakPersistentIS4_EEEEEFvSB_EE3RunEPNS0_13BindStateBaseESB_
#16 0x7f5fe34bb8f6 base::internal::RunMixin<>::Run()
#17 0x7f5fe34bb832 WTF::Function<>::operator()()
#18 0x7f5fe34b9460 blink::(anonymous
namespace)::IntersectionObserverCallbackImpl::handleEvent()
#19 0x7f5fe34b933b blink::IntersectionObserver::deliver()
#20 0x7f5fe34c0387
blink::IntersectionObserverController::deliverIntersectionObservations()
#21 0x7f5fe34c2cb7
_ZN4base8internal13FunctorTraitsIMN5blink30IntersectionObserverControllerEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_
#22 0x7f5fe34c2bda
_ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink30IntersectionObserverControllerEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_
#23 0x7f5fe34c2b62
_ZN4base8internal7InvokerINS0_9BindStateIMN5blink30IntersectionObserverControllerEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#24 0x7f5fe34c2aac
_ZN4base8internal7InvokerINS0_9BindStateIMN5blink30IntersectionObserverControllerEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
#25 0x7f5fe7c783b1
_ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_10RepeatModeE0EEEE3RunEv

[commit-bot: I haz the power, 2016-10-27 10:21:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 10:21:13]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-10-28 10:05:04]

Hey Ojan, do you think this is good to land now?

[ojan, 2016-10-31 19:37:10]

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:348: void
IntersectionObserver::setInitialState(InitialState initialState) {
This will happen to work for ElementVisibilityObserver because it only calls
observe once before setInitialState, but if any code called observe after
calling setInitialState, it wouldn't work.

I think what we want here is instead to:
1. Store a member on the IntersectionObserver that says the initial state is
Auto.
2. In observe, call setLastThresholdIndex when initial state == Auto.
3. DCHECK in setInitialState that observations() is empty. Then no need to loop
over them obviously. 
4. Put the setInitialState call after the IntersectionObserver::create in
ElementVisibilityObserver.

That way, future code that call setInitialState + observe will work right.

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:41: AUTO
The new enum naming style would be kAuto now I think.

https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx...

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:782:
->updateRenderThrottlingStatusForTesting();
I know this isn't new in this patch, so obviously don't need to fix it as part
of this patch, but do we need these ForTesting methods? Can we just call
runPendingTasks instead?

e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Defe...

I'm a huge fan of minimizing having testing-only methods in production code.

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:902:
TEST_F(FrameThrottlingTest, AllowOneAnimationFrame) {
Did we end up finding always doing the first rAF to be necessary for back
compat?

[ojan, 2016-10-31 19:38:27]

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:782:
->updateRenderThrottlingStatusForTesting();
On 2016/10/31 at 19:37:10, ojan wrote:
> I know this isn't new in this patch, so obviously don't need to fix it as part
of this patch, but do we need these ForTesting methods? Can we just call
runPendingTasks instead?
> 
> e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Defe...
> 
> I'm a huge fan of minimizing having testing-only methods in production code.

Oh, I see stefan already asked this. nm.

[ojan, 2016-10-31 19:41:12]

So, lgtm, unless there's contention around changing the enum name and the
setInitialState thing.

[Sami, 2016-11-01 10:44:48]

Thanks!

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:348: void
IntersectionObserver::setInitialState(InitialState initialState) {
On 2016/10/31 19:37:09, ojan wrote:
> This will happen to work for ElementVisibilityObserver because it only calls
> observe once before setInitialState, but if any code called observe after
> calling setInitialState, it wouldn't work.
> 
> I think what we want here is instead to:
> 1. Store a member on the IntersectionObserver that says the initial state is
> Auto.
> 2. In observe, call setLastThresholdIndex when initial state == Auto.
> 3. DCHECK in setInitialState that observations() is empty. Then no need to
loop
> over them obviously. 
> 4. Put the setInitialState call after the IntersectionObserver::create in
> ElementVisibilityObserver.
> 
> That way, future code that call setInitialState + observe will work right.

Thanks, I think this will help to avoid some surprises in the future. The only
detail I had to change was that the default initial state should be kHidden to
preserve the current behavior of IntersectionObserver.

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:41: AUTO
On 2016/10/31 19:37:09, ojan wrote:
> The new enum naming style would be kAuto now I think.
> 
>
https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx...

Done.

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/2272773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:902:
TEST_F(FrameThrottlingTest, AllowOneAnimationFrame) {
On 2016/10/31 19:37:10, ojan wrote:
> Did we end up finding always doing the first rAF to be necessary for back
> compat?

I don't have definite proof that it's required -- I just wanted to document it
as an artifact of our implementation. I know Gmail does some tricksy stuff with
offscreen iframes so they could very well need this. In any case, changing this
would mean we'd have to start sending IObs notifications synchronously (which
IIRC you wanted to do anyway?), but let's leave that to a future patch :)

[Sami, 2016-11-01 10:45:14]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-11-01 10:45:14]

The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2272773002/#ps460001
(title: "Adjust setInitialState API")

[commit-bot: I haz the power, 2016-11-01 10:45:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 12:02:52]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519
==========

[commit-bot: I haz the power, 2016-11-01 12:02:54]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2016-11-01 12:06:32]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519

Committed: https://crrev.com/5d3d4c00320aadad4a044dcb0241b86fc184b114
Cr-Commit-Position: refs/heads/master@{#428984}
==========

[commit-bot: I haz the power, 2016-11-01 12:06:34]

Patchset 24 (id:??) landed as
https://crrev.com/5d3d4c00320aadad4a044dcb0241b86fc184b114
Cr-Commit-Position: refs/heads/master@{#428984}

[please use gerrit instead, 2016-11-01 14:39:18]

A revert of this CL (patchset #24 id:460001) has been created in
https://codereview.chromium.org/2470793002/ by rouslan@chromium.org.

The reason for reverting is: Speculative revert to fix failures in
fast/loader/open-in-srcdoc-unload.html

Failed builds:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis...

Error message:
fast/loader/open-in-srcdoc-unload.html failed unexpectedly (leak detected:
({"numberOfLiveActiveDOMObjects":[2,3]}))
.

[Sami, 2016-11-01 16:27:41]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519

Committed: https://crrev.com/5d3d4c00320aadad4a044dcb0241b86fc184b114
Cr-Commit-Position: refs/heads/master@{#428984}
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182

Committed: https://crrev.com/5d3d4c00320aadad4a044dcb0241b86fc184b114
Cr-Commit-Position: refs/heads/master@{#428984}
==========

[Sami, 2016-11-01 16:28:02]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182

Committed: https://crrev.com/5d3d4c00320aadad4a044dcb0241b86fc184b114
Cr-Commit-Position: refs/heads/master@{#428984}
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182
==========

[Sami, 2016-11-01 16:29:55]

Thanks for linking to the failures. I believe there is a pre-existing leak in
fast/loader/open-in-srcdoc-unload.html which this patch is bringing to the
surface. I've filed crbug.com/661182 to track and updated the expectations here.

[Sami, 2016-11-01 16:30:32]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-11-01 16:30:33]

The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2272773002/#ps480001
(title: "Update LeakExpectations")

[commit-bot: I haz the power, 2016-11-01 16:30:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 17:56:35]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182
==========

[commit-bot: I haz the power, 2016-11-01 17:56:38]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-11-01 18:24:45]

Description was changed from

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182
==========

to

==========
Use intersection observer to control frame throttling

This patch removes the FrameView-specific code for computing viewport
visibility in favor of registering an internal intersection observer.

BUG=616519,647393,661182

Committed: https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e
Cr-Commit-Position: refs/heads/master@{#429046}
==========

[commit-bot: I haz the power, 2016-11-01 18:24:47]

Patchset 25 (id:??) landed as
https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e
Cr-Commit-Position: refs/heads/master@{#429046}

[esprehn, 2016-11-05 01:32:18]

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

[esprehn, 2016-11-05 01:32:20]

This patch broke timer throttling for hidden iframes, it removed the calls to

setCrossOrigin and setFrameVisible that were in there, those now have no callers
so I think we're never throttling off screen cross origin frame tasks which is
supposed to be shipped per:

TimerThrottlingForHiddenFrames status=stable

looks like there's no tests for this, just unit tests for the scheduler:
RepeatingTimer_FrameHidden_SameOrigin

there should really be some layout tests for timer throttling too. :)

[esprehn, 2016-11-05 03:01:11]

https://codereview.chromium.org/2272773002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2272773002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5217:
frame->view()->setupRenderThrottling();
Why do you need to call this here if you're also calling it inside setParent() ?

[Sami, 2016-11-07 11:47:42]

On 2016/11/05 01:32:20, esprehn wrote:
> This patch broke timer throttling for hidden iframes, it removed the calls to
> 
> setCrossOrigin and setFrameVisible that were in there, those now have no
callers
> so I think we're never throttling off screen cross origin frame tasks which is
> supposed to be shipped per:
> 
> TimerThrottlingForHiddenFrames status=stable
> 
> looks like there's no tests for this, just unit tests for the scheduler:
> RepeatingTimer_FrameHidden_SameOrigin
> 
> there should really be some layout tests for timer throttling too. :)

Sorry about that, must have been a rebase snafu on my part. I'll land a fix
along with some new tests once I have some time to work on this again.

FYI the master switch for hidden timer throttling is here[1] and is currently
disabled. The switch needs to be there because Finch doesn't know about Blink
features.

[1]
https://cs.chromium.org/chromium/src/content/public/common/content_features.c...