Throttle rendering pipeline for invisible iframes

Throttle rendering pipeline for invisible frames

This patch avoids computing animations (rAF), layout, 
style, compositing or painting updates for cross origin 
frames which aren't visible in the root viewport. This 
helps to reduce the amount of processing we do for
updates which would not be seen by the user anyway.

Design doc: https://docs.google.com/document/d/1Dd4qi1b_iX-OCZpelvXxizjq6dDJ76XNtk37SZEoTYQ/edit#heading=h.baug2xj0sa2b

BUG=487937

[Sami, 2015-08-28 18:31:00]

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

[Sami, 2015-08-28 18:36:47]

Hey Ojan, I've now got throttling more or less working for the entire rendering
pipeline. Wanna do a first pass to see if things look reasonable? I haven't dug
into layout tests yet (or added new ones for that matter).

The design doc has some more details:
https://docs.google.com/document/d/1Dd4qi1b_iX-OCZpelvXxizjq6dDJ76XNtk37SZEoT...

[ojan, 2015-09-01 19:56:47]

ojan@chromium.org changed reviewers:
+ sami.kyostila@gmail.com

[ojan, 2015-09-01 19:56:48]

This looks very close to me! So excited! \o/

Coincidentally, esprehn has been making a very similar set of changes to try to
fix our FOUC prevention on page load, so he might have so more feedback.

esprehn, this patch isn't ready for landed, but I figured we could give Sami
early feedback to make sure he doesn't waste time writing tests for code he's
going to have to change.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4045: // TODO(skyostil): Expand the viewport to
make it less likely to see stale content while scrolling.
I had another thought here. What if we only made sure that the frame had painted
something? So, even when the visibility is hidden, if the frame doesn't have a
DisplayItemList for it's root composited layer, then we still run the pipeline
that frame. That way we ensure that the frame has painted it's first frame of
data.

We might need to tweak that a bit base off real-world content, e.g. I'm not sure
the first frame would be enough for a good experience. Maybe we'd just do a few
frames/minute for the first 10 minutes or something like that.

I like this better than expanding the viewport rect because it means that an
iframe that's just barely offscreen won't pump it's pipeline in the common case.

WDYT?

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4063: // TODO(skyostil): Don't throttle if we're
playing audio.
Does audio require pumping frames?

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4072: if (!hasCrossedSecurityOrigin &&
parentFrameView() && m_viewportVisibilityForThrottling ==
ViewportVisibility::Hidden) {
I'm not too familiar with the cross-origin stuff. I think mkwest should be able
to help make sure this is the right code for detecting a cross-origin frame.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4083: setNeedsLayout();
Is this needed? I would expect that the needsLayout bit just wouldn't get
cleared when you were throttled. So, if you needed layout before, you'd
naturally need it again when unthrottled and vice versa.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.h:869: ViewportVisibility
m_viewportVisibilityForThrottling;
This member deserves a comment explaining why we need to the variables, i.e.
that we need to compute the visibility when the frame moves but not update it
until after the current frame of the pipeline is finished to avoid violating
assumptions of the layout/painting code.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/layout/com...
File Source/core/layout/compositing/DeprecatedPaintLayerCompositor.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/layout/com...
Source/core/layout/compositing/DeprecatedPaintLayerCompositor.cpp:248: if
(FrameView* childView = localFrame->view()) {
Nit: Looks like you're going from Frame-->FrameView-->
shouldThrottleStyleLayoutAndCompositingUpdates in a number of places. Maybe add
a shouldThrottleStyleLayoutAndCompositingUpdates to Frame that just passes
through to FrameView (and does the null check of course)? That would make the
calling code a bit simpler.

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
Source/web/WebViewImpl.cpp:1942: view->updateThrottling();
I was picturing that this would be called unconditionally at the end of
FrameView::updateLifecyclePhasesInternal (in the AllPhases branch at the
bottom). So, WebViewImpl doesn't need to know anything about it.

Is there a reason it needs to be here?

[dstockwell, 2015-09-01 23:52:23]

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

[dstockwell, 2015-09-01 23:52:24]

https://codereview.chromium.org/1246173002/diff/520001/Source/core/page/PageA...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/page/PageA...
Source/core/page/PageAnimator.cpp:46: continue;
I think we might want to split out the animation clock update (inside
DocumentAnimations::updateAnimationTimingForAnimationFrame) from below so that
it can still happen here. That would mean that while we don't run the pipeline
when throttled any query of animation state would still reflect correct timing.
If we don't update the clock here the animations will still advance logically
but their timing will be slightly out of sync with RAF.

Really there should be just one AnimationClock per page, but that's somewhat
orthogonal.

[Sami, 2015-09-02 10:07:06]

skyostil@chromium.org changed reviewers:
- sami.kyostila@gmail.com

[Sami, 2015-09-02 10:08:44]

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

[Sami, 2015-09-02 10:22:10]

skyostil@chromium.org changed reviewers:
+ mkwst@chromium.org

[Sami, 2015-09-02 10:46:03]

+esprehn

+mkwst -- Could you check that the security origin check looks sane?

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4045: // TODO(skyostil): Expand the viewport to
make it less likely to see stale content while scrolling.
On 2015/09/01 19:56:48, ojan wrote:
> I had another thought here. What if we only made sure that the frame had
painted
> something? So, even when the visibility is hidden, if the frame doesn't have a
> DisplayItemList for it's root composited layer, then we still run the pipeline
> that frame. That way we ensure that the frame has painted it's first frame of
> data.
> 
> We might need to tweak that a bit base off real-world content, e.g. I'm not
sure
> the first frame would be enough for a good experience. Maybe we'd just do a
few
> frames/minute for the first 10 minutes or something like that.
> 
> I like this better than expanding the viewport rect because it means that an
> iframe that's just barely offscreen won't pump it's pipeline in the common
case.
> 
> WDYT?

I think that would be better -- assuming we can make it work :) One problem that
I can think of (and this might be my bad knowledge about the state of slimming
paint) is that if the frame doesn't have its own backing layer, any time we
paint the paint container and skip painting a throttled frame, that frame
effectively disappears from the result. Is there a way to tell the paint system
just to reuse the previous contents if they exist?

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4063: // TODO(skyostil): Don't throttle if we're
playing audio.
On 2015/09/01 19:56:48, ojan wrote:
> Does audio require pumping frames?

I don't think so, unless you're doing something weird like mixing buffers in
rAF. This was mostly a paranoid note to myself to check if we need to do
anything special with audio here, kinda like we never tell tabs that are playing
audio that they are hidden. We can probably ignore it.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4072: if (!hasCrossedSecurityOrigin &&
parentFrameView() && m_viewportVisibilityForThrottling ==
ViewportVisibility::Hidden) {
On 2015/09/01 19:56:48, ojan wrote:
> I'm not too familiar with the cross-origin stuff. I think mkwest should be
able
> to help make sure this is the right code for detecting a cross-origin frame.

Thanks for the suggestion. FWIW this is was cargo culted from
Frame.cpp:canAccessAncestor.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4083: setNeedsLayout();
On 2015/09/01 19:56:48, ojan wrote:
> Is this needed? I would expect that the needsLayout bit just wouldn't get
> cleared when you were throttled. So, if you needed layout before, you'd
> naturally need it again when unthrottled and vice versa.

Right, I just needed something to kickstart the pipeline again.
scheduleVisualUpdate() is probably more appropriate.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.h:869: ViewportVisibility
m_viewportVisibilityForThrottling;
On 2015/09/01 19:56:48, ojan wrote:
> This member deserves a comment explaining why we need to the variables, i.e.
> that we need to compute the visibility when the frame moves but not update it
> until after the current frame of the pipeline is finished to avoid violating
> assumptions of the layout/painting code.

Definitely agreed. Added a comment.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/layout/com...
File Source/core/layout/compositing/DeprecatedPaintLayerCompositor.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/layout/com...
Source/core/layout/compositing/DeprecatedPaintLayerCompositor.cpp:248: if
(FrameView* childView = localFrame->view()) {
On 2015/09/01 19:56:48, ojan wrote:
> Nit: Looks like you're going from Frame-->FrameView-->
> shouldThrottleStyleLayoutAndCompositingUpdates in a number of places. Maybe
add
> a shouldThrottleStyleLayoutAndCompositingUpdates to Frame that just passes
> through to FrameView (and does the null check of course)? That would make the
> calling code a bit simpler.

Good idea, done. (I assume you meant LocalFrame instead of Frame).

https://codereview.chromium.org/1246173002/diff/520001/Source/core/page/PageA...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/page/PageA...
Source/core/page/PageAnimator.cpp:46: continue;
On 2015/09/01 23:52:24, dstockwell wrote:
> I think we might want to split out the animation clock update (inside
> DocumentAnimations::updateAnimationTimingForAnimationFrame) from below so that
> it can still happen here. That would mean that while we don't run the pipeline
> when throttled any query of animation state would still reflect correct
timing.
> If we don't update the clock here the animations will still advance logically
> but their timing will be slightly out of sync with RAF.
> 
> Really there should be just one AnimationClock per page, but that's somewhat
> orthogonal.

Making sure the time still advances sounds like a good idea. I'll look into
doing that.

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
Source/web/WebViewImpl.cpp:1942: view->updateThrottling();
On 2015/09/01 19:56:48, ojan wrote:
> I was picturing that this would be called unconditionally at the end of
> FrameView::updateLifecyclePhasesInternal (in the AllPhases branch at the
> bottom). So, WebViewImpl doesn't need to know anything about it.
> 
> Is there a reason it needs to be here?

Yeah, that was my assumption too but the problem is painting happens after
updateLifecyclePhasesInternal(), so we might try to paint a frame that just
became unthrottled but didn't do a lifecycle update. I'm open to suggestion here
-- maybe this callback should be more painting specific or maybe we do a forced
lifecycle update when unthrottling?

[Mike West, 2015-09-02 11:30:08]

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4074: hasCrossedSecurityOrigin =
!origin->canAccess(parentOrigin);
This looks pretty reasonable.

That said:

1. Do we need to care about the public suffix list? That is, are
"hangouts.google.com" and "myaccounts.google.com" cross-origin for purposes of
throttling? If not, you'll want to look into `OriginAccessEntry` (used in
`Document::firstPartyForCookies`, for example).

2. How will this work once the OOPIF work is done? I suppose `RemoteFrame`s
would always be cross-origin?

[ojan, 2015-09-02 11:33:27]

mpb: FYI since this overlaps a lot with IntersectionObserver implementation and
will likely use that implmentation directly once it's available.

pdr/chrishtr: See my question about DisplayItemList backings for frames.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4045: // TODO(skyostil): Expand the viewport to
make it less likely to see stale content while scrolling.
On 2015/09/02 at 10:46:02, Sami wrote:
> On 2015/09/01 19:56:48, ojan wrote:
> > I had another thought here. What if we only made sure that the frame had
painted
> > something? So, even when the visibility is hidden, if the frame doesn't have
a
> > DisplayItemList for it's root composited layer, then we still run the
pipeline
> > that frame. That way we ensure that the frame has painted it's first frame
of
> > data.
> > 
> > We might need to tweak that a bit base off real-world content, e.g. I'm not
sure
> > the first frame would be enough for a good experience. Maybe we'd just do a
few
> > frames/minute for the first 10 minutes or something like that.
> > 
> > I like this better than expanding the viewport rect because it means that an
> > iframe that's just barely offscreen won't pump it's pipeline in the common
case.
> > 
> > WDYT?
> 
> I think that would be better -- assuming we can make it work :) One problem
that I can think of (and this might be my bad knowledge about the state of
slimming paint) is that if the frame doesn't have its own backing layer, any
time we paint the paint container and skip painting a throttled frame, that
frame effectively disappears from the result. Is there a way to tell the paint
system just to reuse the previous contents if they exist?

I believe we retain the DisplayItemList, so this should just naturally work.
That said, there are future plans to throw away the DisplayItemList in some
cases (e.g. due to memory pressure). At that point, we'll need to check here if
the root layer of the frame as a DisplayItemList. If it doesn't, then I think
the right behavior is to pump just that one frame (but otherwise leave the frame
throttled) so we can get a fresh DisplayItemList for that frame in case it's
scrolled into view.

In either case, since the DisplayItemList is currently never discarded, I think
this can just be a TODO. CC chrishtr/pdr to confirm.

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4083: setNeedsLayout();
On 2015/09/02 at 10:46:02, Sami wrote:
> On 2015/09/01 19:56:48, ojan wrote:
> > Is this needed? I would expect that the needsLayout bit just wouldn't get
> > cleared when you were throttled. So, if you needed layout before, you'd
> > naturally need it again when unthrottled and vice versa.
> 
> Right, I just needed something to kickstart the pipeline again.
scheduleVisualUpdate() is probably more appropriate.

Ah, yes. scheduleVisualUpdate is the right thing for that.

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
Source/web/WebViewImpl.cpp:1942: view->updateThrottling();
On 2015/09/02 at 10:46:03, Sami wrote:
> On 2015/09/01 19:56:48, ojan wrote:
> > I was picturing that this would be called unconditionally at the end of
> > FrameView::updateLifecyclePhasesInternal (in the AllPhases branch at the
> > bottom). So, WebViewImpl doesn't need to know anything about it.
> > 
> > Is there a reason it needs to be here?
> 
> Yeah, that was my assumption too but the problem is painting happens after
updateLifecyclePhasesInternal(), so we might try to paint a frame that just
became unthrottled but didn't do a lifecycle update. I'm open to suggestion here
-- maybe this callback should be more painting specific or maybe we do a forced
lifecycle update when unthrottling?

Ah right. Doing it synchronously in updateLifecyclePhasesInternal doesn't work.
If we want to match what PositionObserver (now IntersectionObserver) is planning
to do, which I think we do if we can, then we'd do a PostTask from inside
updateLifecyclePhasesInternal. esprehn, does this match your intended direction
for the IntersectionObserver implementation?

[chrishtr, 2015-09-02 17:42:46]

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

[chrishtr, 2015-09-02 17:42:47]

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4045: // TODO(skyostil): Expand the viewport to
make it less likely to see stale content while scrolling.
On 2015/09/02 at 11:33:27, ojan wrote:
> On 2015/09/02 at 10:46:02, Sami wrote:
> > On 2015/09/01 19:56:48, ojan wrote:
> > > I had another thought here. What if we only made sure that the frame had
painted
> > > something? So, even when the visibility is hidden, if the frame doesn't
have a
> > > DisplayItemList for it's root composited layer, then we still run the
pipeline
> > > that frame. That way we ensure that the frame has painted it's first frame
of
> > > data.
> > > 
> > > We might need to tweak that a bit base off real-world content, e.g. I'm
not sure
> > > the first frame would be enough for a good experience. Maybe we'd just do
a few
> > > frames/minute for the first 10 minutes or something like that.
> > > 
> > > I like this better than expanding the viewport rect because it means that
an
> > > iframe that's just barely offscreen won't pump it's pipeline in the common
case.
> > > 
> > > WDYT?
> > 
> > I think that would be better -- assuming we can make it work :) One problem
that I can think of (and this might be my bad knowledge about the state of
slimming paint) is that if the frame doesn't have its own backing layer, any
time we paint the paint container and skip painting a throttled frame, that
frame effectively disappears from the result. Is there a way to tell the paint
system just to reuse the previous contents if they exist?
> 
> I believe we retain the DisplayItemList, so this should just naturally work.
That said, there are future plans to throw away the DisplayItemList in some
cases (e.g. due to memory pressure). At that point, we'll need to check here if
the root layer of the frame as a DisplayItemList. If it doesn't, then I think
the right behavior is to pump just that one frame (but otherwise leave the frame
throttled) so we can get a fresh DisplayItemList for that frame in case it's
scrolled into view.
> 
> In either case, since the DisplayItemList is currently never discarded, I
think this can just be a TODO. CC chrishtr/pdr to confirm.

What you will need to do is make sure that:

a) the contents of the iframe are not paint invalidated
b) the paint code still recurses into the iframe during paint

When those are true, then the paint code will be able to automatically reuse
cached information from the prior frame (DisplayItemList has this behavior), and
merge it into the new DisplayItemList.

You shouldn't worry about the performance of (b), it is part of the slimming
paint design that we have clever ways to make this fast.

(a) may not be easy if the iframe has script which causes paint invalidations.
If that is a problem, then you may need to generalize delayed paint
invalidations beyond animated GIFs (which I think would be fine, but needs
thought).

Also note that the difference between composited and non-composited iframes
w.r.t. paint will soon be gone, so even with composited iframes this problem
will arise.

[ojan, 2015-09-02 18:05:59]

On 2015/09/02 at 17:42:47, chrishtr wrote:
>
https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
> Source/core/frame/FrameView.cpp:4045: // TODO(skyostil): Expand the viewport
to make it less likely to see stale content while scrolling.
> On 2015/09/02 at 11:33:27, ojan wrote:
> > On 2015/09/02 at 10:46:02, Sami wrote:
> > > On 2015/09/01 19:56:48, ojan wrote:
> > > > I had another thought here. What if we only made sure that the frame had
painted
> > > > something? So, even when the visibility is hidden, if the frame doesn't
have a
> > > > DisplayItemList for it's root composited layer, then we still run the
pipeline
> > > > that frame. That way we ensure that the frame has painted it's first
frame of
> > > > data.
> > > > 
> > > > We might need to tweak that a bit base off real-world content, e.g. I'm
not sure
> > > > the first frame would be enough for a good experience. Maybe we'd just
do a few
> > > > frames/minute for the first 10 minutes or something like that.
> > > > 
> > > > I like this better than expanding the viewport rect because it means
that an
> > > > iframe that's just barely offscreen won't pump it's pipeline in the
common case.
> > > > 
> > > > WDYT?
> > > 
> > > I think that would be better -- assuming we can make it work :) One
problem that I can think of (and this might be my bad knowledge about the state
of slimming paint) is that if the frame doesn't have its own backing layer, any
time we paint the paint container and skip painting a throttled frame, that
frame effectively disappears from the result. Is there a way to tell the paint
system just to reuse the previous contents if they exist?
> > 
> > I believe we retain the DisplayItemList, so this should just naturally work.
That said, there are future plans to throw away the DisplayItemList in some
cases (e.g. due to memory pressure). At that point, we'll need to check here if
the root layer of the frame as a DisplayItemList. If it doesn't, then I think
the right behavior is to pump just that one frame (but otherwise leave the frame
throttled) so we can get a fresh DisplayItemList for that frame in case it's
scrolled into view.
> > 
> > In either case, since the DisplayItemList is currently never discarded, I
think this can just be a TODO. CC chrishtr/pdr to confirm.
> 
> What you will need to do is make sure that:
> 
> a) the contents of the iframe are not paint invalidated
> b) the paint code still recurses into the iframe during paint
> 
> When those are true, then the paint code will be able to automatically reuse
cached information from the prior frame (DisplayItemList has this behavior), and
merge it into the new DisplayItemList.
> 
> You shouldn't worry about the performance of (b), it is part of the slimming
paint design that we have clever ways to make this fast.
> 
> (a) may not be easy if the iframe has script which causes paint invalidations.
If that is a problem, then you may need to generalize delayed paint
invalidations beyond animated GIFs (which I think would be fine, but needs
thought).

Can you explain roughly what you have in mind here? I guess I was naively
thinking we could just use the stale DisplayItemLists inside the iframe and that
the paint code that consumes the DisplayItemLists doesn't need to know that
they're stale.

Regarding (a), a large part of the point of throttling is to avoid doing the
painting work even if the page contents have changed since the last frame (i.e.
to skip as much of the painting work as possible).

Regarding (b), why is it important that the paint code still recurse into the
iframe during paint? I guess I'm not sure what we're referring to as "paint
code". The code that generates the DisplayItemList for the iframe shouldn't need
to be called since we're intentionally trying to use the stale one, right?

> Also note that the difference between composited and non-composited iframes
w.r.t. paint will soon be gone, so even with composited iframes this problem
will arise.

[esprehn, 2015-09-02 21:10:38]

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:1324: void
FrameView::setLifecycleThrottlingModeRecursive(LifecycleThrottlingMode
lifecycleThrottlingMode)
This actually doesn't need to be recursive, you can just do
.traverseNext(*frame())

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4020: // TODO(skyostil): Replace this with a
position observer.
This needs lifecycle asserts, what phases is this allowed in? What times?

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4057: updateThrottlingRecursive(false);
This boolean is not clear.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4067: || m_viewportVisibilityForThrottling ==
ViewportVisibility::Unknown);
Lets remove the Unknown state, you're either visible or not.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4071: if (!hasCrossedSecurityOrigin &&
parentFrameView() && m_viewportVisibilityForThrottling ==
ViewportVisibility::Hidden) {
What's the reason for this?

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4094: return m_lifecycleThrottlingMode ==
LifecycleThrottlingMode::Allow
This bit seems like it should be in the DocumentLifecyle, not here.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4103: return parent()
This parent check shouldn't be needed.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:842: return
view()->shouldThrottleRenderingPipeline();
return view() && view()->...;

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:849: return
view()->shouldThrottleStyleLayoutAndCompositingUpdates();
ditto

https://codereview.chromium.org/1246173002/diff/540001/Source/core/page/PageA...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/page/PageA...
Source/core/page/PageAnimator.cpp:45: // FIXME: Keep the animation timing
ticking while throttled.
I don't think you need to do anything special here, the animation system should
be able to do an on demand update?

https://codereview.chromium.org/1246173002/diff/540001/Source/core/page/PageA...
Source/core/page/PageAnimator.cpp:99:
view->updateAllLifecyclePhases(FrameView::LifecycleThrottlingMode::Allow);
Lets un-nest the enum.

[dglazkov, 2015-09-02 21:12:05]

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

[dglazkov, 2015-09-02 21:12:06]

https://codereview.chromium.org/1246173002/diff/540001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/1246173002/diff/540001/public/web/WebWidget.h...
public/web/WebWidget.h:113: virtual void didCommitCompositorFrame() { }
Can you help me understand why this needs to be at the DidComitCompositorFrame
phase and not at the end of WebViewImpl::layout, like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[ojan, 2015-09-02 21:17:12]

https://codereview.chromium.org/1246173002/diff/540001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/1246173002/diff/540001/public/web/WebWidget.h...
public/web/WebWidget.h:113: virtual void didCommitCompositorFrame() { }
On 2015/09/02 at 21:12:06, dglazkov wrote:
> Can you help me understand why this needs to be at the DidComitCompositorFrame
phase and not at the end of WebViewImpl::layout, like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

See
https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl....

TL;DR: We do the actual paint recording after WebViewImpl::layout. We compute
the visibility at the end of layout, but we don't want it to apply mid-way
through the pipeline, we want to it apply after the current frame ends and
before the next one begins. Otherwise, we end up painting a frame that's in an
invalid state.

[Sami, 2015-09-03 14:07:56]

Thanks for all the great feedback everyone. I'll respond bit by bit as I go
through everything.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4074: hasCrossedSecurityOrigin =
!origin->canAccess(parentOrigin);
On 2015/09/02 11:30:08, Mike West (buried) wrote:
> This looks pretty reasonable.

Great, thanks for checking.
 
> That said:
> 
> 1. Do we need to care about the public suffix list? That is, are
> "hangouts.google.com" and "myaccounts.google.com" cross-origin for purposes of
> throttling? If not, you'll want to look into `OriginAccessEntry` (used in
> `Document::firstPartyForCookies`, for example).

Good question. The main thing I'm looking for here is whether frame A can
execute JS in frame B. If yes, then we shouldn't throttle B because A might
directly see the effects of throttling and the page could break. To use your
example, can hangouts.google.com poke into myaccounts.google.com (or vice versa)
and what determines that?

> 2. How will this work once the OOPIF work is done? I suppose `RemoteFrame`s
> would always be cross-origin?

Thanks for mentioning this, I hadn't really considered all the interactions with
OOPIF. My understanding is that the local <=> remote boundary will always be
cross-origin, so this logic should work. I'm more unsure about what the frame
tree looks like in the remote processes and whether they can even do any
meaningful visibility determination. I'll do some experiments to find out.

[dglazkov, 2015-09-03 16:51:28]

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
Source/web/WebViewImpl.cpp:1942: view->updateThrottling();
On 2015/09/02 at 11:33:27, ojan wrote:
> On 2015/09/02 at 10:46:03, Sami wrote:
> > On 2015/09/01 19:56:48, ojan wrote:
> > > I was picturing that this would be called unconditionally at the end of
> > > FrameView::updateLifecyclePhasesInternal (in the AllPhases branch at the
> > > bottom). So, WebViewImpl doesn't need to know anything about it.
> > > 
> > > Is there a reason it needs to be here?
> > 
> > Yeah, that was my assumption too but the problem is painting happens after
updateLifecyclePhasesInternal(), so we might try to paint a frame that just
became unthrottled but didn't do a lifecycle update. I'm open to suggestion here
-- maybe this callback should be more painting specific or maybe we do a forced
lifecycle update when unthrottling?
> 
> Ah right. Doing it synchronously in updateLifecyclePhasesInternal doesn't
work. If we want to match what PositionObserver (now IntersectionObserver) is
planning to do, which I think we do if we can, then we'd do a PostTask from
inside updateLifecyclePhasesInternal. esprehn, does this match your intended
direction for the IntersectionObserver implementation?

I am still a little bit confused, please forgive me. It seems that
DidCommitCompositorFrame originates from the tail end of the
ThreadProxy::BeginMainFrame
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...),
and the call to WebViewImpl::layout (which is here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...)
is right before it. Is there something interesting that can happen between these
two that would make any difference?

[Sami, 2015-09-03 17:09:27]

On 2015/09/03 16:51:28, dglazkov wrote:
> I am still a little bit confused, please forgive me. It seems that
> DidCommitCompositorFrame originates from the tail end of the
> ThreadProxy::BeginMainFrame
>
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...),
> and the call to WebViewImpl::layout (which is here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...)
> is right before it. Is there something interesting that can happen between
these
> two that would make any difference?

The interesting/problematic bit is the call to UpdateLayers, which ends up
calling back into Blink to paint/update recordings:

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...

[Sami, 2015-09-03 17:23:20]

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:1324: void
FrameView::setLifecycleThrottlingModeRecursive(LifecycleThrottlingMode
lifecycleThrottlingMode)
On 2015/09/02 21:10:37, esprehn wrote:
> This actually doesn't need to be recursive, you can just do
> .traverseNext(*frame())

Neat, thanks!

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4020: // TODO(skyostil): Replace this with a
position observer.
On 2015/09/02 21:10:37, esprehn wrote:
> This needs lifecycle asserts, what phases is this allowed in? What times?

Yeah, agreed. Since frameRectsChanged() is called from various places, the
lifecycle states we get here are pretty over the map. I'm thinking it'd be
better to just make a note here that the visibility info is dirty and actually
compute it at the end of the lifecycle. That way we only do it once per cycle
too.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4057: updateThrottlingRecursive(false);
On 2015/09/02 21:10:37, esprehn wrote:
> This boolean is not clear.

Replaced with an enum.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4067: || m_viewportVisibilityForThrottling ==
ViewportVisibility::Unknown);
On 2015/09/02 21:10:37, esprehn wrote:
> Lets remove the Unknown state, you're either visible or not.

Ok, let's default to visible.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4071: if (!hasCrossedSecurityOrigin &&
parentFrameView() && m_viewportVisibilityForThrottling ==
ViewportVisibility::Hidden) {
On 2015/09/02 21:10:37, esprehn wrote:
> What's the reason for this?

Added a big comment, but the tl;dr is that we want to avoid cases where frame A
depends on frame B and breaks if B is throttled. This could happen for instance
if the rAF in B drives something in A. The theory is that we can reduce the
chances this happening if we only throttle cross origin frames, because they
already need to communicate via postMessage and hence have an expectation of
some asynchronity.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4094: return m_lifecycleThrottlingMode ==
LifecycleThrottlingMode::Allow
On 2015/09/02 21:10:37, esprehn wrote:
> This bit seems like it should be in the DocumentLifecyle, not here.

Good idea, done.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:4103: return parent()
On 2015/09/02 21:10:37, esprehn wrote:
> This parent check shouldn't be needed.

Done.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:842: return
view()->shouldThrottleRenderingPipeline();
On 2015/09/02 21:10:37, esprehn wrote:
> return view() && view()->...;

Done.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:849: return
view()->shouldThrottleStyleLayoutAndCompositingUpdates();
On 2015/09/02 21:10:37, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/page/PageA...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1246173002/diff/540001/Source/core/page/PageA...
Source/core/page/PageAnimator.cpp:45: // FIXME: Keep the animation timing
ticking while throttled.
On 2015/09/02 21:10:37, esprehn wrote:
> I don't think you need to do anything special here, the animation system
should
> be able to do an on demand update?

This is referring to dstockwell@'s earlier comment about making sure the
animation time remains valid if someone queries for it
(DocumentAnimations::updateAnimationTimingForAnimationFrame) but still avoiding
any other animation related processing.

https://codereview.chromium.org/1246173002/diff/540001/Source/core/page/PageA...
Source/core/page/PageAnimator.cpp:99:
view->updateAllLifecyclePhases(FrameView::LifecycleThrottlingMode::Allow);
On 2015/09/02 21:10:37, esprehn wrote:
> Lets un-nest the enum.

Done.

[chrishtr, 2015-09-03 17:39:51]

On 2015/09/03 at 17:09:27, skyostil wrote:
> On 2015/09/03 16:51:28, dglazkov wrote:
> > I am still a little bit confused, please forgive me. It seems that
> > DidCommitCompositorFrame originates from the tail end of the
> > ThreadProxy::BeginMainFrame
> >
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...),
> > and the call to WebViewImpl::layout (which is here:
> >
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...)
> > is right before it. Is there something interesting that can happen between
these
> > two that would make any difference?
> 
> The interesting/problematic bit is the call to UpdateLayers, which ends up
calling back into Blink to paint/update recordings:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...

The sequence in this CL right now is (w/pseudocode):

// begin frame
blink::updateLifecycle(); // doesn't include paint
cc::UpdateLayers()); // among other things, paints
updateThrottling();
// end frame

updateThrottling may set bits to un-throttle an iframe, which also calls
scheduleVisualUpdate() for the next frame.
If updateLifecycle() called updateThrottling(), then the call to UpdateLayers
would pain the iframe (due to the throttling bits no longer being set) before
the frame requested in scheduleVisualUpdate().

Note that slimming paint phase 2 will integrate paint into the lifecycle, which
will make this code simpler.

I don't see where in this patch we avoid painting the throttled frame though?

[Sami, 2015-09-03 17:42:21]

On 2015/09/02 18:05:59, ojan wrote:
> > What you will need to do is make sure that:
> > 
> > a) the contents of the iframe are not paint invalidated
> > b) the paint code still recurses into the iframe during paint
> > 
> > When those are true, then the paint code will be able to automatically reuse
> cached information from the prior frame (DisplayItemList has this behavior),
and
> merge it into the new DisplayItemList.
> > 
> > You shouldn't worry about the performance of (b), it is part of the slimming
> paint design that we have clever ways to make this fast.
> > 
> > (a) may not be easy if the iframe has script which causes paint
invalidations.
> If that is a problem, then you may need to generalize delayed paint
> invalidations beyond animated GIFs (which I think would be fine, but needs
> thought).
> 
> Can you explain roughly what you have in mind here? I guess I was naively
> thinking we could just use the stale DisplayItemLists inside the iframe and
that
> the paint code that consumes the DisplayItemLists doesn't need to know that
> they're stale.
> 
> Regarding (a), a large part of the point of throttling is to avoid doing the
> painting work even if the page contents have changed since the last frame
(i.e.
> to skip as much of the painting work as possible).

Thanks for explaining the slimming paint angle here Chris. I agree that not
handling (a) would be problematic since while this patch can quench
invalidations from rAF, timers and everything would still keep ticking and
causing paints. Would it be feasible to have a toggle somewhere in the painting
code to always return the old DisplayItemList if a frame is throttled?

> Regarding (b), why is it important that the paint code still recurse into the
> iframe during paint? I guess I'm not sure what we're referring to as "paint
> code". The code that generates the DisplayItemList for the iframe shouldn't
need
> to be called since we're intentionally trying to use the stale one, right?

Chris, correct me if I'm wrong but this is needed so the compositor has valid
contents for the iframe and not just a blank region. I think this painting step
is basically just handing off the DisplayItemList to the compositor, so it
should be cheap.

[chrishtr, 2015-09-03 17:44:36]

On 2015/09/02 at 18:05:59, ojan wrote:
> On 2015/09/02 at 17:42:47, chrishtr wrote:
> >
https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
> > File Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/1246173002/diff/520001/Source/core/frame/Fram...
> > Source/core/frame/FrameView.cpp:4045: // TODO(skyostil): Expand the viewport
to make it less likely to see stale content while scrolling.
> > On 2015/09/02 at 11:33:27, ojan wrote:
> > > On 2015/09/02 at 10:46:02, Sami wrote:
> > > > On 2015/09/01 19:56:48, ojan wrote:
> > > > > I had another thought here. What if we only made sure that the frame
had painted
> > > > > something? So, even when the visibility is hidden, if the frame
doesn't have a
> > > > > DisplayItemList for it's root composited layer, then we still run the
pipeline
> > > > > that frame. That way we ensure that the frame has painted it's first
frame of
> > > > > data.
> > > > > 
> > > > > We might need to tweak that a bit base off real-world content, e.g.
I'm not sure
> > > > > the first frame would be enough for a good experience. Maybe we'd just
do a few
> > > > > frames/minute for the first 10 minutes or something like that.
> > > > > 
> > > > > I like this better than expanding the viewport rect because it means
that an
> > > > > iframe that's just barely offscreen won't pump it's pipeline in the
common case.
> > > > > 
> > > > > WDYT?
> > > > 
> > > > I think that would be better -- assuming we can make it work :) One
problem that I can think of (and this might be my bad knowledge about the state
of slimming paint) is that if the frame doesn't have its own backing layer, any
time we paint the paint container and skip painting a throttled frame, that
frame effectively disappears from the result. Is there a way to tell the paint
system just to reuse the previous contents if they exist?
> > > 
> > > I believe we retain the DisplayItemList, so this should just naturally
work. That said, there are future plans to throw away the DisplayItemList in
some cases (e.g. due to memory pressure). At that point, we'll need to check
here if the root layer of the frame as a DisplayItemList. If it doesn't, then I
think the right behavior is to pump just that one frame (but otherwise leave the
frame throttled) so we can get a fresh DisplayItemList for that frame in case
it's scrolled into view.
> > > 
> > > In either case, since the DisplayItemList is currently never discarded, I
think this can just be a TODO. CC chrishtr/pdr to confirm.
> > 
> > What you will need to do is make sure that:
> > 
> > a) the contents of the iframe are not paint invalidated
> > b) the paint code still recurses into the iframe during paint
> > 
> > When those are true, then the paint code will be able to automatically reuse
cached information from the prior frame (DisplayItemList has this behavior), and
merge it into the new DisplayItemList.
> > 
> > You shouldn't worry about the performance of (b), it is part of the slimming
paint design that we have clever ways to make this fast.
> > 
> > (a) may not be easy if the iframe has script which causes paint
invalidations. If that is a problem, then you may need to generalize delayed
paint invalidations beyond animated GIFs (which I think would be fine, but needs
thought).
> 
> Can you explain roughly what you have in mind here? I guess I was naively
thinking we could just use the stale DisplayItemLists inside the iframe and that
the paint code that consumes the DisplayItemLists doesn't need to know that
they're stale.
> 
> Regarding (a), a large part of the point of throttling is to avoid doing the
painting work even if the page contents have changed since the last frame (i.e.
to skip as much of the painting work as possible).

Fair point.

> 
> Regarding (b), why is it important that the paint code still recurse into the
iframe during paint? I guess I'm not sure what we're referring to as "paint
code". The code that generates the DisplayItemList for the iframe shouldn't need
to be called since we're intentionally trying to use the stale one, right?

Allowing recursion into the second iframe avoids putting in special-case code in
paint to try to understand this scenario. Also see below.

> 
> > Also note that the difference between composited and non-composited iframes
w.r.t. paint will soon be gone, so even with composited iframes this problem
will arise.

Work currently in progress will allow us to easily early-out right at the top of
recursing into the iframe, when the iframe is throttled, and reuse the cached
DisplayItems for the entire iframe. I think the part to support throttling will
only be a few lines of code.

We plan to ship it in M47, so it would be in place in time to go along with this
CL.

[Sami, 2015-09-03 17:46:04]

Thanks for expanding on that Chris -- I was being opaque.

On 2015/09/03 17:39:51, chrishtr wrote:
> Note that slimming paint phase 2 will integrate paint into the lifecycle,
which
> will make this code simpler.

I was just about to suggest that but looks like I don't need to :)

> I don't see where in this patch we avoid painting the throttled frame though?

That would be the two early-outs in FrameView::paint(). It was a bit difficult
to verify that they're doing what I think they are, so I'd appreciate if you
double check.

[chrishtr, 2015-09-03 18:03:58]

https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:3807: if (shouldThrottleRenderingPipeline())
This is not the right place. Instead you should put early outs into
DeprecatedPaintLayerPainter::paintLayer() and
deprecatedPaintLayerPainter::paintLayerContents().
 
Those are the entry points for recursive painting of self-painting
DeprecatedPaintLayers, and composited iframes.

However, iframes need not be self-painting. For that, you need an additional
early-out in ViewPainter::paint.

[chrishtr, 2015-09-03 18:06:57]

On 2015/09/03 at 18:03:58, chrishtr wrote:
>
https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
> Source/core/frame/FrameView.cpp:3807: if (shouldThrottleRenderingPipeline())
> This is not the right place. Instead you should put early outs into
DeprecatedPaintLayerPainter::paintLayer() and
deprecatedPaintLayerPainter::paintLayerContents().
>  
> Those are the entry points for recursive painting of self-painting
DeprecatedPaintLayers, and composited iframes.
> 
> However, iframes need not be self-painting. For that, you need an additional
early-out in ViewPainter::paint.

On further reflection, you also need to early out in FrameView::paint if you
want to avoid painting scrollbars.

[Sami, 2015-09-03 19:29:10]

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1246173002/diff/520001/Source/web/WebViewImpl...
Source/web/WebViewImpl.cpp:1942: view->updateThrottling();
> Ah right. Doing it synchronously in updateLifecyclePhasesInternal doesn't
work.
> If we want to match what PositionObserver (now IntersectionObserver) is
planning
> to do, which I think we do if we can, then we'd do a PostTask from inside
> updateLifecyclePhasesInternal. esprehn, does this match your intended
direction
> for the IntersectionObserver implementation?

I just gave the PostTask approach a try since you were going to do it for
IntersectionObserver anyway. A small problem is that any tasks (e.g., input
handling) might slip between the lifecycle update and the posted task, which
means that we can't rely on being at in a clean lifecycle state once the task
runs. I think throttling updates can be made to work regardless, but do you
think that's gonna be a problem for IntersectionObserver? Would the explicit
commit signal be better?

[dglazkov, 2015-09-03 20:50:36]

On 2015/09/03 at 18:06:57, chrishtr wrote:
> On 2015/09/03 at 18:03:58, chrishtr wrote:
> >
https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
> > File Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
> > Source/core/frame/FrameView.cpp:3807: if (shouldThrottleRenderingPipeline())
> > This is not the right place. Instead you should put early outs into
DeprecatedPaintLayerPainter::paintLayer() and
deprecatedPaintLayerPainter::paintLayerContents().
> >  
> > Those are the entry points for recursive painting of self-painting
DeprecatedPaintLayers, and composited iframes.
> > 
> > However, iframes need not be self-painting. For that, you need an additional
early-out in ViewPainter::paint.
> 
> On further reflection, you also need to early out in FrameView::paint if you
want to avoid painting scrollbars.

Thanks for the explanation, this is really helpful. BTW, just a smell -- these
early returns seem to be indicative of a design/layering problem.

[chrishtr, 2015-09-03 20:54:54]

On 2015/09/03 at 20:50:36, dglazkov wrote:
> On 2015/09/03 at 18:06:57, chrishtr wrote:
> > On 2015/09/03 at 18:03:58, chrishtr wrote:
> > >
https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
> > > File Source/core/frame/FrameView.cpp (right):
> > > 
> > >
https://codereview.chromium.org/1246173002/diff/560001/Source/core/frame/Fram...
> > > Source/core/frame/FrameView.cpp:3807: if
(shouldThrottleRenderingPipeline())
> > > This is not the right place. Instead you should put early outs into
DeprecatedPaintLayerPainter::paintLayer() and
deprecatedPaintLayerPainter::paintLayerContents().
> > >  
> > > Those are the entry points for recursive painting of self-painting
DeprecatedPaintLayers, and composited iframes.
> > > 
> > > However, iframes need not be self-painting. For that, you need an
additional early-out in ViewPainter::paint.
> > 
> > On further reflection, you also need to early out in FrameView::paint if you
want to avoid painting scrollbars.
> 
> Thanks for the explanation, this is really helpful. BTW, just a smell -- these
early returns seem to be indicative of a design/layering problem.

Yep. Painting across iframes is not cleanly separated.

[Sami, 2015-09-04 16:59:20]

Painting:

Chris and I talked about the different paint throttling options here and agreed
that having the early outs is probably the best option at the moment. Once
Slimming Paint's subtree caching
(https://code.google.com/p/chromium/issues/detail?id=410097) lands, we can unify
the early outs with the ones that code is adding. There's a currently corner
case where non-self-painting iframes won't be cacheable, but it sounds like we
might be able to lift that limitation.

OOPIF:

I did some testing with OOPIF at ToT and noticed that rAFs from cross origin
frames aren't getting reliably serviced. I think we need to fix that first
before trying to implement OOPIF throttling. I'll file a bug.

[chrishtr, 2015-09-04 17:30:46]

On 2015/09/04 at 16:59:20, skyostil wrote:
> Painting:
> 
> Chris and I talked about the different paint throttling options here and
agreed that having the early outs is probably the best option at the moment.
Once Slimming Paint's subtree caching
(https://code.google.com/p/chromium/issues/detail?id=410097) lands, we can unify
the early outs with the ones that code is adding. There's a currently corner
case where non-self-painting iframes won't be cacheable, but it sounds like we
might be able to lift that limitation.

Good news: I looked in more detail, and what actually paints the frame contents
is a DeprecatedPaintLayer for the LayoutView of the frame, and LayoutView always
has a self-painting DeprecatedPaintLayer.  Therefore there is no self-painting
case to worry about.

In addition, when painting a sub-frame, there is only one recursion into it,
during the foreground paint phase. Therefore the caching system Xianzhu is
working on will work correctly out of the box.

> 
> OOPIF:
> 
> I did some testing with OOPIF at ToT and noticed that rAFs from cross origin
frames aren't getting reliably serviced. I think we need to fix that first
before trying to implement OOPIF throttling. I'll file a bug.

[chrishtr, 2015-09-04 17:32:04]

https://codereview.chromium.org/1246173002/diff/600001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/600001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:3799: if (shouldThrottleRenderingPipeline())
Add a TODO to get rid of this in favor of painting cached scrollbars. It should
be feasible to put special code in FramePainter to cache these scrollbars.

https://codereview.chromium.org/1246173002/diff/600001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1246173002/diff/600001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:95: if
(isThrottledFrameView(m_paintLayer.layoutObject()))
Add TODO here and below about subsequence caching.

[Sami, 2015-09-07 17:27:17]

Thanks Chris, I've added some TODOs.

I think the last remaining architectural issue here is the
didCommitCompositorFrame() signal vs. posting a post-lifecycle task. I kinda
prefer the notification since it'll always run at a predictable time regardless
of other posted work. Ojan, Dimitri, WDYT?

https://codereview.chromium.org/1246173002/diff/600001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/600001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:3799: if (shouldThrottleRenderingPipeline())
On 2015/09/04 17:32:04, chrishtr wrote:
> Add a TODO to get rid of this in favor of painting cached scrollbars. It
should
> be feasible to put special code in FramePainter to cache these scrollbars.

Done.

https://codereview.chromium.org/1246173002/diff/600001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1246173002/diff/600001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:95: if
(isThrottledFrameView(m_paintLayer.layoutObject()))
On 2015/09/04 17:32:04, chrishtr wrote:
> Add TODO here and below about subsequence caching.

Done.

[esprehn, 2015-09-21 21:27:21]

https://codereview.chromium.org/1246173002/diff/720001/LayoutTests/fast/repai...
File LayoutTests/fast/repaint/resources/ensure-no-repaints.js (right):

https://codereview.chromium.org/1246173002/diff/720001/LayoutTests/fast/repai...
LayoutTests/fast/repaint/resources/ensure-no-repaints.js:8: var frameCount = 10;
This is racy, we don't want to do this. Can you write a Sim* test instead? See
DocumentLoadingRenderingTest. You want to add real ASSERTs at the repaint/paint
level.

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:3986: // during painting (e.g., because of block
selection gaps).
huh?

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
File Source/core/frame/FrameViewTest.cpp (right):

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
Source/core/frame/FrameViewTest.cpp:22: class MockChromeClient final : public
EmptyChromeClient {
Can we just write a Sim test instead?

[Sami, 2015-09-23 18:31:07]

Thanks for the sim test suggestion Elliott. I've merged the layout and unit
tests into a common test suite.

I found a bug where scrolling a throttled iframe into view sometimes doesn't
make it repaint and it remains transparent. I'll dig into that more.

https://codereview.chromium.org/1246173002/diff/720001/LayoutTests/fast/repai...
File LayoutTests/fast/repaint/resources/ensure-no-repaints.js (right):

https://codereview.chromium.org/1246173002/diff/720001/LayoutTests/fast/repai...
LayoutTests/fast/repaint/resources/ensure-no-repaints.js:8: var frameCount = 10;
On 2015/09/21 21:27:20, esprehn wrote:
> This is racy, we don't want to do this. Can you write a Sim* test instead? See
> DocumentLoadingRenderingTest. You want to add real ASSERTs at the
repaint/paint
> level.

Neat, I did't know those were a thing. Done.

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:3986: // during painting (e.g., because of block
selection gaps).
On 2015/09/21 21:27:20, esprehn wrote:
> huh?

Yeah :( I found out that block selections can invalidate compositing inputs
during painting, see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

Added a pointer to that function here.

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
File Source/core/frame/FrameViewTest.cpp (right):

https://codereview.chromium.org/1246173002/diff/720001/Source/core/frame/Fram...
Source/core/frame/FrameViewTest.cpp:22: class MockChromeClient final : public
EmptyChromeClient {
On 2015/09/21 21:27:21, esprehn wrote:
> Can we just write a Sim test instead?

Looks much nicer that way. Done. One minor issue was that I needed to make the
test a friend of the FrameView since it wants to check its visibility.
Alternatively I could add a public test-only getter for visibility.

[Sami, 2015-09-24 15:58:23]

Turns out you can't change the base url of an issue, so I've migrated to over to
https://codereview.chromium.org/1364063007/.