Fix incorrectly calculated content_size for fragments that create new FC

Fix incorrectly calculated content_size for fragments that create new FC

This patch adds MarginStrut.BlockEndSum to the fragment's content size
if the fragment establishes a new formatting context.

The change is tested with layout_ng virtual test suite located in fast/block/basic.

BUG=635619
Committed: https://crrev.com/1163ff6253bce80dde1d8960c80ed5deaf32c0a7
Cr-Commit-Position: refs/heads/master@{#422878}

[Gleb Lanbin, 2016-10-03 19:52:13]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-10-03 19:52:40]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 19:53:12]

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

[Gleb Lanbin, 2016-10-03 19:53:53]

glebl@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[cbiesinger, 2016-10-03 19:58:27]

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

[cbiesinger, 2016-10-03 19:58:27]

I would probably separate out the addition of the virtual test suite

https://codereview.chromium.org/2389823003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2389823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:111:
content_size_ += builder_->MarginStrut().BlockEndSum();
That does not look right, shouldn't this be handled by the parent? We don't add
margin-start to content_size either.

[cbiesinger, 2016-10-03 20:13:07]

> That does not look right, shouldn't this be handled by the parent? We don't
add
> margin-start to content_size either.

Err, I meant margin-block-start (aka margin-before)

[commit-bot: I haz the power, 2016-10-03 22:42:22]

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

[commit-bot: I haz the power, 2016-10-03 22:42:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gleb Lanbin, 2016-10-04 14:43:33]

Patchset #2 (id:40001) has been deleted

[Gleb Lanbin, 2016-10-04 14:47:49]

PTAL

https://codereview.chromium.org/2389823003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2389823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:111:
content_size_ += builder_->MarginStrut().BlockEndSum();
On 2016/10/03 19:58:27, cbiesinger wrote:
> That does not look right, shouldn't this be handled by the parent? We don't
add
> margin-start to content_size either.

the logic is correct, I double verified it with Ian. I moved this part to
CollapseMargins method. Hope it looks better now.

[Gleb Lanbin, 2016-10-04 14:48:03]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 14:48:19]

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

[cbiesinger, 2016-10-04 15:03:03]

On 2016/10/04 14:47:49, glebl wrote:
> PTAL
> 
>
https://codereview.chromium.org/2389823003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
> (right):
> 
>
https://codereview.chromium.org/2389823003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:111:
> content_size_ += builder_->MarginStrut().BlockEndSum();
> On 2016/10/03 19:58:27, cbiesinger wrote:
> > That does not look right, shouldn't this be handled by the parent? We don't
> add
> > margin-start to content_size either.
> 
> the logic is correct, I double verified it with Ian. I moved this part to
> CollapseMargins method. Hope it looks better now.

Sorry, you're right -- I misunderstood the code. However, the new code is
clearer, so thank you :-)

lgtm!

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

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

[commit-bot: I haz the power, 2016-10-04 16:14:21]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2016-10-04 16:21:36]

this looks great, just one question about clearing the margin strut :)

https://codereview.chromium.org/2389823003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2389823003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:166:
builder_->ClearMarginStrut();
isn't this immediately going to be set at UpdateMarginStrut?

Instead of clearing can we just set it when needed?

[Gleb Lanbin, 2016-10-04 17:25:10]

Patchset #3 (id:80001) has been deleted

[Gleb Lanbin, 2016-10-04 17:25:19]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 17:25:52]

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

[Gleb Lanbin, 2016-10-04 17:29:11]

On 2016/10/04 16:21:36, ikilpatrick wrote:
> this looks great, just one question about clearing the margin strut :)
> 
>
https://codereview.chromium.org/2389823003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
> (right):
> 
>
https://codereview.chromium.org/2389823003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:166:
> builder_->ClearMarginStrut();
> isn't this immediately going to be set at UpdateMarginStrut?
> 
> Instead of clearing can we just set it when needed?

thanks for catching this.
1) regrouped the code so "NewFormattingContext" logic resides together. I think
it makes the code less error-prone.
2) changed the test to verify that we produce an empty margin strut of the
fragment that establishes a new formatting context

PTAL

[ikilpatrick, 2016-10-04 17:56:42]

lgtm, thanks!

[Gleb Lanbin, 2016-10-04 17:58:50]

The CQ bit was unchecked by glebl@chromium.org

[Gleb Lanbin, 2016-10-04 17:58:56]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-10-04 17:58:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2389823003/#ps100001
(title: "Regroup the code to put together the Collapsing Margins logic for
NewFormattingContext's fragments")

[commit-bot: I haz the power, 2016-10-04 17:59:11]

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-04 19:09:22]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-10-04 19:15:23]

Description was changed from

==========
Fix incorrectly calculated content_size for fragments that create new FC

This patch adds MarginStrut.BlockEndSum to the fragment's content size
if the fragment establishes a new formatting context.

The change is tested with layout_ng virtual test suite located in
fast/block/basic.

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

to

==========
Fix incorrectly calculated content_size for fragments that create new FC

This patch adds MarginStrut.BlockEndSum to the fragment's content size
if the fragment establishes a new formatting context.

The change is tested with layout_ng virtual test suite located in
fast/block/basic.

BUG=635619

Committed: https://crrev.com/1163ff6253bce80dde1d8960c80ed5deaf32c0a7
Cr-Commit-Position: refs/heads/master@{#422878}
==========

[commit-bot: I haz the power, 2016-10-04 19:15:24]

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