Collapse bottom margins of a last child and its parent if parent's height=auto

Collapse bottom margins of a last child and its parent if parent's height=auto

This patch adds support of the margin collapsing case for the next vertically-adjacent box edges:
- bottom margin of a last in-flow child and bottom margin of its parent if the parent has 'auto' computed height
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

BUG=635619
Committed: https://crrev.com/329849949a9a4d49593c75167f2fc6fc8f405d48
Cr-Commit-Position: refs/heads/master@{#418407}

[Gleb Lanbin, 2016-09-13 18:49:29]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-09-13 18:49:52]

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

[cbiesinger, 2016-09-13 18:59:48]

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

[cbiesinger, 2016-09-13 18:59:49]

some drive-by nits

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:26: ;
I recently learned that the style guide actually doesn't allow this:
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

Mind fixing that while you're touching this?

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:69:
auto space = new NGConstraintSpace(
auto* please, for all of these -- I think that was the consensus on previous
discussions

[ikilpatrick, 2016-09-13 19:08:39]

lgtm

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:115: if
(current_child_->Style()->height().isAuto()) {
not logicalHeight here?

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:69:
auto space = new NGConstraintSpace(
auto*

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:175:
auto space = new NGConstraintSpace(
auto*

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_units.cc:98: return
std::tie(other.margin_block_start, other.margin_block_end,
I assume there is no overhead for using std::tie?

[Gleb Lanbin, 2016-09-13 19:33:45]

thanks for the review

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:115: if
(current_child_->Style()->height().isAuto()) {
On 2016/09/13 19:08:39, ikilpatrick wrote:
> not logicalHeight here?

Done.

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:26: ;
On 2016/09/13 18:59:48, cbiesinger wrote:
> I recently learned that the style guide actually doesn't allow this:
> https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements
> 
> Mind fixing that while you're touching this?

Done.

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:69:
auto space = new NGConstraintSpace(
On 2016/09/13 19:08:39, ikilpatrick wrote:
> auto*

Done.

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:69:
auto space = new NGConstraintSpace(
On 2016/09/13 18:59:49, cbiesinger wrote:
> auto* please, for all of these -- I think that was the consensus on previous
> discussions

Done.

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:175:
auto space = new NGConstraintSpace(
On 2016/09/13 19:08:39, ikilpatrick wrote:
> auto*

Done.

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

https://codereview.chromium.org/2336353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_units.cc:98: return
std::tie(other.margin_block_start, other.margin_block_end,
On 2016/09/13 19:08:39, ikilpatrick wrote:
> I assume there is no overhead for using std::tie?

std::tie is approved to use in Chromium
https://chromium-cpp.appspot.com/

[Gleb Lanbin, 2016-09-13 19:33:50]

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

[commit-bot: I haz the power, 2016-09-13 19:34:10]

Dry run: 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-09-13 20:05:43]

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

[commit-bot: I haz the power, 2016-09-13 20:05:44]

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

[Gleb Lanbin, 2016-09-13 20:26:54]

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

[commit-bot: I haz the power, 2016-09-13 20:27:26]

Dry run: 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-09-13 20:57:44]

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

[commit-bot: I haz the power, 2016-09-13 20:57:46]

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

[Gleb Lanbin, 2016-09-13 21:47:39]

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

[commit-bot: I haz the power, 2016-09-13 21:48:05]

Dry run: 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-09-13 23:14:42]

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

[commit-bot: I haz the power, 2016-09-13 23:14:43]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2016-09-13 23:15:46]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-09-13 23:15:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2336353002/#ps80001
(title: "add CORE_EXPORT")

[commit-bot: I haz the power, 2016-09-13 23:16:26]

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-09-13 23:22:55]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-13 23:25:17]

Description was changed from

==========
Collapse bottom margins of a last child and its parent if parent's height=auto

This patch adds support of the margin collapsing case for the next
vertically-adjacent box edges:
- bottom margin of a last in-flow child and bottom margin of its parent if the
parent has 'auto' computed height
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

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

to

==========
Collapse bottom margins of a last child and its parent if parent's height=auto

This patch adds support of the margin collapsing case for the next
vertically-adjacent box edges:
- bottom margin of a last in-flow child and bottom margin of its parent if the
parent has 'auto' computed height
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

BUG=635619

Committed: https://crrev.com/329849949a9a4d49593c75167f2fc6fc8f405d48
Cr-Commit-Position: refs/heads/master@{#418407}
==========

[commit-bot: I haz the power, 2016-09-13 23:25:19]

Patchset 4 (id:??) landed as
https://crrev.com/329849949a9a4d49593c75167f2fc6fc8f405d48
Cr-Commit-Position: refs/heads/master@{#418407}