RenderBlock::isSelfCollapsingBlock() should only be used when an object does not require layout.

RenderBlock::isSelfCollapsingBlock() should only be used when an object does not require layout.

|isSelfCollapsingBlock| should only be called on an object after it has had layout. In order to enforce this with an ASSERT I had to account for a couple of things I hadn't previously considered:

- In a couple of places we look at the previous child sibling and check whether it's selfCollapsing. When we do that we need to be sure that it's a normal flow block we're looking at.

- Placeholder elements are not laid out until the dimensions of their parent text control are known, so they don't get layout until their parent has had layout - this is unique in the layout tree and unfortunately when we call isSelfCollapsingBlock on them we find that they still need layout. Since placeholder elements only exist when they have content we can enforce the rule that they are never self-collapsing.

- When we are in partial layout and have entered layoutBlockChildren to layout one or more children we should return early when we are done. The current behaviour, which ignores the rest of the children and then adjusts the height of the parent block with the current margin information, is unnecessary and likely to be wrong.


BUG=

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

[ojan, 2014-01-14 00:34:18]

pdr, can you look at the partialLayout change?

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1012: RenderBlockFlow*
previousBlockFlow =  prev && prev->isRenderBlockFlow() &&
!prev->isFloatingOrOutOfFlowPositioned() ? toRenderBlockFlow(prev) : 0;
Doesn't this change behavior? Was it always wrong to try to collapse margins
with self collapsing floats/out-of-flow-positioneds?

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderTextControlPlaceholder.h (right):

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderTextControlPlaceholder.h:32: virtual bool
isSelfCollapsingBlock() const OVERRIDE FINAL { return false; }
I think this deserve a comment explaining why we override it.

Also, can you point me at the code that you reference in the change description
that does the parent's layout before the child?

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderTextControlPlaceholder.h:35:
DEFINE_RENDER_OBJECT_TYPE_CASTS(RenderTextControlPlaceholder, isTableCaption());
isTableCaption?

[pdr., 2014-01-14 00:50:29]

On 2014/01/14 00:34:18, ojan wrote:
> pdr, can you look at the partialLayout change?

The partial layout change looks correct to me. LGTM for that part.

[rhogan, 2014-01-14 19:23:01]

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderBlockFlow.cpp (right):

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderBlockFlow.cpp:1012: RenderBlockFlow*
previousBlockFlow =  prev && prev->isRenderBlockFlow() &&
!prev->isFloatingOrOutOfFlowPositioned() ? toRenderBlockFlow(prev) : 0;
On 2014/01/14 00:34:18, ojan wrote:
> Doesn't this change behavior? Was it always wrong to try to collapse margins
> with self collapsing floats/out-of-flow-positioneds?

Yes, it was incorrect not to skip past floats/outofflows as they do not
participate in margin-collapsing.

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderTextControlPlaceholder.h (right):

https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderTextControlPlaceholder.h:32: virtual bool
isSelfCollapsingBlock() const OVERRIDE FINAL { return false; }
On 2014/01/14 00:34:18, ojan wrote:
> I think this deserve a comment explaining why we override it.
> 
> Also, can you point me at the code that you reference in the change
description
> that does the parent's layout before the child?

Yes: the placeholder is not laid out until the end of 
RenderTextControlSingleLine::layout().

[rhogan, 2014-01-14 19:29:15]

On 2014/01/14 19:23:01, rhogan wrote:
>
https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
> File Source/core/rendering/RenderBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/129873004/diff/200001/Source/core/rendering/R...
> Source/core/rendering/RenderBlockFlow.cpp:1012: RenderBlockFlow*
> previousBlockFlow =  prev && prev->isRenderBlockFlow() &&
> !prev->isFloatingOrOutOfFlowPositioned() ? toRenderBlockFlow(prev) : 0;
> On 2014/01/14 00:34:18, ojan wrote:
> > Doesn't this change behavior? Was it always wrong to try to collapse margins
> > with self collapsing floats/out-of-flow-positioneds?
> 
> Yes, it was incorrect not to skip past floats/outofflows as they do not
> participate in margin-collapsing.

s/skip/ignore/

[ojan, 2014-01-14 21:58:14]

lgtm

[commit-bot: I haz the power, 2014-01-14 22:00:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robhogan@gmail.com/129873004/380001

[commit-bot: I haz the power, 2014-01-14 23:48:40]

Change committed as 165099

[ojan, 2014-01-28 23:59:47]

https://codereview.chromium.org/129873004/diff/380001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/129873004/diff/380001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:165: if (element->shadowPseudoId() ==
"-webkit-input-placeholder")
esprehn nagged me that we really shouldn't be adding special-cases like this if
we can at all avoid them. I think we should revert this part of the patch, which
I realize means putting back in the FIXME in
RenderBlock::isSelfCollapsingBlock().