Strip anonymous blocks when change in style removes need for them

Strip anonymous blocks when change in style removes need for them

When a block element becomes a float or out of flow we should strip
any anonymous blocks wrapping its inline siblings.

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

[ojan, 2014-05-06 18:43:02]

jchaffraix, you mind reviewing this? I think you know about anonymous wrapper
code more than I do.

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

https://codereview.chromium.org/253313005/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:338: if (s_noLongerAffectsParentBlock)
Why do you need the static? Can't you do the check in styleWillChange here
instead?

[esprehn, 2014-05-06 18:55:44]

Things like this should really be a post processing step for recalcStyle, this
means toggling all child elements between positioning of floating produces n^2
behavior looking for the wrappers.

Is this only a memory savings?

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

https://codereview.chromium.org/253313005/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:113: static bool
s_noLongerAffectsParentBlock = false;
This should be a member of RenderBlock like the other static bits.

[Julien - ping for review, 2014-05-07 01:38:46]

Some comments, I haven't thought about the change in great details.

> Is this only a memory savings?

I would expect this to be a functional bugs as anonymous wrappers left in the
tree can impacts our tree placement algorithm (at least table had visible issues
before we implemented anonymous wrapper cleaning).

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

https://codereview.chromium.org/253313005/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:313: s_noLongerAffectsParentBlock =
!isFloatingOrOutOfFlowPositioned()
RenderObject has a static boolean called s_affectsParentBlock, are those
related?

https://codereview.chromium.org/253313005/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1066: for (RenderObject* child =
firstChild(); child; child = child->nextSibling()) {
You seem to care only about blocks so we should probably just walk our box
children.

Better even, don't have a children walk here.

https://codereview.chromium.org/253313005/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1081: while (child) {
Same comment about walking boxes (ideally blocks but we don't have them).

https://codereview.chromium.org/253313005/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1085: child = next;
I would put a comment here as this is important:

// WARNING: |child| is destroyed here.

[rhogan, 2014-05-19 20:25:05]

On 2014/05/07 01:38:46, Julien Chaffraix - CET wrote:
> Some comments, I haven't thought about the change in great details.
> 
Julie - Updated per your comments. Can you take another look?

[esprehn, 2014-05-19 20:54:57]

How do you know the children will always be blocks? Are you sure they can't be
boxes?

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

https://codereview.chromium.org/253313005/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:331: if (isFloatingOrOutOfFlowPositioned()
&& (oldStyle && !oldStyle->isFloating() && !oldStyle->hasOutOfFlowPosition()) &&
parent() && parent()->isRenderBlockFlow())
You don't need these nested parens, it's all &&

https://codereview.chromium.org/253313005/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1060: RenderBlock* child =
toRenderBlock(firstChildBox());
How do you know this cast is safe?

https://codereview.chromium.org/253313005/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1062: RenderBlock* next =
toRenderBlock(child->nextSiblingBox());
ditto

[esprehn, 2014-05-20 19:42:45]

lgtm, few nits

https://codereview.chromium.org/253313005/diff/90001/LayoutTests/fast/block/s...
File
LayoutTests/fast/block/strip-anonymous-blocks-when-block-child-becomes-float.html
(right):

https://codereview.chromium.org/253313005/diff/90001/LayoutTests/fast/block/s...
LayoutTests/fast/block/strip-anonymous-blocks-when-block-child-becomes-float.html:3:
<head>
We usually leave off <html>, <head> and <body>

https://codereview.chromium.org/253313005/diff/90001/LayoutTests/fast/block/s...
LayoutTests/fast/block/strip-anonymous-blocks-when-block-child-becomes-float.html:20:
<body onload="checkLayout('#container')">
I'd just do onload = function() { ... } in the script at the bottom.

[rhogan, 2014-05-20 20:46:25]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-05-20 20:47:55]

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

[commit-bot: I haz the power, 2014-05-20 22:15:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-20 22:48:08]

Change committed as 174420

[esprehn, 2014-05-22 17:14:05]

A revert of this CL has been created in
https://codereview.chromium.org/287193013/ by esprehn@chromium.org.

The reason for reverting is: Looks like it's causing crashes:

CPU Info: GenuineIntel family 6 model 37 stepping 5
=========================================================
Thread 0 CRASHED [EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000720] MAGIC SIGNATURE
THREAD
	0x557533da	[chrome_child.dll -renderblocklinelayout.cpp:166
]	WebCore::RenderBlockFlow::createLineBoxes(WebCore::RenderObject
*,WebCore::LineInfo const &,WebCore::InlineBox *)
	0x557531b2	[chrome_child.dll -renderblocklinelayout.cpp:267
]	WebCore::RenderBlockFlow::constructLine(WebCore::BidiRunList<WebCore::BidiRun>
&,WebCore::LineInfo const &)
	0x55752f4b	[chrome_child.dll -renderblocklinelayout.cpp:825
]	WebCore::RenderBlockFlow::createLineBoxesFromBidiRuns(unsigned
int,WebCore::BidiRunList<WebCore::BidiRun> &,WebCore::InlineIterator const
&,WebCore::LineInfo &,WebCore::VerticalPositionCache &,WebCore::BidiRun
*,WTF::Vector<WebCore::WordMeasurement,64,WTF::DefaultAllocator> &)
	0x5574f44c	[chrome_child.dll -renderblocklinelayout.cpp:1016
]	WebCore::RenderBlockFlow::layoutRunsAndFloatsInRange(WebCore::LineLayoutState
&,WebCore::BidiResolver<WebCore::InlineIterator,WebCore::BidiRun>
&,WebCore::InlineIterator const &,WebCore::BidiStatus const &,unsigned int)
	0x5574e3d0	[chrome_child.dll -renderblocklinelayout.cpp:916
]	WebCore::RenderBlockFlow::layoutRunsAndFloats(WebCore::LineLayoutState &)
	0x555dff70	[chrome_child.dll -renderblocklinelayout.cpp:1734
]	WebCore::RenderBlockFlow::layoutInlineChildren(bool,WebCore::LayoutUnit
&,WebCore::LayoutUnit &,WebCore::LayoutUnit)
	0x555d86c1	[chrome_child.dll -renderblockflow.cpp:384
]	WebCore::RenderBlockFlow::layoutBlockFlow(bool,WebCore::LayoutUnit
&,WebCore::SubtreeLayoutScope &)
	0x555d8049	[chrome_child.dll -renderblockflow.cpp:313
]	WebCore::RenderBlockFlow::layoutBlock(bool)
	0x555d7ce4	[chrome_child.dll -renderblock.cpp:1352
]	WebCore::RenderBlock::layout()
	0x5575b223	[chrome_child.dll -renderblockflow.cpp:2217
]	WebCore::RenderBlockFlow::insertFloatingObject(WebCore::RenderBox *)
	0x555dc43d	[chrome_child.dll -renderblockflow.cpp:921
]	WebCore::RenderBlockFlow::layoutBlockChildren(bool,WebCore::SubtreeLayoutScope
&,WebCore::LayoutUnit,WebCore::LayoutUnit)
[...]
	0x555dc45b	[chrome_child.dll -renderblockflow.cpp:927
]	WebCore::RenderBlockFlow::layoutBlockChildren(bool,WebCore::SubtreeLayoutScope
&,WebCore::LayoutUnit,WebCore::LayoutUnit)
	0x555d86cf	[chrome_child.dll -renderblockflow.cpp:386
]	WebCore::RenderBlockFlow::layoutBlockFlow(bool,WebCore::LayoutUnit
&,WebCore::SubtreeLayoutScope &)
	0x555d8049	[chrome_child.dll -renderblockflow.cpp:313
]	WebCore::RenderBlockFlow::layoutBlock(bool)
	0x555d7ce4	[chrome_child.dll -renderblock.cpp:1352
]	WebCore::RenderBlock::layout()
	0x555d7abc	[chrome_child.dll -renderview.cpp:225
]	WebCore::RenderView::layout()
	0x555d6ee0	[chrome_child.dll -frameview.cpp:802
]	WebCore::FrameView::performLayout(WebCore::RenderObject *,bool)
	0x555997f6	[chrome_child.dll -frameview.cpp:974
]	WebCore::FrameView::layout(bool)
	0x554f33b2	[chrome_child.dll -document.cpp:2598
]	WebCore::Document::implicitClose()
	0x554f2e10	[chrome_child.dll -frameloader.cpp:467
]	WebCore::FrameLoader::checkCompleted()
	0x555f718e	[chrome_child.dll -resourcefetcher.cpp:1105
]	WebCore::ResourceFetcher::didLoadResource(WebCore::Resource *)
	0x555f6ef6	[chrome_child.dll -resourceloader.cpp:100
]	WebCore::ResourceLoader::releaseResources()
	0x555f5dfa	[chrome_child.dll -resourceloader.cpp:433
]	WebCore::ResourceLoader::didFinishLoading(blink::WebURLLoader
*,double,__int64)
	0x555f5ccf	[chrome_child.dll -web_url_loader_impl.cc:639
]	content::WebURLLoaderImpl::Context::OnCompletedRequest(int,bool,bool,std::basic_string<char,std::char_traits<char>,std::allocator<char>
> const &,base::TimeTicks const &,__int64)
.

[esprehn, 2014-05-27 20:05:02]

https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1086: void
RenderBlock::removeAnonymousWrappersIfRequired()
Should this method be in RenderBlockFlow instead? You check isRenderBlockFlow
above.

https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1088: Vector<RenderBox*> blocksToRemove;
Add inline capacity, I'd go with 8 or 16.

https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1099: child = child->nextSiblingBox();
This should be a for() loop now.

https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1102: for (unsigned i = 0; i <
blocksToRemove.size(); i++)
size_t

[rhogan, 2014-05-27 20:56:14]

On 2014/05/27 20:05:02, esprehn wrote:
>
https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/253313005/diff/120001/Source/core/rendering/R...
> Source/core/rendering/RenderBlock.cpp:1086: void
> RenderBlock::removeAnonymousWrappersIfRequired()
> Should this method be in RenderBlockFlow instead? You check isRenderBlockFlow
> above.

It seems better to keep it in the same place as the other functions that
manipulate anonymous blocks.

[esprehn, 2014-05-27 21:44:42]

lgtm, lets try it again!

https://codereview.chromium.org/253313005/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/253313005/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1088: Vector<RenderBox*, 16>
blocksToRemove;
I would ASSERT(isRenderBlockFlow()) here if you expect it to only be called on
those.

[rhogan, 2014-06-01 11:48:31]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-06-01 11:48:44]

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

[commit-bot: I haz the power, 2014-06-01 12:48:38]

Change committed as 175220

[rhogan, 2014-06-22 22:47:52]

A revert of this CL has been created in
https://codereview.chromium.org/349103002/ by robhogan@gmail.com.

The reason for reverting is: Caused an incorrect cast in RenderBlockFlow.

TBR=inferno@chromium.org,eae@chromium.org
BUG=387211.

[rhogan, 2014-11-10 19:22:57]

https://codereview.chromium.org/253313005/diff/190001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/253313005/diff/190001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1118: if (!child->childrenInline())
This addresses the cause of the clusterfuzz crashes that forced me to revert my
last attempt.

[ojan, 2014-11-10 19:32:49]

ojan@chromium.org changed reviewers:
- ojan@chromium.org

[leviw_travelin_and_unemployed, 2014-11-10 21:46:10]

The code looks fine. Just adding obnoxious nits to your test case ;)

https://codereview.chromium.org/253313005/diff/190001/LayoutTests/fast/block/...
File
LayoutTests/fast/block/do-not-strip-anonymous-blocks-when-block-child-becomes-float-and-anonymous-blocks-have-inline-children.html
(right):

https://codereview.chromium.org/253313005/diff/190001/LayoutTests/fast/block/...
LayoutTests/fast/block/do-not-strip-anonymous-blocks-when-block-child-becomes-float-and-anonymous-blocks-have-inline-children.html:3:
.c2 { position: fixed; }
Can we get descriptive names instead of c2, c4, c7,. etc?

https://codereview.chromium.org/253313005/diff/190001/LayoutTests/fast/block/...
LayoutTests/fast/block/do-not-strip-anonymous-blocks-when-block-child-becomes-float-and-anonymous-blocks-have-inline-children.html:5:
.c7:nth-child(2n+10) { -webkit-column-count: 1; }
Seems like 2n+10 could also be a smaller #.

https://codereview.chromium.org/253313005/diff/190001/LayoutTests/fast/block/...
LayoutTests/fast/block/do-not-strip-anonymous-blocks-when-block-child-becomes-float-and-anonymous-blocks-have-inline-children.html:12:
nav = document.createElement('nav');
Add "var" for these variables.

https://codereview.chromium.org/253313005/diff/190001/LayoutTests/fast/block/...
LayoutTests/fast/block/do-not-strip-anonymous-blocks-when-block-child-becomes-float-and-anonymous-blocks-have-inline-children.html:36:
<p></p>
Do we need this many p tags?

[esprehn, 2014-11-11 19:02:40]

lgtm w/ nits from leviw@ addressed.

[rhogan, 2014-11-11 20:54:13]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-11-11 20:54:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/253313005/210001

[commit-bot: I haz the power, 2014-11-11 22:11:40]

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

[commit-bot: I haz the power, 2014-11-11 22:11:42]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[rhogan, 2014-11-12 10:28:11]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-11-12 10:28:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/253313005/230001

[commit-bot: I haz the power, 2014-11-12 11:40:43]

Committed patchset #14 (id:230001) as 185200