CanUseNewLayout should work for more cases

CanUseNewLayout should trigger nglayout in more cases than we handle today.

CanUseNewLayout was returning false even if there were no inline children.
Ian confirmed that if there were no inline children, we should use
nglayout.

The root cause is that LayoutBlockFlow.isInline flag is initialized to true
in constructor.
It gets reset in LayoutBlockFlow::makeChildrenNonInline.
This code only gets called if at least one BlockFlow child is laid out. If all your children are absolute, this does not happen.

This change also causes 6 more existing virtual/nglayout test failures that I have not investigated.

BUG=635619
Committed: https://crrev.com/453444244e58b4e26fc7e5772e75f7ed8b05d985
Cr-Commit-Position: refs/heads/master@{#428150}

[atotic, 2016-10-25 23:07:11]

Description was changed from

==========
CanUseNewLayout should work for more cases

BUG=635619
==========

to

==========
CanUseNewLayout should trigger nglayout in more cases than we handle today.

CanUseNewLayout was returning false even if there were no inline children.
Ian confirmed that if there were no inline children, we should use
nglayout.

The root cause is that LayoutBlockFlow.isInline flag is initialized to true
in constructor.
It gets reset in LayoutBlockFlow::makeChildrenNonInline.
This code only gets called if at least one BlockFlow child is laid out. If all
your children are absolute, this does not happen.

This change also causes 6 more existing virtual/nglayout test failures that I
have not investigated.

BUG=635619
==========

[atotic, 2016-10-25 23:07:11]

atotic@chromium.org changed reviewers:
+ eae@chromium.org, glebl@chromium.org, ikilpatrick@chromium.org

[eae, 2016-10-26 17:02:15]

eae@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[eae, 2016-10-26 17:02:16]

OK, please check with cbiesinger before landing.

LGTM

[Gleb Lanbin, 2016-10-26 17:06:34]

On 2016/10/26 05:19:13, atotic2 wrote:

what's your plan for "causes 6 more existing virtual/nglayout test failures" ?
Could you run CQ for this patch ?

[cbiesinger, 2016-10-26 17:17:51]

I think this is fine -- I believe we handle floats now, which is the other case
this would cover, and I can't think of anything else. lgtm, pending some kind of
resolution on the test failures.

[atotic, 2016-10-26 17:47:26]

On 2016/10/26 at 17:17:51, cbiesinger wrote:
> I think this is fine -- I believe we handle floats now, which is the other
case this would cover, and I can't think of anything else. lgtm, pending some
kind of resolution on the test failures.

The 6 tests that are failing, 3 crashers, and 3 failures. My plan is to
- comment out 3 failures. This just goes into pile of other tests we are
failing.
- glebl is going to take a look at crashes. They are all the same assertion
failure.

Tests that crashed (3): [flag all] [unflag all]

+virtual/layout_ng/fast/block/float/floats-not-cleared-crash.html
+virtual/layout_ng/fast/block/float/float-not-removed-from-next-sibling-crash.html
+virtual/layout_ng/fast/block/float/float-not-removed-from-next-sibling5.html

Tests that failed text/pixel/audio diff (3): [flag all] [unflag all]

+virtual/layout_ng/fast/block/float/floats-with-margin-should-not-wrap.html
+virtual/layout_ng/fast/block/float/previous-sibling-float-001.html
+virtual/layout_ng/fast/block/float/relative-painted-twice.html

The crashes are all the same crash:

STDERR: ASSERTION FAILED: floatingObject.isPlaced() &&
floatingObject.isInPlacedTree()
STDERR: ../../third_party/WebKit/Source/core/layout/FloatingObjects.cpp(537) :
void blink::FloatingObjects::removePlacedObject(blink::FloatingObject &)
blink::LayoutBlockFlow::removeFloatingObject(blink::LayoutBox*)
blink::LayoutBlockFlow::markAllDescendantsWithFloatsForLayout(blink::LayoutBox*,
bool)
blink::LayoutBox::removeFloatingOrPositionedChildFromBlockLists()
blink::LayoutObject::removeChild(blink::LayoutObject*)
blink::LayoutBlockFlow::removeChild(blink::LayoutObject*)
blink::LayoutObject::willBeDestroyed()

[atotic, 2016-10-26 17:59:27]

Just noticed we are also testing margin-collapse. This one fails too, plan to
comment it out:

virtual/layout_ng/fast/block/margin-collapse/positioned-element-margin-change.html

[cbiesinger, 2016-10-26 19:21:05]

sgtm

[atotic, 2016-10-26 19:43:36]

On 2016/10/26 at 17:59:27, atotic2 wrote:
> Just noticed we are also testing margin-collapse. This one fails too, plan to
comment it out:
> 
>
virtual/layout_ng/fast/block/margin-collapse/positioned-element-margin-change.html

I ran the complete virtual test suite (with skipped tests). First the good news:

Tests that are now passing:

virtual/layout_ng/fast/block/margin-collapse/041.html.

Bad news:

25 crashes in float suite, some new interesting causes

#1: Multicolumn set
virtual/layout_ng/fast/block/float/trailing-float-with-columns.html
STDERR: ASSERTION FAILED: selfNeedsLayout()
STDERR: ../../third_party/WebKit/Source/core/layout/LayoutFlowThread.cpp(83) :
void blink::LayoutFlowThread::invalidateColumnSets()
STDERR: 1   0x7f8a41ca4c01 blink::LayoutFlowThread::invalidateColumnSets()
STDERR: 2   0x7f8a41ca4b87
blink::LayoutFlowThread::removeColumnSetFromThread(blink::LayoutMultiColumnSet*)

#2: opportunity.isEmpty
virtual/layout_ng/fast/block/float/containing-block-change-compositing.html
virtual/layout_ng/fast/block/float/add-inline-to-block-flow-and-ensure-layout-on-containers-of-removed-floats.html	
virtual/layout_ng/fast/block/float/overhanging-float-container-remove-compositing.html

STDERR: [1:1:1026/110856:158087488336:FATAL:ng_block_layout_algorithm.cc(332)]
Check failed: !opportunity.IsEmpty(). Opportunity is empty but it shouldn't be
STDERR: #0 0x7f75a9f4f06e base::debug::StackTrace::StackTrace()
STDERR: #1 0x7f75a9fbe59f logging::LogMessage::~LogMessage()
STDERR: #2 0x7f75a1ba5036 blink::NGBlockLayoutAlgorithm::PositionFloatFragment()

#3: multi-column set, different crash
virtual/layout_ng/fast/block/float/floats-do-not-overhang-from-block-formatting-context.html
virtual/layout_ng/fast/block/float/float-not-removed-from-next-sibling4.html
STDOUT: #CRASHED - renderer
STDERR:
[1:1:1026/110939:158130856781:FATAL:LayoutMultiColumnFlowThread.cpp(448)] Check
failed: !m_columnSetsInvalidated. 
STDERR: #0 0x7f601456d06e base::debug::StackTrace::StackTrace()
STDERR: #1 0x7f60145dc59f logging::LogMessage::~LogMessage()
STDERR: #2 0x7f600c0c661d
blink::LayoutMultiColumnFlowThread::columnSetAtBlockOffset()
STDERR: #3 0x7f600bfd68f7 blink::FragmentainerIterator::FragmentainerIterator()
STDERR: #4 0x7f600c37ba85 blink::PaintLayer::collectFragments()
STDERR: #5 0x7f600c38cf18 blink::PaintLayerPainter::paintLayerContents()

#4 ng_length_utils.cc
virtual/layout_ng/fast/block/float/overhanging-float-crashes-when-sibling-becomes-formatting-context.html
virtual/layout_ng/fast/block/float/add-float-back-to-anonymous-block.html

STDERR: [1:1:1026/111054:158205980799:FATAL:ng_length_utils.cc(65)] Check
failed: constraintSpace.ContainerSize().inline_size >= LayoutUnit() ("-1" vs.
"0")
STDERR: #0 0x7f4e3f0cc06e base::debug::StackTrace::StackTrace()
STDERR: #1 0x7f4e3f13b59f logging::LogMessage::~LogMessage()
STDERR: #2 0x7f4e36d313b0 blink::ResolveInlineLength()
STDERR: #3 0x7f4e36d3188c blink::ComputePadding()

[cbiesinger, 2016-10-26 21:47:17]

On 2016/10/26 19:43:36, atotic2 wrote:
> #4 ng_length_utils.cc
>
virtual/layout_ng/fast/block/float/overhanging-float-crashes-when-sibling-becomes-formatting-context.html

This one crashes due to vertical writing mode, where we swap the -1 and 0 to get
a result we don't expect :/

Probably mark as [Crash] for now.

I need to come up with a better way to handle this (and... see how our current
code handles it)

> virtual/layout_ng/fast/block/float/add-float-back-to-anonymous-block.html

I get a different assert for that one:
537	  ASSERT(floatingObject.isPlaced() && floatingObject.isInPlacedTree());
(and for what it's worth, isPlaced() is true but isInPlacedTree() is false)

[Gleb Lanbin, 2016-10-26 23:15:57]

https://codereview.chromium.org/2444323003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2444323003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:208: crbug.com/635619
virtual/layout_ng/fast/block/margin-collapse/positioned-element-margin-change.html
[ Skip ]
please update numbers at #155 line, same below.

[atotic, 2016-10-27 20:09:39]

On 2016/10/26 at 23:15:57, glebl wrote:
>
https://codereview.chromium.org/2444323003/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2444323003/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:208: crbug.com/635619
virtual/layout_ng/fast/block/margin-collapse/positioned-element-margin-change.html
[ Skip ]
> please update numbers at #155 line, same below.

Done.

I've marked additional 5 failing tests as [ Skip ]

Now there are 25 tests in float test suite that crash. I've tried marking them
as [ Skip Crash ], but had to back it out.  Testrunner does not skip tests
marked [Skip Crash]

I'll commit now.

[atotic, 2016-10-27 20:09:42]

The CQ bit was checked by atotic@chromium.org

[atotic, 2016-10-27 20:09:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2444323003/#ps40001
(title: "Adjust TestExpectations")

[commit-bot: I haz the power, 2016-10-27 20:10:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 22:05:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 22:08:09]

Description was changed from

==========
CanUseNewLayout should trigger nglayout in more cases than we handle today.

CanUseNewLayout was returning false even if there were no inline children.
Ian confirmed that if there were no inline children, we should use
nglayout.

The root cause is that LayoutBlockFlow.isInline flag is initialized to true
in constructor.
It gets reset in LayoutBlockFlow::makeChildrenNonInline.
This code only gets called if at least one BlockFlow child is laid out. If all
your children are absolute, this does not happen.

This change also causes 6 more existing virtual/nglayout test failures that I
have not investigated.

BUG=635619
==========

to

==========
CanUseNewLayout should trigger nglayout in more cases than we handle today.

CanUseNewLayout was returning false even if there were no inline children.
Ian confirmed that if there were no inline children, we should use
nglayout.

The root cause is that LayoutBlockFlow.isInline flag is initialized to true
in constructor.
It gets reset in LayoutBlockFlow::makeChildrenNonInline.
This code only gets called if at least one BlockFlow child is laid out. If all
your children are absolute, this does not happen.

This change also causes 6 more existing virtual/nglayout test failures that I
have not investigated.

BUG=635619

Committed: https://crrev.com/453444244e58b4e26fc7e5772e75f7ed8b05d985
Cr-Commit-Position: refs/heads/master@{#428150}
==========

[commit-bot: I haz the power, 2016-10-27 22:08:10]

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