Issue 1611413007: Remove unnecessary anonymous blocks when splitting anonymous boxes

Issue 1611413007: Remove unnecessary anonymous blocks when splitting anonymous boxes (Closed)

Created:
4 years, 11 months ago by rhogan

Modified:
4 years, 10 months ago

Reviewers:
mstensho (USE GERRIT)

CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1

Base URL:
https://chromium.googlesource.com/chromium/src.git@568387-RemovePercentHeight

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Remove unnecessary anonymous blocks when splitting anonymous boxes BUG=568387 Committed: https://crrev.com/45061dd7b68e9561d7a44fd0ba0f2f5e0219cdde Cr-Commit-Position: refs/heads/master@{#373096}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Updated #

Total comments: 2

Patch Set 3 : Updated #

Created: 4 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+123 lines, -1 line)			Patch
M	third_party/WebKit/LayoutTests/TestExpectations	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html	View		1 chunk	+12 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-3.html	View		1 chunk	+13 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-4.html	View	1	1 chunk	+12 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-2-expected.png	View	1	Binary file		0 comments	Download
A	third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-2-expected.txt	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-3-expected.png	View	1	Binary file		0 comments	Download
A	third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-3-expected.txt	View	1	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-4-expected.png	View	1	Binary file		0 comments	Download
A	third_party/WebKit/LayoutTests/platform/linux/fast/table/split-table-section-before-anonymous-block-4-expected.txt	View	1	1 chunk	+18 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutBlock.h	View	1 2	2 chunks	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutBlock.cpp	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutBox.cpp	View	1 2	2 chunks	+20 lines, -0 lines	0 comments	Download

Messages

Total messages: 14 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

rhogan

> https://codereview.chromium.org/1551573002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode4680 > third_party/WebKit/Source/core/layout/LayoutBox.cpp:4680: // We're splitting from our previousSibling(). > On 2016/01/24 12:31:37, rhogan ...

4 years, 11 months ago (2016-01-26 20:30:36 UTC) #2

mstensho (USE GERRIT)

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html File third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html (right): https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html#newcode1 third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html:1: <!DOCTYPE html> These tests both pass *without* your code ...

4 years, 10 months ago (2016-01-28 20:45:14 UTC) #3

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html
(right):

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html:1:
<!DOCTYPE html>
These tests both pass *without* your code changes too.

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4673: static bool
collapseIfWillBecomeLoneAnonymousBlock(LayoutBox* child)
I think this would fit better in LayoutBlock. Doing so much LayoutBlock-specific
stuff here isn't so great.

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4680: // We're splitting
from our previousSibling().
I'm still uneasy about this.

I think we should rather nuke superfluous anonymous blocks after the split,
rather than guessing what the tree is going to look like before making the
split.

So, basically, for each postBox (once each is created) in
LayoutBox::splitAnonymousBoxesAroundChild():
If postBox only has one child, and that child is an anonyomus block that's
collapsable, collapseAnonymousBlockChild() it!

And don't we really have to do the same thing for the other side of the split as
well? Might end up with a lone anonymous block there too:

<div style="display:table-row-group;">
    <div style="display:inline-block; width:75px; height:50%;
background:blue;"></div>
    <div id="splitter" style="display:table-row;"></div>
    <div></div>
</div>

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4683: LayoutBlock* parent =
child->containingBlock();
This should always be the same as child->parent(), or we'd be in trouble, right?

Then I think it would be safer (and slightly faster, woho!) to call
child->parent() and bail if it's not a block.

rhogan

4 years, 10 months ago (2016-01-30 12:38:58 UTC) #4

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html
(right):

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/split-table-section-before-anonymous-block-2.html:1:
<!DOCTYPE html>
On 2016/01/28 at 20:45:14, mstensho wrote:
> These tests both pass *without* your code changes too.

Yes, I give up on making these non-layout-tree tests. We're testing the removal
of an unnecessary layout object so the render tree is the appropriate test I
think. (I tried a unit test and failed to get it to work.)

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4673: static bool
collapseIfWillBecomeLoneAnonymousBlock(LayoutBox* child)
On 2016/01/28 at 20:45:14, mstensho wrote:
> I think this would fit better in LayoutBlock. Doing so much
LayoutBlock-specific stuff here isn't so great.

Waaaah! I moved it here because you thought it should be static. :)

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4680: // We're splitting
from our previousSibling().
On 2016/01/28 at 20:45:14, mstensho wrote:
> I'm still uneasy about this.
> 
> I think we should rather nuke superfluous anonymous blocks after the split,
rather than guessing what the tree is going to look like before making the
split.

We're not guessing though - if we create postBox here and it has no
nextSibling() then it's a lone anonymous block and can be collapsed right away.

> 
> So, basically, for each postBox (once each is created) in
LayoutBox::splitAnonymousBoxesAroundChild():
> If postBox only has one child, and that child is an anonyomus block that's
collapsable, collapseAnonymousBlockChild() it!
> 
> And don't we really have to do the same thing for the other side of the split
as well? Might end up with a lone anonymous block there too:
> 
> <div style="display:table-row-group;">
>     <div style="display:inline-block; width:75px; height:50%;
background:blue;"></div>
>     <div id="splitter" style="display:table-row;"></div>
>     <div></div>
> </div>

All good points - applied.

https://codereview.chromium.org/1611413007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4683: LayoutBlock* parent =
child->containingBlock();
On 2016/01/28 at 20:45:14, mstensho wrote:
> This should always be the same as child->parent(), or we'd be in trouble,
right?
> 
> Then I think it would be safer (and slightly faster, woho!) to call
child->parent() and bail if it's not a block.

Done.

rhogan

> > I think we should rather nuke superfluous anonymous blocks after the split, rather ...

4 years, 10 months ago (2016-01-30 12:47:16 UTC) #5

mstensho (USE GERRIT)

lgtm I think you need a blank line before "BUG=568387" in the description? Or will ...

4 years, 10 months ago (2016-02-01 12:25:36 UTC) #6

rhogan

https://codereview.chromium.org/1611413007/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1611413007/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode4410 third_party/WebKit/Source/core/layout/LayoutBox.cpp:4410: child = boxToSplit->slowFirstChild(); On 2016/02/01 at 12:25:36, mstensho wrote: ...

4 years, 10 months ago (2016-02-02 21:38:25 UTC) #7

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-02 21:42:55 UTC) #11

commit-bot: I haz the power

4 years, 10 months ago (2016-02-03 00:22:13 UTC) #14

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/45061dd7b68e9561d7a44fd0ba0f2f5e0219cdde
Cr-Commit-Position: refs/heads/master@{#373096}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages