Add infrastructure for partial layouts

Only do partial layouts when possible.

This patch changes offsetWidth, offsetHeight, offsetLeft, and offsetTop to only
layout a portion of the render tree, up to the target renderer. Partial layout
leaves the root needing layout so a full layout must happen before painting.

An example of the benefit of this patch is wikipedia where the following
pattern is common:
1) page load
2) javascript calls offsetWidth, forcing a full layout
3) javascript injects style into the page, invalidating layout
4) a full layout is performed for painting

With this patch, the offsetWidth call in #2 is significantly improved.

Text autosizing currently removes most of the benefit of this patch, this will
be addressed in a followup patch. Similarly, non-overlay scrollbars reduce
the benefit from this patch.

This is currently implemented behind the PartialLayout
runtime enabled feature flag.

BUG=283623
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157252

[achicu, 2013-08-15 13:48:21]

https://codereview.chromium.org/18601002/diff/9001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/9001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1082: performPostLayoutTasks();
I think that the biggest performance improvement comes from not running
updateCompositingLayersAfterLayout() and performPostLayoutTasks().

For updateCompositingLayersAfterLayout there's already a bug in WebKit:
https://bugs.webkit.org/show_bug.cgi?id=84393 . As per their comments it might
be dangerous to not call this function.

I have a local patch where I renamed all the ::layout() functions to
::internalLayout() and made the RenderObject::layout() a non-virtual method that
does a TRACE_EVENT and then calls internalLayout. It seems like even on
complicated pages the incremental layout is amazingly fast compared to the
amount of time spent in FrameView::layout.

[pdr., 2013-08-15 23:34:08]

On 2013/08/15 13:48:21, achicu wrote:
>
https://codereview.chromium.org/18601002/diff/9001/Source/core/page/FrameView...
> File Source/core/page/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/18601002/diff/9001/Source/core/page/FrameView...
> Source/core/page/FrameView.cpp:1082: performPostLayoutTasks();
> I think that the biggest performance improvement comes from not running
> updateCompositingLayersAfterLayout() and performPostLayoutTasks().
> 
> For updateCompositingLayersAfterLayout there's already a bug in WebKit:
> https://bugs.webkit.org/show_bug.cgi?id=84393 . As per their comments it might
> be dangerous to not call this function.
> 
> I have a local patch where I renamed all the ::layout() functions to
> ::internalLayout() and made the RenderObject::layout() a non-virtual method
that
> does a TRACE_EVENT and then calls internalLayout. It seems like even on
> complicated pages the incremental layout is amazingly fast compared to the
> amount of time spent in FrameView::layout.

Lets find out! https://codereview.chromium.org/22880008

[pdr., 2013-08-18 02:29:21]

On 2013/08/15 23:34:08, pdr wrote:
> On 2013/08/15 13:48:21, achicu wrote:
> >
>
https://codereview.chromium.org/18601002/diff/9001/Source/core/page/FrameView...
> > File Source/core/page/FrameView.cpp (right):
> > 
> >
>
https://codereview.chromium.org/18601002/diff/9001/Source/core/page/FrameView...
> > Source/core/page/FrameView.cpp:1082: performPostLayoutTasks();
> > I think that the biggest performance improvement comes from not running
> > updateCompositingLayersAfterLayout() and performPostLayoutTasks().
> > 
> > For updateCompositingLayersAfterLayout there's already a bug in WebKit:
> > https://bugs.webkit.org/show_bug.cgi?id=84393 . As per their comments it
might
> > be dangerous to not call this function.
> > 
> > I have a local patch where I renamed all the ::layout() functions to
> > ::internalLayout() and made the RenderObject::layout() a non-virtual method
> that
> > does a TRACE_EVENT and then calls internalLayout. It seems like even on
> > complicated pages the incremental layout is amazingly fast compared to the
> > amount of time spent in FrameView::layout.
> 
> Lets find out! https://codereview.chromium.org/22880008

Neither updateCompositingLayersAfterLayout nor performPostLayoutTasks really
show up on any traces. I tried several sites (google.com, wikipedia cats,
facebook) with and without impl-side-painting and these functions were not
significant. As for the safety of not calling performPostLayoutTasks: I think
we're safe in this scenario because we leave the root as needing layout. Before
anyone can paint / composite, a fresh layout must occur.

[pdr., 2013-08-18 02:45:12]

This patch is getting close to needing a real review. We no longer fail any
layouttests, and the performance win on top_25 is 6%.

Any takers for an initial review? I'm happy to walk you through this patch in
person if it would help.

[esprehn, 2013-08-18 03:00:30]

I'll bite! This is very exciting.

[esprehn, 2013-08-18 03:43:15]

Your partial layout method needs to always do a recalc style first, otherwise
that loop up the render tree in your partialLayoutIgnorePendingStylesheet method
is over stale renderers and the render object you passed into the method is
stale too. Instead you should pass a Node* (or Element*), do a recalc style,
then you can read out the renderer(). You probably want to refactor the
recalcstyle stuff out of updateLayoutIgnorePendingStylesheet so you can call it
here, or you can just add an argument to
updateLayoutIgnorePendingStylesheet(Element* partialLayoutStoppingPoint).

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1801: void
Document::tryPartialUpdateLayoutIgnorePendingStylesheets(RenderObject*
stopLayoutAtRenderer)
This should take a Node, not a renderer. You need to do a style recalc just like
in updateLayoutIgnorePendingStylesheet (you probably want to refactor that
method) to make sure the renderer for Node is correct, then you can set it as
the stop renderer.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1806: if (true ||
ScrollbarTheme::theme()->usesOverlayScrollbars()) {
true? You should probably delete this.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1815: } while ((renderer = renderer->parent()));
This is wrong, the style recalc inside updateLayoutIgnorePendingStylesheet can
change all these renderers and then your check here is not correct.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1817:
view()->setStopLayoutAtRenderer(stopLayoutAtRenderer);
You don't call any methods on it, but storing a renderer before a recalc style
is a use after free.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.h:491: void
tryPartialUpdateLayoutIgnorePendingStylesheets(RenderObject*);
Remove the "try" prefix. That only really makes sense when this returns a
boolean.

Lets just do partialLayoutIgnorePendingStylesheets(...)

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:571:
document()->tryPartialUpdateLayoutIgnorePendingStylesheets(renderer());
partialUpdateLayoutIgnorePendingStylesheet(this)

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:990: // Text Autosizing requires two-pass layout
which is incompatible with partial layout. If
This should be a FIXME for text autosizing not to do that.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:993: if (m_frame && m_frame->settings() &&
m_frame->settings()->textAutosizingEnabled()) {
m_frame cannot be null here. I'm pretty sure that'd be catastrophic to do layout
on a frameless document.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1122: void
FrameView::checkPartialLayoutComplete(RenderObject* renderer)
This check should be inline.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1129: } while ((renderer = renderer->parent()));
Why do you need to check this twice? Doing it either before or after should be
enough?

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:341: void resetPartialLayoutState() {
m_stopLayoutAtRenderer = 0; m_shouldStopPartialLayout = false; }
Isn't shouldStopPartialLayout the same as checking if stopLayoutAtRenderer is
not null? Why do you need a boolean?

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2530:
document()->view()->checkPartialLayoutComplete(child);
you should add frameView() to RenderObject if you want to do this.

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:752: if (frame()) {
Add a frameView(), also I don't know how this can be null. Frameless documents
never have renderers.

If it can be null this should be an early return.

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Sub...
File Source/core/rendering/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Sub...
Source/core/rendering/SubtreeLayoutScope.cpp:47: if
(!m_root->document()->view()->shouldStopPartialLayout()) {
This should be an early return.

[pdr., 2013-08-20 06:19:10]

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1801: void
Document::tryPartialUpdateLayoutIgnorePendingStylesheets(RenderObject*
stopLayoutAtRenderer)
On 2013/08/18 03:43:15, esprehn wrote:
> This should take a Node, not a renderer. You need to do a style recalc just
like
> in updateLayoutIgnorePendingStylesheet (you probably want to refactor that
> method) to make sure the renderer for Node is correct, then you can set it as
> the stop renderer.

Great catch!

I refactored the common style recalc as
"recalcStyleForLayoutIgnoringPendingStylesheets()".

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1806: if (true ||
ScrollbarTheme::theme()->usesOverlayScrollbars()) {
On 2013/08/18 03:43:15, esprehn wrote:
> true? You should probably delete this.

I'd like to leave this in for a bit longer since it makes performance testing
possible on Linux. Android and OSX have overlay scrollbars which don't take up
space (or CSS causes them to always take up space). Linux unfortunately has
scrollbars that appear if there's enough content which will force a second
layout, which breaks partial layout. I've updated the comment to be explicit
about why I've left it in, and I'll remove this before landing. The layouttests
will keep me honest here ;)

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1815: } while ((renderer = renderer->parent()));
On 2013/08/18 03:43:15, esprehn wrote:
> This is wrong, the style recalc inside updateLayoutIgnorePendingStylesheet can
> change all these renderers and then your check here is not correct.

Done.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1817:
view()->setStopLayoutAtRenderer(stopLayoutAtRenderer);
On 2013/08/18 03:43:15, esprehn wrote:
> You don't call any methods on it, but storing a renderer before a recalc style
> is a use after free.

No use so... just "after free"? :)

Fixed.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Document....
Source/core/dom/Document.h:491: void
tryPartialUpdateLayoutIgnorePendingStylesheets(RenderObject*);
On 2013/08/18 03:43:15, esprehn wrote:
> Remove the "try" prefix. That only really makes sense when this returns a
> boolean.
> 
> Lets just do partialLayoutIgnorePendingStylesheets(...)

Done.

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:571:
document()->tryPartialUpdateLayoutIgnorePendingStylesheets(renderer());
On 2013/08/18 03:43:15, esprehn wrote:
> partialUpdateLayoutIgnorePendingStylesheet(this)

Done.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:990: // Text Autosizing requires two-pass layout
which is incompatible with partial layout. If
On 2013/08/18 03:43:15, esprehn wrote:
> This should be a FIXME for text autosizing not to do that.

Added fixme and a reference to crbug.com/256657.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:993: if (m_frame && m_frame->settings() &&
m_frame->settings()->textAutosizingEnabled()) {
On 2013/08/18 03:43:15, esprehn wrote:
> m_frame cannot be null here. I'm pretty sure that'd be catastrophic to do
layout
> on a frameless document.

Done.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1122: void
FrameView::checkPartialLayoutComplete(RenderObject* renderer)
On 2013/08/18 03:43:15, esprehn wrote:
> This check should be inline.

Done.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1129: } while ((renderer = renderer->parent()));
On 2013/08/18 03:43:15, esprehn wrote:
> Why do you need to check this twice? Doing it either before or after should be
> enough?

The supportsPartialLayout loop was intended to just be an assert. I've cleaned
this up like so:
1) I've inlined checkPartialLayoutComplete and changed it to return a bool if
layout is complete.
2) I've moved the assertion loop behind an #ifndef NDEBUG check and into a
private function "checkPartialLayoutAllowed". The purpose of
checkPartialLayoutAllowed is just to assert that everything is working properly.

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:341: void resetPartialLayoutState() {
m_stopLayoutAtRenderer = 0; m_shouldStopPartialLayout = false; }
On 2013/08/18 03:43:15, esprehn wrote:
> Isn't shouldStopPartialLayout the same as checking if stopLayoutAtRenderer is
> not null? Why do you need a boolean?

We need the boolean to track that we're exiting in the middle of layout.

m_stopLayoutAtRenderer is used to determine which renderer we need to stop
layout on.
m_shouldStopPartialLayout is used to say 'STOP' to all the layout methods.

In a full, complete layout, m_stopLayoutAtRenderer will be 0 and
m_shouldStopPartialLayout will be false.
In a partial layout, before we've laid out the target renderer,
m_shouldStopPartialLayout will be false.
In a partial layout, after we've laid out the target renderer,
m_shouldStopPartialLayout will be true.

Happy to update these names if we can make this more understandable. One idea is
to move this state into a small struct or class such as PartialLayoutState to
hide some of these details. We could also hide some kind of
temporarilyDisablePartialLayout functionality in a class for working around
two-pass layouts.

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2530:
document()->view()->checkPartialLayoutComplete(child);
On 2013/08/18 03:43:15, esprehn wrote:
> you should add frameView() to RenderObject if you want to do this.

Done.

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:752: if (frame()) {
On 2013/08/18 03:43:15, esprehn wrote:
> Add a frameView(), also I don't know how this can be null. Frameless documents
> never have renderers.
> 
> If it can be null this should be an early return.

Done.

I changed this to not even check for null so we'll null deref crash in debug if
our assumption is wrong.

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Sub...
File Source/core/rendering/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/18601002/diff/23001/Source/core/rendering/Sub...
Source/core/rendering/SubtreeLayoutScope.cpp:47: if
(!m_root->document()->view()->shouldStopPartialLayout()) {
On 2013/08/18 03:43:15, esprehn wrote:
> This should be an early return.

Done.

[eseidel, 2013-08-20 21:00:39]

https://codereview.chromium.org/18601002/diff/15001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/18601002/diff/15001/Source/core/dom/Document....
Source/core/dom/Document.h:491: void
tryPartialUpdateLayoutIgnorePendingStylesheets(RenderObject*);
We really need to create some new object to hold all the rendering-tree public
APIs.  Obviously a separate concern from this patch..

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:996: if (m_frame->settings() &&
m_frame->settings()->textAutosizingEnabled()) {
You don't need all this if you add a "m_disablePartialLayoutCount" and just
increment it for the first layout when textAutosigingEnabled is true.

// We need to disable partial layouts for the first layout if we might
// end up doing two-pass layout for autosizing.
PartialLayoutDisabler disabler(m_frame->settings()->textAutosizingEnabled());

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1040: resetPartialLayoutState();
Shouldn't the original caller drive this?

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:554: m_shouldStopPartialLayout = true;
I think when you do this, you should also clear m_stopLayoutAtRenderer.  We've
transitioned from the phase of "should we stop" to "we're stopping" and then
when we hit "stopped" we'll even clear the "we're stopping" bool.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1604: statePusher.pop();
gnarly.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2531: if
(frameView()->checkPartialLayoutComplete(child))
You should add a comment as to why.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2787: FrameView*
RenderObject::frameView() const
I think we should explain why this should never be null for RenderObject and
ASSERT it.  I would do this in a separate patch.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderView.h:74: FrameView* frameView() const OVERRIDE {
return m_frameView; }
virtual?

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Sub...
File Source/core/rendering/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Sub...
Source/core/rendering/SubtreeLayoutScope.cpp:47: if
(m_root->document()->view()->shouldStopPartialLayout())
You might leave a comment here explaining why.

[eseidel, 2013-08-20 21:08:53]

As I said to pdr in person:

This work is fantastic.

I would encourage him to move as quickly as possible to a land-able state so we
can close this (now month old?) review and iterate on trunk.

He's proven the concept.  It's time to get it in the engine so we can polish the
edges.

[dglazkov, 2013-08-23 16:27:15]

Sorry, just now got to look at it. This is great stuff, pdr!

[esprehn, 2013-08-23 20:47:26]

Don't forget that always true if statement when you submit! :P

https://codereview.chromium.org/18601002/diff/33001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1797: bool oldIgnore = m_ignorePendingStylesheets;
You should switch this to a TemporaryChange<bool>

https://codereview.chromium.org/18601002/diff/33001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1805: bool oldIgnore = m_ignorePendingStylesheets;
Ditto

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1129: while (renderer) {
This could be a for() loop :)

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1604: statePusher.pop();
It's really weird that LayoutStateMaintainer doesn't pop() in the destructor, it
already has booleans to know if it should.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2598: if
(document()->view()->shouldStopPartialLayout())
frameView()

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:497: // FIXME: This should be pure virtual
with an implementation in the header.
What does that mean? pure virtual is = 0 and then how do you have an
implementation?

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2789: // FIXME: Should we check
isRooted()?
There's no reason to.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2790: if (RenderView* renderView =
view())
Don't do this, just do return document()->view();

What you have is actually:

m_node->document()->renderView()->frameView()

instead you want:

m_node->document()->view()

which is what document()->view() does.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:593: virtual FrameView* frameView() const;
frameView() should not be virtual

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderView.h:74: FrameView* frameView() const OVERRIDE {
return m_frameView; }
This should not be virtual, just do document()->view() in RenderObject. Virtual
call overhead doesn't make sense. It's fine if RenderView shadows the impl on
RenderObject with a faster version.

(In truth we should probably eliminate this m_frameView thing and just use the
RenderObject frameView() that does document()->view().

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderWidget.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderWidget.h:77: FrameView* frameView() const OVERRIDE {
return m_frameView; }
Same deal, lets just use the one on RenderObject. Also these are missing the
virtual keyword.

[pdr., 2013-08-26 05:50:39]

Thank you for the excellent reviews! This is getting closer.

I'd like feedback on the new class I've added in the latest patch:
"PartialLayoutState.h".

I need to make sure tests pass on Android tomorrow and re-run the performance
numbers. Assuming that goes well, I'll move this feature behind a flag and
upload another patch for (final?) reviews.

https://codereview.chromium.org/18601002/diff/33001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1797: bool oldIgnore = m_ignorePendingStylesheets;
On 2013/08/23 20:47:27, esprehn wrote:
> You should switch this to a TemporaryChange<bool>

Done

https://codereview.chromium.org/18601002/diff/33001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1805: bool oldIgnore = m_ignorePendingStylesheets;
On 2013/08/23 20:47:27, esprehn wrote:
> Ditto

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:996: if (m_frame->settings() &&
m_frame->settings()->textAutosizingEnabled()) {
On 2013/08/20 21:00:39, eseidel - vacation until 9.3 wrote:
> You don't need all this if you add a "m_disablePartialLayoutCount" and just
> increment it for the first layout when textAutosigingEnabled is true.
> 
> // We need to disable partial layouts for the first layout if we might
> // end up doing two-pass layout for autosizing.
> PartialLayoutDisabler disabler(m_frame->settings()->textAutosizingEnabled());

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1040: resetPartialLayoutState();
On 2013/08/20 21:00:39, eseidel - vacation until 9.3 wrote:
> Shouldn't the original caller drive this?

Fixing this was enlightening. resetPartialLayoutState() was masking a bug.

We disable overlay scrollbars when running tests (for cross-platform
consistency). Because non-overlay scrollbars cause a second layout through
adjustViewSize(), I've added a PartialLayoutDisabler for the primary layout. In
the common case, the first ("primary?") layout will early-out if scrollbars are
present, so we should still have a nice speed win.

I've also changed this so the original caller
(partialUpdateLayoutIgnorePendingStylesheets) is now the one to call reset().

I've added two tests to prove this works.

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1129: while (renderer) {
On 2013/08/23 20:47:27, esprehn wrote:
> This could be a for() loop :)

This function was useful for checking my logic in writing this patch, but it's
no longer all that useful (it's redundant since we do this check in
Document::partialUpdateLayoutIgnorePendingStylesheets). I've removed this for
now.

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:554: m_shouldStopPartialLayout = true;
On 2013/08/20 21:00:39, eseidel - vacation until 9.3 wrote:
> I think when you do this, you should also clear m_stopLayoutAtRenderer.  We've
> transitioned from the phase of "should we stop" to "we're stopping" and then
> when we hit "stopped" we'll even clear the "we're stopping" bool.

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2531: if
(frameView()->checkPartialLayoutComplete(child))
On 2013/08/20 21:00:39, eseidel - vacation until 9.3 wrote:
> You should add a comment as to why.

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2598: if
(document()->view()->shouldStopPartialLayout())
On 2013/08/23 20:47:27, esprehn wrote:
> frameView()

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:497: // FIXME: This should be pure virtual
with an implementation in the header.
On 2013/08/23 20:47:27, esprehn wrote:
> What does that mean? pure virtual is = 0 and then how do you have an
> implementation?

You can have a pure virtual with an implementation but it's not easy to do in
this case due to how we create RenderBlocks. I've just removed this comment for
now.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2789: // FIXME: Should we check
isRooted()?
On 2013/08/23 20:47:27, esprehn wrote:
> There's no reason to.

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2790: if (RenderView* renderView =
view())
On 2013/08/23 20:47:27, esprehn wrote:
> Don't do this, just do return document()->view();
> 
> What you have is actually:
> 
> m_node->document()->renderView()->frameView()
> 
> instead you want:
> 
> m_node->document()->view()
> 
> which is what document()->view() does.

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:593: virtual FrameView* frameView() const;
On 2013/08/23 20:47:27, esprehn wrote:
> frameView() should not be virtual

Done.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderView.h:74: FrameView* frameView() const OVERRIDE {
return m_frameView; }
On 2013/08/23 20:47:27, esprehn wrote:
> This should not be virtual, just do document()->view() in RenderObject.
Virtual
> call overhead doesn't make sense. It's fine if RenderView shadows the impl on
> RenderObject with a faster version.
> 
> (In truth we should probably eliminate this m_frameView thing and just use the
> RenderObject frameView() that does document()->view().

Thanks for this explanation. Fixed

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderWidget.h (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderWidget.h:77: FrameView* frameView() const OVERRIDE {
return m_frameView; }
On 2013/08/23 20:47:27, esprehn wrote:
> Same deal, lets just use the one on RenderObject. Also these are missing the
> virtual keyword.

We can't remove this function in favor of RenderObject's because the widget tree
can outlast the document, causing a crash when using document().
fast/block/child-not-removed-from-parent-lineboxes-crash.html tests this. I've
removed the OVERRIDE that was incorrectly put here.

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Sub...
File Source/core/rendering/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/18601002/diff/33001/Source/core/rendering/Sub...
Source/core/rendering/SubtreeLayoutScope.cpp:47: if
(m_root->document()->view()->shouldStopPartialLayout())
On 2013/08/20 21:00:39, eseidel - vacation until 9.3 wrote:
> You might leave a comment here explaining why.

Done, and added an assert that the root does need a layout.

[ojan, 2013-08-27 01:33:41]

No real substantive feedback from me...

https://codereview.chromium.org/18601002/diff/50001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/50001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1781: } else if (m_hasNodesWithPlaceholderStyle)
Nit: moar curly braces plz.

https://codereview.chromium.org/18601002/diff/50001/Source/core/rendering/Sub...
File Source/core/rendering/SubtreeLayoutScope.cpp (right):

https://codereview.chromium.org/18601002/diff/50001/Source/core/rendering/Sub...
Source/core/rendering/SubtreeLayoutScope.cpp:47: // Partial layout early-exits
layotu and will leave the tree as needing layout.
typo: layotu

https://codereview.chromium.org/18601002/diff/50001/Source/core/rendering/Sub...
Source/core/rendering/SubtreeLayoutScope.cpp:56: for
(HashSet<RenderObject*>::iterator it = m_renderersToLayout.begin(); it !=
m_renderersToLayout.end(); ++it) {
I think we can still do this loop in the partial layout case. Not sure.

[pdr., 2013-09-02 04:08:17]

OK gang, this has taken long enough :) The latest patch puts this feature behind
a runtime enabled feature flag so we can start iterating live and get some
performance numbers.

I'd like to land this Tuesday. PTAL

[esprehn, 2013-09-03 18:15:35]

Your flag doesn't actually disable this feature since you don't early return.
You also need to clear the partial layout state inside FrameView::layout
otherwise JS code that runs in postLayout will get the stale state.

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1798: TemporaryChange<bool>
ignorePendingStylesheets(m_ignorePendingStylesheets,
m_ignorePendingStylesheets);
This TemporaryChange should be inside
recalcStyleForLayoutIgnoringPendingStylesheets() otherwise that method doesn't
ignore pending sheets (unless a caller sets up this magic var) :P

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1806: updateLayoutIgnorePendingStylesheets();
You need to early return, otherwise your code still runs below.

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1827: view()->partialLayout().reset();
I think this needs to be inside FrameView::layout() as a postLayout task. In
particular layout can cause JS to run which can then renter layout() with a
stale partialLayout state now.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
File Source/core/rendering/PartialLayoutState.h (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:51: RenderObject* stopAtRenderer() {
return m_disableCount > 0 ? 0 : m_stopAtRenderer; }
Is there a reason stopAtRenderer() needs to be public? I fear future
use-after-frees.

[eseidel, 2013-09-03 18:16:08]

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
File LayoutTests/fast/dom/partial-layout-block-expected.txt (right):

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
LayoutTests/fast/dom/partial-layout-block-expected.txt:9: WARN: shouldBe()
expects string arguments
Looks wrong.

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
File LayoutTests/fast/dom/partial-layout-block.html (right):

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
LayoutTests/fast/dom/partial-layout-block.html:44: shouldBe(measureWidth,
"document.body.offsetWidth - 20");
This looks wrong.  "measureWidth" is what you want.

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:899: rootForThisLayout->layout(); // THIS IS
WHERE LAYOUT ACTUALLY HAPPENS.
Do you mean to keep this comment?

[eseidel, 2013-09-03 19:14:26]

The idea is definitely lgtm.  We really need to get this in and iterate.

Left you a few more comments.

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:183: , m_partialLayout(PartialLayoutState())
This isn't needed.

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:284: m_partialLayout.reset();
Or m_partialLayout = PartialLayoutState().  I suspect both work.

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:899: rootForThisLayout->layout(); // THIS IS
WHERE LAYOUT ACTUALLY HAPPENS.
On 2013/09/03 18:16:08, eseidel wrote:
> Do you mean to keep this comment?

nm.  I now see it predates your patch.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
File Source/core/rendering/PartialLayoutState.h (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:48: bool shouldStop() const { return
m_shouldStop; }
I might have called this isStopping()?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:49: bool
checkPartialLayoutComplete(RenderObject*);
const?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:50: void
setStopAtRenderer(RenderObject* renderer) { m_stopAtRenderer = renderer; }
const?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:51: RenderObject* stopAtRenderer() {
return m_disableCount > 0 ? 0 : m_stopAtRenderer; }
const?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:52: void reset() { m_shouldStop =
false; m_stopAtRenderer = 0; }
Yeah, I would probably have added assignment instead of bothered with a reset()
method, that way you can depend on the constructor to init everything for you.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:55: void disable() {
m_disableCount++; }
ASSERT(!m_shouldStop);?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:58: bool m_shouldStop;
This tracks "did we reach the stopping renderer yet", correct?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:65: if (m_shouldStop)
Is it OK for this method to be called after shouldStop is set?  Once we enter
the "stopping" phase, it should be an error for any layout to continue, no?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:70: m_stopAtRenderer = 0;
You might add a note as to why you clear the m_stopAtRenderer member here.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:80:
PartialLayoutDisabler(PartialLayoutState& partialLayout, bool disable)
should disable default to true?

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1603: statePusher.pop();
Is this explicit pop neeeded?  Surprised this isnt' handled by a scoping object.

[pdr., 2013-09-03 21:37:35]

Thanks for the comments. I need to figure out how to get linux and win bots to
pass tests now (they will differ due to having scrollbars).

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
File LayoutTests/fast/dom/partial-layout-block-expected.txt (right):

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
LayoutTests/fast/dom/partial-layout-block-expected.txt:9: WARN: shouldBe()
expects string arguments
On 2013/09/03 18:16:08, eseidel wrote:
> Looks wrong.

Fixed, I moved all of these off the test-pre/test-post harness and onto
testharness.js. The former requires strings :/

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
File LayoutTests/fast/dom/partial-layout-block.html (right):

https://codereview.chromium.org/18601002/diff/59001/LayoutTests/fast/dom/part...
LayoutTests/fast/dom/partial-layout-block.html:44: shouldBe(measureWidth,
"document.body.offsetWidth - 20");
On 2013/09/03 18:16:08, eseidel wrote:
> This looks wrong.  "measureWidth" is what you want.

I don't follow.

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1798: TemporaryChange<bool>
ignorePendingStylesheets(m_ignorePendingStylesheets,
m_ignorePendingStylesheets);
On 2013/09/03 18:15:36, esprehn wrote:
> This TemporaryChange should be inside
> recalcStyleForLayoutIgnoringPendingStylesheets() otherwise that method doesn't
> ignore pending sheets (unless a caller sets up this magic var) :P

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1806: updateLayoutIgnorePendingStylesheets();
On 2013/09/03 18:15:36, esprehn wrote:
> You need to early return, otherwise your code still runs below.

:(

Stupid mistake on my part; good catch.

https://codereview.chromium.org/18601002/diff/59001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1827: view()->partialLayout().reset();
On 2013/09/03 18:15:36, esprehn wrote:
> I think this needs to be inside FrameView::layout() as a postLayout task. In
> particular layout can cause JS to run which can then renter layout() with a
> stale partialLayout state now.

I don't think we need to run the post layout tasks at all. Is that not the case?

JS cannot run in the middle of layout so I think we're good wrt reentrant
layouts.

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:183: , m_partialLayout(PartialLayoutState())
On 2013/09/03 19:14:27, eseidel wrote:
> This isn't needed.

how do i c++?

Fixed.

https://codereview.chromium.org/18601002/diff/59001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:899: rootForThisLayout->layout(); // THIS IS
WHERE LAYOUT ACTUALLY HAPPENS.
On 2013/09/03 19:14:27, eseidel wrote:
> On 2013/09/03 18:16:08, eseidel wrote:
> > Do you mean to keep this comment?
> 
> nm.  I now see it predates your patch.

While you were on vacation I complained about this comment :) I'll save the rant
for another patch, but this comment is more confusing than helpful.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
File Source/core/rendering/PartialLayoutState.h (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:48: bool shouldStop() const { return
m_shouldStop; }
On 2013/09/03 19:14:27, eseidel wrote:
> I might have called this isStopping()?

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:49: bool
checkPartialLayoutComplete(RenderObject*);
On 2013/09/03 19:14:27, eseidel wrote:
> const?

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:50: void
setStopAtRenderer(RenderObject* renderer) { m_stopAtRenderer = renderer; }
On 2013/09/03 19:14:27, eseidel wrote:
> const?

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:51: RenderObject* stopAtRenderer() {
return m_disableCount > 0 ? 0 : m_stopAtRenderer; }
On 2013/09/03 18:15:36, esprehn wrote:
> Is there a reason stopAtRenderer() needs to be public? I fear future
> use-after-frees.

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:52: void reset() { m_shouldStop =
false; m_stopAtRenderer = 0; }
On 2013/09/03 19:14:27, eseidel wrote:
> Yeah, I would probably have added assignment instead of bothered with a
reset()
> method, that way you can depend on the constructor to init everything for you.

Mind if we save this for a followup?

This may turn into a state pop in a followup patch.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:55: void disable() {
m_disableCount++; }
On 2013/09/03 19:14:27, eseidel wrote:
> ASSERT(!m_shouldStop);?

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:58: bool m_shouldStop;
EOn 2013/09/03 19:14:27, eseidel wrote:
> This tracks "did we reach the stopping renderer yet", correct?

Exactly

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:65: if (m_shouldStop)
On 2013/09/03 19:14:27, eseidel wrote:
> Is it OK for this method to be called after shouldStop is set?  Once we enter
> the "stopping" phase, it should be an error for any layout to continue, no?

This is a good question.

This function is combining two operations, since you always do them both:
1) checking if layout should stop
2) checking if this is the layout that should stop

Otherwise, all the callers would need to do:
if (partialLayout()->isStopping())
    return;
if (checkPartialLayoutComplete(...))
    return

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Par...
Source/core/rendering/PartialLayoutState.h:80:
PartialLayoutDisabler(PartialLayoutState& partialLayout, bool disable)
On 2013/09/03 19:14:27, eseidel wrote:
> should disable default to true?

Done.

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/18601002/diff/59001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1603: statePusher.pop();
On 2013/09/03 19:14:27, eseidel wrote:
> Is this explicit pop neeeded?  Surprised this isnt' handled by a scoping
object.

Great question. Unfortunately, it is needed. It's also explicitly called in
relayoutForPagination. We should clean this up, but that's for a followup.

[esprehn, 2013-09-04 00:40:25]

LGTM, Shadowfax show us the meaning of partial layout!

[pdr., 2013-09-04 06:33:47]

On 2013/09/04 00:40:25, esprehn wrote:
> LGTM, Shadowfax show us the meaning of partial layout!

On the train home I thought about how the scrollbar code works and realized it
was not correct after all.

@Esprehn, Do you mind one more once-over of the latest patch, especially
FrameView.cpp? Instead of disabling the first of 2 layouts I changed it to not
do the second layout, nor any post layouts that may occur. The new
isPartialLayout checks roughly follow the existing inSubtreeLayout checks.

[esprehn, 2013-09-04 07:10:12]

one nit, otherwise lgtm. Lets get this in and start iterating.

https://codereview.chromium.org/18601002/diff/86001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/18601002/diff/86001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:589: FrameView* frameView() const;
This should be inline in the header here just like the view() method.

[commit-bot: I haz the power, 2013-09-04 20:26:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/18601002/59002

[commit-bot: I haz the power, 2013-09-05 01:06:26]

Change committed as 157252