Throttle rendering pipeline for invisible frames

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
Committed: https://crrev.com/8ab2ba28e9dafe5b45ad914a274197128017221f
Cr-Commit-Position: refs/heads/master@{#354780}

[Sami, 2015-09-24 15:57:24]

skyostil@chromium.org changed reviewers:
+ chrishtr@chromium.org, dglazkov@chromium.org, dstockwell@chromium.org,
esprehn@chromium.org, mkwst@chromium.org, ojan@chromium.org

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

(Migrated from https://codereview.chromium.org/1246173002/)

[chrishtr, 2015-09-25 18:25:00]

LGTM for paint. I also talked with Xianzhu and we agree that subsequence caching
will solve the painting problem nicely for you. Xianzhu is finishing up
subsequence caching now, and we can connect it up to your work after you commit
this CL.

[ojan, 2015-10-01 06:25:55]

This patch looks great! I really like the unittests for this (kudos to esprehn
for building this framework). As you said, the only thing to resolve is the
timing of the finalizing.

It's not clear to me why posting a task is more complicated to reason about than
doing it at didCommitCompositorFrame timing. They are both async (right?), so
any arbitrary work can fit between them. With our DocumentLifeCycle asserts, you
can have confidence that your task won't get called back in a weird place (e.g.
in the middle of layout). And, if it does, that points to a bug somewhere else
in the system that we would need to fix.

Make sense?

BTW, I'll be at scheduling summit next week, so we can def chat about this in
person there. Happy to continue the conversation online before then.

So sorry it took so long to get back to reviewing this.

[esprehn, 2015-10-01 08:21:37]

Sim is modeling the real lifecycle, don't call layout() or mess with
setNeedsLayout or setNeedsAnimate manually. You're the scheduler, it's your job
to run some tasks, call m_compositor.beginFrame(), maybe mutate the DOM, then
call it again. If the compositor needs to call back to blink sometimes do you
might want to add m_compositor.finishedFrame() which is that posted callback.

If you want the iframes to be moved then move them by getting the <iframe> node
and setting style on it or scrolling the outer page. Calling move() like you're
doing can't really happen in a real page.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:95:
frameView->finalizeLifecycleUpdate();
m_compositor.beginFrame()

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:108:
webView().layout();
ditto, layout is only triggered from frames, so you want
m_compositor.beginFrame()

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:110:
frameView->finalizeLifecycleUpdate();
and then you don't need this

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:130:
grandChildFrameView->move(0, frameView->height());
this can't happen in a real page, set some style that moves the frame then run
m_compositor.beginFrame()

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:138:
frameView->finalizeLifecycleUpdate();
why would this happen again in a real page?

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:153:
childFrameView->move(0, frameView->height());
move with style, real pages can't go through ->move() without a layout

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:155:
frameView->finalizeLifecycleUpdate();
ditto, beginFrame()

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:160:
childFrameView->setNeedsLayout();
don't artificially cause layout, do something real. setNeedsLayout isn't really
a public API like this

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:162:
frameView->updateAllLifecyclePhases(DocumentLifecycle::ThrottlingMode::Allow);
this can't really happen

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:177:
childFrameView->move(0, frameView->height());
ditto

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:186:
m_layerTreeView.clearNeedsAnimate();
beginFrame() clears this bit, real pages never clear the bit without running all
the steps

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:218:
webView().layout();
beginFrame()

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:219:
frameView->finalizeLifecycleUpdate();
How does this happen in a real page? You cause an artificial layout which is
impossible since layout() only happens from inside beginFrame(), then you
finalize and then run another frame

[Sami, 2015-10-01 15:39:33]

Thanks for the comments guys. I'll respond to Ojan first and then dig into
Elliott's test suggestions.

On 2015/10/01 06:25:55, ojan wrote:
> This patch looks great! I really like the unittests for this (kudos to esprehn
> for building this framework). As you said, the only thing to resolve is the
> timing of the finalizing.
> 
> It's not clear to me why posting a task is more complicated to reason about
than
> doing it at didCommitCompositorFrame timing. They are both async (right?), so
> any arbitrary work can fit between them. With our DocumentLifeCycle asserts,
you
> can have confidence that your task won't get called back in a weird place
(e.g.
> in the middle of layout). And, if it does, that points to a bug somewhere else
> in the system that we would need to fix.
> 
> Make sense?

Actually didCommitCompositorFrame is sync (because it is called inside
BeginMainFrame) and the posted task is async, and that is the problem. The
specific bug I ran into was that something like a timer slipped in between
BeginMainFrame and the posted task and mutated the DOM, so that when I went to
recompute the visibility of frames, the lifecycle state was no longer >=
LayoutClean. I guess I could just ignore the lifecycle state and work with the
most recent coordinates at that point, but IMO a deterministic signal feels
cleaner.

> BTW, I'll be at scheduling summit next week, so we can def chat about this in
> person there. Happy to continue the conversation online before then.

Great, see you there!

[Sami, 2015-10-05 17:58:25]

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:95:
frameView->finalizeLifecycleUpdate();
On 2015/10/01 08:21:37, esprehn wrote:
> m_compositor.beginFrame()

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:108:
webView().layout();
On 2015/10/01 08:21:37, esprehn wrote:
> ditto, layout is only triggered from frames, so you want
> m_compositor.beginFrame()

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:110:
frameView->finalizeLifecycleUpdate();
On 2015/10/01 08:21:36, esprehn wrote:
> and then you don't need this

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:130:
grandChildFrameView->move(0, frameView->height());
On 2015/10/01 08:21:36, esprehn wrote:
> this can't happen in a real page, set some style that moves the frame then run
> m_compositor.beginFrame()

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:138:
frameView->finalizeLifecycleUpdate();
On 2015/10/01 08:21:36, esprehn wrote:
> why would this happen again in a real page?

You're right, it wouldn't. To make this more realistic I've split it into two
tests: one for same origin frames and another for cross origin ones.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:153:
childFrameView->move(0, frameView->height());
On 2015/10/01 08:21:36, esprehn wrote:
> move with style, real pages can't go through ->move() without a layout

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:155:
frameView->finalizeLifecycleUpdate();
On 2015/10/01 08:21:36, esprehn wrote:
> ditto, beginFrame()

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:160:
childFrameView->setNeedsLayout();
On 2015/10/01 08:21:37, esprehn wrote:
> don't artificially cause layout, do something real. setNeedsLayout isn't
really
> a public API like this

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:162:
frameView->updateAllLifecyclePhases(DocumentLifecycle::ThrottlingMode::Allow);
On 2015/10/01 08:21:37, esprehn wrote:
> this can't really happen

Replaced with another beginFrame().

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:177:
childFrameView->move(0, frameView->height());
On 2015/10/01 08:21:37, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:186:
m_layerTreeView.clearNeedsAnimate();
On 2015/10/01 08:21:36, esprehn wrote:
> beginFrame() clears this bit, real pages never clear the bit without running
all
> the steps

Thanks, replaced this with beginFrame too, but had to make sure it clears the
needsAnimate bit before actually doing the beginFrame (like the real
compositor).

https://codereview.chromium.org/1364063007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:219:
frameView->finalizeLifecycleUpdate();
On 2015/10/01 08:21:36, esprehn wrote:
> How does this happen in a real page? You cause an artificial layout which is
> impossible since layout() only happens from inside beginFrame(), then you
> finalize and then run another frame

Replaced this with a series of composites that show more clearly what should
happen and when.

[Sami, 2015-10-13 15:18:12]

Ojan and I had a great whiteboard session about how this throttling could be
done in a way that's closer to the IntersectionObserver model, and I've now
reworked the patch accordingly.

The main difference is that whenever the viewport intersection of a frame
becomes empty or non-empty during layout, we post a notification task. This task
then checks whether the frame should now be throttled or not based on the most
recent intersected bounds.

Ojan, wanna take a look?

[ojan, 2015-10-13 23:31:29]

Very close! Just a few small issues.

Also, this patch reemphasizes to me the importance of us merging the frame tree
walks up into updateLifecyclePhasesInternal. I'm not sure which team should own
that work though. It could easily be any of style, layout, paint.

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

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2432:
setLifecycleThrottlingModeForSubtree(throttlingMode);
This call and the one below are unconditional walks of the whole frame tree on
every animation frame. We should avoid that if we can (e.g. it hurts our
null-raf-loop performance). In the future, we really should work towards a world
in which we do the frame tree walking all in updateLifecyclePhasesInternal and
not in each sub-function. Then we'd be able to simplify a lot of this patch.

I don't want to block this patch on that though. So, it's gross, but until we
get to that future world, can we just have a static bool about whether we're
throttling? This takes advantage of that fact that we're single threaded and we
have a clear path towards getting rid of it, so I'm OK with it being a bit gross
for now.

WDYT?

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

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:570: bool
shouldThrottleStyleLayoutAndCompositingUpdates() const;
These names are confusing. They're both basically throttling the rendering
pipeline. How about shouldThrottleRenderingPipeline and
shouldThrottleRenderingPipelineIfAllowed? I don't love those names either, but
they're a bit closer to what they do.

Actually, the word pipeline is a bit redundant too. How about
shouldThrottleRendering and shouldThrottleRenderingIfAllowed?

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:63:
document->serviceScriptedAnimations(monotonicAnimationStartTime);
I think you want to skip these two lines as well when throttling. I'm curious if
we want to execute this code for a document that doesn't have a view. Maybe add
a TODO to investigate that? I don't want to block this patch on figuring that
out.

[Sami, 2015-10-14 16:52:11]

Thanks Ojan.

I also noticed a bug with synchronous/forced layouts where throttling was
preventing them from completing. Document::updateLayout() now disables
throttling for this reason and there's a new test too.

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

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2432:
setLifecycleThrottlingModeForSubtree(throttlingMode);
On 2015/10/13 23:31:29, ojan wrote:
> This call and the one below are unconditional walks of the whole frame tree on
> every animation frame. We should avoid that if we can (e.g. it hurts our
> null-raf-loop performance). In the future, we really should work towards a
world
> in which we do the frame tree walking all in updateLifecyclePhasesInternal and
> not in each sub-function. Then we'd be able to simplify a lot of this patch.
> 
> I don't want to block this patch on that though. So, it's gross, but until we
> get to that future world, can we just have a static bool about whether we're
> throttling? This takes advantage of that fact that we're single threaded and
we
> have a clear path towards getting rid of it, so I'm OK with it being a bit
gross
> for now.
> 
> WDYT?

Yeah, I wasn't crazy about the extra tree walks either. I tried a few options
and I think the static global is the least worst option we have without doing a
larger refactoring. Basically we need a place to stash data that affects
everything under a common layout root. LocalFrame::localFrameRoot() was the
other way I looked at but it's marked as a "temporary hack" and also does a tree
walk for every call.

I've also added an early out to the intersection update walk to avoid doing it
if nothing changed in the subtree.

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

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:570: bool
shouldThrottleStyleLayoutAndCompositingUpdates() const;
On 2015/10/13 23:31:29, ojan wrote:
> These names are confusing. They're both basically throttling the rendering
> pipeline. How about shouldThrottleRenderingPipeline and
> shouldThrottleRenderingPipelineIfAllowed? I don't love those names either, but
> they're a bit closer to what they do.
> 
> Actually, the word pipeline is a bit redundant too. How about
> shouldThrottleRendering and shouldThrottleRenderingIfAllowed?

Agreed, I like those better. Changed and reworded the comments to make the
distinction clearer.

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1364063007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:63:
document->serviceScriptedAnimations(monotonicAnimationStartTime);
On 2015/10/13 23:31:29, ojan wrote:
> I think you want to skip these two lines as well when throttling. I'm curious
if
> we want to execute this code for a document that doesn't have a view. Maybe
add
> a TODO to investigate that? I don't want to block this patch on figuring that
> out.

These two are already being skipped thanks to the "continue" above. It's not
very clear from the way the code is laid out -- but I don't have good ideas for
making it more obvious assuming these two also need to be run when we don't have
a view.

Added a TODO for finding out if these should also be skipped when we don't have
a view.

[esprehn, 2015-10-14 22:09:47]

this looks really good

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1534: if (!view() ||
!view()->shouldThrottleRendering())
we shouldn't be able to get here without a view(), it would hit one of the
asserts above because the document wouldn't be isActive(). no need to null check
view()

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1795: if (view() &&
view()->shouldThrottleRenderingIfAllowed())
you must have a view here, no need to null check, you can add an
ASSERT(isActive()) if you want

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1915:
lifecycle().setThrottlingMode(DocumentLifecycle::ThrottlingMode::Disallow);
Lets add a scope object for this instead of manual setting/unsetting.

DocumentLifecycle::ThrottleScope

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:43: static
DocumentLifecycle::ThrottlingMode s_throttlingMode =
DocumentLifecycle::ThrottlingMode::Allow;
unsigned s_throttleCount

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentLifecycle.h:163: ThrottlingMode
throttlingMode() const;
You want to add a ThrottleScope, just like the DetachScope. You don't need the
mode enum at all, just use an integer you increment/decrement. That avoids
having nested calls to something unset it by mistake.

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1072: if
(shouldThrottleRendering())
why not the ifAllowed() version? it's confusing when you should use each one

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1706: if
(!shouldThrottleRendering())
this is suble, why not the IfAllowed() version?

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2408: void
FrameView::updateLifecycleToLayoutClean(DocumentLifecycle::ThrottlingMode
throttlingMode)
you can remove all these and just use the static getter

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2423:
lifecycle().setThrottlingMode(throttlingMode);
use the ThrottleScope object

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3986: while (parent) {
for (FrameView* parent = parentFrameView(); parent; parent =
parent->parentFrameView())

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4052:
ASSERT(!isInPerformLayout());
!inStyleRecalc() as well

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4069: while (parentFrame) {
for loop

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4085: return
shouldThrottleRendering() && lifecycle().throttlingMode() ==
DocumentLifecycle::ThrottlingMode::Allow;
static getter

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4092: return
m_viewportVisibilityForThrottling == ViewportVisibility::Hidden
m_isViewportVisibleForThrottling and make it a boolean

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:234: void
updateAllLifecyclePhases(DocumentLifecycle::ThrottlingMode =
DocumentLifecycle::ThrottlingMode::Disallow, const LayoutRect* interestRect =
nullptr);
We shouldn't need to pass this as an argument, just have callers check the
static. If you want to call this with throttling put a
DocumentLifecyle::ThrottleScope on the stack. To check call the static bool
DocumentLifecycle::isThrottled()

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:864: enum class
SecurityOriginStatus { IsSameOrigin, IsCrossOrigin };
ditto, this can just be a boolean

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:870: ViewportVisibility
m_viewportVisibilityForThrottling;
this should just be a boolean

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1201: if (frameView &&
frameView->shouldThrottleRenderingIfAllowed())
ditto, the frameView must not be null if the LayoutObject/LayoutView exist

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3253: if (!frameView() ||
!frameView()->shouldThrottleRendering())
no null check

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3266:
frame()->page()->animator().scheduleVisualUpdate(); // In case that this is
called outside of FrameView::updateLayoutAndStyleForPainting().
this should be inside a helper so we don't need this if
shouldThrottleRendering() thing in many places

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:619:
ASSERT(lifecycle().state() >= DocumentLifecycle::PaintInvalidationClean ||
(m_layoutView.frameView() &&
m_layoutView.frameView()->shouldThrottleRendering()));
If there's a compositor there must be a FrameView, no need to null check

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:893:
|| (view && view->shouldThrottleRendering()));
ditto, no need for null check

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:97:
view->updateAllLifecyclePhases(DocumentLifecycle::ThrottlingMode::Allow);
DocumentLifecycle::ThrottleAllowedScope on the stack

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h:66:
document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToCompositingCleanPlusScrolling,
DocumentLifecycle::ThrottlingMode::Disallow, nullptr);
ditto

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:48: static bool
isThrottledFrameView(const LayoutObject* layoutObject)
I'd put this on LayoutPart, and do 

->isLayoutPart() && toLayoutPart(thing)->isThrottledFrameView()

at the two callers

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:43:
webView().layout();
You want to use SimRequest instead, don't use innerHTML to load the main page

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:48:
setInnerHTML("<iframe sandbox=\"\" id=\"frame\"></iframe>");
use single quotes, (or no quotes) you don't need the backslashes.

"<iframe sandbox id=frame></iframe>"

also you want to use SimRequest

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:72: return
frameView->m_viewportVisibilityForThrottling;
add a getter, don't reach into members

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:154:
setInnerHTML("<iframe id=\"frame\" srcdoc=\"<iframe
sandbox></iframe>\"></iframe>");
ditto

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:221:
setInnerHTML("<iframe id=\"frame\" sandbox=\"\" srcdoc=\"<style> html {
background: red; } </style>\"></iframe>");
remove quotes and us single quotes, remove black slashes and it's much easier to
read.

[ojan, 2015-10-15 04:18:26]

I'm happy with this once you address Elliott's comments. Elliott, please
l-g-t-m+cq when you're happy.

Now that we have the static, I think Elliott's right that we can merge the
shouldThrottleRendering methods and just always check the static. That makes the
checks a bit less error-prone too.

[Sami, 2015-10-16 16:48:09]

Thanks for the scoping idea -- it makes many things here much cleaner.

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1534: if (!view() ||
!view()->shouldThrottleRendering())
On 2015/10/14 22:09:46, esprehn wrote:
> we shouldn't be able to get here without a view(), it would hit one of the
> asserts above because the document wouldn't be isActive(). no need to null
check
> view()

Thanks, I didn't realize isActive() implies view().

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1795: if (view() &&
view()->shouldThrottleRenderingIfAllowed())
On 2015/10/14 22:09:46, esprehn wrote:
> you must have a view here, no need to null check, you can add an
> ASSERT(isActive()) if you want

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1915:
lifecycle().setThrottlingMode(DocumentLifecycle::ThrottlingMode::Disallow);
On 2015/10/14 22:09:45, esprehn wrote:
> Lets add a scope object for this instead of manual setting/unsetting.
> 
> DocumentLifecycle::ThrottleScope

Much cleaner, done. In case it isn't obvious, I made the scope signify that
throttling is *not* allowed because that is generally what call sites want to do
(e.g., doing a synchronous style recalc inside a context that otherwise allows
throttling).

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:43: static
DocumentLifecycle::ThrottlingMode s_throttlingMode =
DocumentLifecycle::ThrottlingMode::Allow;
On 2015/10/14 22:09:46, esprehn wrote:
> unsigned s_throttleCount

Done (made it s_throttlingDisallowCount since that's really what the scopes are
doing.)

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentLifecycle.h:163: ThrottlingMode
throttlingMode() const;
On 2015/10/14 22:09:46, esprehn wrote:
> You want to add a ThrottleScope, just like the DetachScope. You don't need the
> mode enum at all, just use an integer you increment/decrement. That avoids
> having nested calls to something unset it by mistake.

Done.

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1072: if
(shouldThrottleRendering())
On 2015/10/14 22:09:46, esprehn wrote:
> why not the ifAllowed() version? it's confusing when you should use each one

Agreed. How about this:

- shouldThrottleRendering() means the frame is actively throttling rendering and
the lifecycle bit allows it.

- canThrottleRendering() means the frame could throttle rendering. Basically
this becomes just an internal helper for FrameView which isn't called elsewhere.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1706: if
(!shouldThrottleRendering())
On 2015/10/14 22:09:46, esprehn wrote:
> this is suble, why not the IfAllowed() version?

Fixed.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2408: void
FrameView::updateLifecycleToLayoutClean(DocumentLifecycle::ThrottlingMode
throttlingMode)
On 2015/10/14 22:09:46, esprehn wrote:
> you can remove all these and just use the static getter

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2423:
lifecycle().setThrottlingMode(throttlingMode);
On 2015/10/14 22:09:46, esprehn wrote:
> use the ThrottleScope object

Done. (Actually there's no need to do anything here since throttling is allowed
by default.)

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3986: while (parent) {
On 2015/10/14 22:09:46, esprehn wrote:
> for (FrameView* parent = parentFrameView(); parent; parent =
> parent->parentFrameView())

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4052:
ASSERT(!isInPerformLayout());
On 2015/10/14 22:09:46, esprehn wrote:
> !inStyleRecalc() as well

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4069: while (parentFrame) {
On 2015/10/14 22:09:46, esprehn wrote:
> for loop

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4085: return
shouldThrottleRendering() && lifecycle().throttlingMode() ==
DocumentLifecycle::ThrottlingMode::Allow;
On 2015/10/14 22:09:46, esprehn wrote:
> static getter

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4092: return
m_viewportVisibilityForThrottling == ViewportVisibility::Hidden
On 2015/10/14 22:09:46, esprehn wrote:
> m_isViewportVisibleForThrottling and make it a boolean

Done.

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:234: void
updateAllLifecyclePhases(DocumentLifecycle::ThrottlingMode =
DocumentLifecycle::ThrottlingMode::Disallow, const LayoutRect* interestRect =
nullptr);
On 2015/10/14 22:09:46, esprehn wrote:
> We shouldn't need to pass this as an argument, just have callers check the
> static. If you want to call this with throttling put a
> DocumentLifecyle::ThrottleScope on the stack. To check call the static bool
> DocumentLifecycle::isThrottled()

Yeah, that's much cleaner. Thanks for the idea.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:864: enum class
SecurityOriginStatus { IsSameOrigin, IsCrossOrigin };
On 2015/10/14 22:09:46, esprehn wrote:
> ditto, this can just be a boolean

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:870: ViewportVisibility
m_viewportVisibilityForThrottling;
On 2015/10/14 22:09:46, esprehn wrote:
> this should just be a boolean

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1201: if (frameView &&
frameView->shouldThrottleRenderingIfAllowed())
On 2015/10/14 22:09:46, esprehn wrote:
> ditto, the frameView must not be null if the LayoutObject/LayoutView exist

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3253: if (!frameView() ||
!frameView()->shouldThrottleRendering())
On 2015/10/14 22:09:46, esprehn wrote:
> no null check

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3266:
frame()->page()->animator().scheduleVisualUpdate(); // In case that this is
called outside of FrameView::updateLayoutAndStyleForPainting().
On 2015/10/14 22:09:46, esprehn wrote:
> this should be inside a helper so we don't need this if
> shouldThrottleRendering() thing in many places

Added Frame::scheduleVisualUpdateUnlessThrottled().

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:619:
ASSERT(lifecycle().state() >= DocumentLifecycle::PaintInvalidationClean ||
(m_layoutView.frameView() &&
m_layoutView.frameView()->shouldThrottleRendering()));
On 2015/10/14 22:09:46, esprehn wrote:
> If there's a compositor there must be a FrameView, no need to null check

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:893:
|| (view && view->shouldThrottleRendering()));
On 2015/10/14 22:09:46, esprehn wrote:
> ditto, no need for null check

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:97:
view->updateAllLifecyclePhases(DocumentLifecycle::ThrottlingMode::Allow);
On 2015/10/14 22:09:46, esprehn wrote:
> DocumentLifecycle::ThrottleAllowedScope on the stack

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h:66:
document().view()->updateLifecyclePhasesInternal(FrameView::OnlyUpToCompositingCleanPlusScrolling,
DocumentLifecycle::ThrottlingMode::Disallow, nullptr);
On 2015/10/14 22:09:46, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:48: static bool
isThrottledFrameView(const LayoutObject* layoutObject)
On 2015/10/14 22:09:46, esprehn wrote:
> I'd put this on LayoutPart, and do 
> 
> ->isLayoutPart() && toLayoutPart(thing)->isThrottledFrameView()
> 
> at the two callers

Thanks, much better.

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

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:43:
webView().layout();
On 2015/10/14 22:09:47, esprehn wrote:
> You want to use SimRequest instead, don't use innerHTML to load the main page

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:48:
setInnerHTML("<iframe sandbox=\"\" id=\"frame\"></iframe>");
On 2015/10/14 22:09:46, esprehn wrote:
> use single quotes, (or no quotes) you don't need the backslashes.
> 
> "<iframe sandbox id=frame></iframe>"
> 
> also you want to use SimRequest

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:72: return
frameView->m_viewportVisibilityForThrottling;
On 2015/10/14 22:09:47, esprehn wrote:
> add a getter, don't reach into members

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:154:
setInnerHTML("<iframe id=\"frame\" srcdoc=\"<iframe
sandbox></iframe>\"></iframe>");
On 2015/10/14 22:09:47, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1364063007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:221:
setInnerHTML("<iframe id=\"frame\" sandbox=\"\" srcdoc=\"<style> html {
background: red; } </style>\"></iframe>");
On 2015/10/14 22:09:47, esprehn wrote:
> remove quotes and us single quotes, remove black slashes and it's much easier
to
> read.

Done.

[esprehn, 2015-10-16 21:40:45]

lgtm, your patch is awesome!

http://i.imgur.com/0tc6uqt.gif

fyi: isActive() implies layoutView() is not null, frameView() is not null, and
Frame is not null, I'll add some docs.

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:72:
m_lifecycle.incrementPreventThrottlingCount();
if you're going to put these out of line you don't need the increment/decrement
methods, just access the s_preventThrottlingCount directly. DetachScope uses
methods because it's inline.

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutPart.h (right):

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutPart.h:57: bool
isThrottledFrameView();
const ?

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:62: // TODO(skyostil): Find
out if these functions need to be run for documents without views.
They definitely shouldn't run for documents without views, that's a bug.

[Sami, 2015-10-19 10:51:06]

Thanks for all the feedback everyone (especially in gif form!)

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right):

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:72:
m_lifecycle.incrementPreventThrottlingCount();
On 2015/10/16 21:40:45, esprehn wrote:
> if you're going to put these out of line you don't need the
increment/decrement
> methods, just access the s_preventThrottlingCount directly. DetachScope uses
> methods because it's inline.

Ah, of course. Done.

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutPart.h (right):

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutPart.h:57: bool
isThrottledFrameView();
On 2015/10/16 21:40:45, esprehn wrote:
> const ?

Done.

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/1364063007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:62: // TODO(skyostil): Find
out if these functions need to be run for documents without views.
On 2015/10/16 21:40:45, esprehn wrote:
> They definitely shouldn't run for documents without views, that's a bug.

Right, I wasn't sure if we were sure about that. Updated the text.

[Sami, 2015-10-19 10:51:11]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-10-19 10:51:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1364063007/#ps220001
(title: "Rebased + review comments.")

[commit-bot: I haz the power, 2015-10-19 10:51:32]

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

[commit-bot: I haz the power, 2015-10-19 11:45:53]

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

[commit-bot: I haz the power, 2015-10-19 11:45:54]

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

[Sami, 2015-10-19 14:15:26]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-10-19 14:15:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1364063007/#ps240001
(title: "Fix layout test by not dumping throttled FrameViews.")

[commit-bot: I haz the power, 2015-10-19 14:15:55]

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

[commit-bot: I haz the power, 2015-10-19 15:34:26]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-10-19 15:35:48]

Patchset 13 (id:??) landed as
https://crrev.com/8ab2ba28e9dafe5b45ad914a274197128017221f
Cr-Commit-Position: refs/heads/master@{#354780}

[alogvinov, 2015-10-21 13:02:43]

alogvinov@yandex-team.ru changed reviewers:
+ alogvinov@yandex-team.ru

[alogvinov, 2015-10-21 13:02:44]

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

https://codereview.chromium.org/1364063007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2411:
ASSERT(lifecycle().state() >= DocumentLifecycle::LayoutClean);
Shouldn't this assert be removed or amended now that
updateStyleAndLayoutIfNeededRecursive() may return immediately if
shouldThrottleRendering() is true, leaving lifecycle().state() unchanged?

[Sami, 2015-10-21 13:15:16]

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

https://codereview.chromium.org/1364063007/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2411:
ASSERT(lifecycle().state() >= DocumentLifecycle::LayoutClean);
On 2015/10/21 13:02:44, alogvinov wrote:
> Shouldn't this assert be removed or amended now that
> updateStyleAndLayoutIfNeededRecursive() may return immediately if
> shouldThrottleRendering() is true, leaving lifecycle().state() unchanged?

Right now I think this works since we never throttle the local root, but that
shouldn't really be baked in at this level so I agree that this should be
relaxed. Let me put together a patch.