Add RenderObject::needsResizeLayout

Add RenderObject::needsResizeLayout

Originally we use setNeedsLayout to schedule layout for resizes, which
causes full layouts that may be unnecessary in many cases.

Add RenderObject::needsResizeLayout to schedule such layouts. Handle
such layout in RenderBlock::simplifiedLayout() and fall back to full
layout if simplified layout is not feasible.

For now this flag is for RenderView only to optimize RenderView resize.
Generic resize layout needs more verifications.

BUG=258219, 310875
TEST=none because the flag has not been used yet. Will test in followup
     changes.

[Julien - ping for review, 2014-03-21 19:39:10]

FWIW it seems to me like what we would like is something like a recompute-size
phase instead of a new layout mode. That's because adding new layout modes won't
scale very well and we will see duplication soon (we are already seeing
confusion about the different phases and what they are supposed to do).

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:538: return
tryLayoutDoingPositionedMovementOnly();
I don't think this is fundamentally right to do a positioned movement only.
RenderView is artificially positioned (it's more of an implementation detail)
but it doesn't *move*, it resizes and that's different. Also this won't handle
the following cases:
- vh units.
- positioned objects with a percentage bottom / top.

[Xianzhu, 2014-03-21 20:10:36]

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:538: return
tryLayoutDoingPositionedMovementOnly();
On 2014/03/21 19:39:10, Julien Chaffraix - PST wrote:
> I don't think this is fundamentally right to do a positioned movement only.

Actually the intent is not to do a position movement. Calling it just because
the things to do happen to be the same: update logical width and height; if
logical width is different, returns false.

To avoid confusion, I can also repeat the code here.

> RenderView is artificially positioned (it's more of an implementation detail)
> but it doesn't *move*, it resizes and that's different. Also this won't handle
> the following cases:
> - vh units.
> - positioned objects with a percentage bottom / top.

In these cases, before RenderView's tryLayoutDoingResizeOnly() is called, the
objects have been setNeedsLayout() (which also marked the ancestors
setNeedsLayout()). This will cause a normal full layout (after early returning
from trySimplifiedLayout), despite the setNeedsSizeLayout() flag.

The unit test in https://codereview.chromium.org/204813002/ covers these cases.

This is why we need setNeedsResizeLayout(): otherwise during layout we can't
know if the object needs a simple resize, or a full layout triggered by its
descendants.

[esprehn, 2014-03-21 20:15:39]

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1351: setNeedsResizeLayout(true);
Please don't add this overload like this.

[Xianzhu, 2014-03-21 20:37:14]

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/197283038/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1351: setNeedsResizeLayout(true);
On 2014/03/21 20:15:39, esprehn wrote:
> Please don't add this overload like this.

Done.

(Note: naming of setNeedsResizeLayoutInternal(bool) is now inconsistent with
other existing setNeedsXXXLayout(bool) functions. Perhaps we can make them
consistent in another change.)

[esprehn, 2014-03-24 22:40:04]

The methods should be on RenderView, there's no reason to add them to
RenderObject and then just ASSERT you're a RenderView.

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3822: if (node()->isShadowRoot() &&
toShadowRoot(node())->host()->hasTagName(inputTag))
Why did you change this?

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:533: bool tryLayoutDoingResizeOnly()
This should not be inline in the header. Move it to the cpp file.

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:552: && !m_bitfields.posChildNeedsLayout()
&& !m_bitfields.needsSimplifiedNormalFlowLayout() &&
!m_bitfields.needsPositionedMovementLayout();
Can you put one check per line?

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1345: inline void
RenderObject::setNeedsResizeLayout()
This doesn't need to be inline.

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1349: ASSERT(isRenderView());
These methods should be on RenderView.

[Xianzhu, 2014-03-24 23:53:23]

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:3822: if (node()->isShadowRoot() &&
toShadowRoot(node())->host()->hasTagName(inputTag))
On 2014/03/24 22:40:04, esprehn wrote:
> Why did you change this?

Sorry for the carelessness. May be resulted from a bad merge. Reverted.

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:533: bool tryLayoutDoingResizeOnly()
On 2014/03/24 22:40:04, esprehn wrote:
> This should not be inline in the header. Move it to the cpp file.

Done.

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:552: && !m_bitfields.posChildNeedsLayout()
&& !m_bitfields.needsSimplifiedNormalFlowLayout() &&
!m_bitfields.needsPositionedMovementLayout();
On 2014/03/24 22:40:04, esprehn wrote:
> Can you put one check per line?

Done.

https://codereview.chromium.org/197283038/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1349: ASSERT(isRenderView());
On 2014/03/24 22:40:04, esprehn wrote:
> These methods should be on RenderView.

Moved this method to RenderView, but kept others to keep m_bitfields private.
May move this method back soon when applying it on generic RenderObjects.

[Xianzhu, 2014-03-26 15:31:50]

PTAL at Patch Set 6. Thanks.

Notes:
- Still keep setNeedsResizeLayout() functions in RenderObject.h. Removed
ASSERT(isRenderView()) because the function will be used to optimize resize
layout for generic objects, though its first use is for RenderView.
- Still keep RenderObject::setNeedsResizeLayoutBit(bool) inlined to keep
consistency with existing similar methods. Filed crbug.com/356675 to discuss the
existing inline issue.

[Xianzhu, 2014-03-26 15:33:30]

On 2014/03/26 15:31:50, Xianzhu wrote:
> PTAL at Patch Set 6. Thanks.
> 
> Notes:
> - Still keep setNeedsResizeLayout() functions in RenderObject.h. Removed
> ASSERT(isRenderView()) because the function will be used to optimize resize
> layout for generic objects, though its first use is for RenderView.
> - Still keep RenderObject::setNeedsResizeLayoutBit(bool) inlined to keep
> consistency with existing similar methods. Filed crbug.com/356675 to discuss
the
> existing inline issue.
s/RenderObject::setNeedsResizeLayoutBit(bool)/RenderObject::setNeedsResizeLayout()/

[eseidel, 2014-04-01 18:25:19]

Resizing RenderObjects is exactly what "full layout" is supposed to do.  I'm
surprised we need to add a fast-path here?  Could you help me understand what
other costly side-effects of a "full layout" there are that you're trying to
avoid?  I guess that full layout always causes a repaint (which it ideally
shouldn't)?

https://codereview.chromium.org/197283038/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/197283038/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1176:
ADD_BOOLEAN_BITFIELD(needsResizeLayout, NeedsResizeLayout);
Presumably we have space for this otherwise the KeepRenderObjectSmall
COMPILE_ASSERT would fail?

[Xianzhu, 2014-04-01 18:43:41]

On 2014/04/01 18:25:19, eseidel wrote:
> Resizing RenderObjects is exactly what "full layout" is supposed to do.  I'm
> surprised we need to add a fast-path here?  Could you help me understand what
> other costly side-effects of a "full layout" there are that you're trying to
> avoid?  I guess that full layout always causes a repaint (which it ideally
> shouldn't)?
> 

Thought of the fast-path based on the existing fast-path:
RenderBlock::simpifiedLayout().

I think we need some profiling data to see if there is real reason of them.
Perhaps we could remove the existing fast-paths if profiling proved they were
not worth it :) Before that I agree that optimizing repaint should be of higher
priority.

[eseidel, 2014-04-01 18:49:48]

I don't know the history of RenderBlock::simpifiedLayout well enough to justify
it's existence:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The general trend in WebKit was towards fast-paths to side-step all the crazy
that is the general case on the web.  The general trend in Blink has been away
from fast-paths (they add a lot of complexity to the engine and "falling off a
cliff" behavior for authors) and just making the common paths faster.  But each
approach has its place.