[layoutng] Allow testing NGBox without a LayoutObject

[layoutng] Allow testing NGBox without a LayoutObject

And use that to add a test for ng_block_layout_algorithm that tests layout
with children.

BUG=635619
R=eae@chromium.org,glebl@chromium.org,ikilpatrick@chromium.org

Committed: https://crrev.com/f2e5768ae0a7b3514005522adcfc93781852a0ec
Cr-Commit-Position: refs/heads/master@{#415762}

[cbiesinger, 2016-08-30 18:12:15]

I will wait until your patches are in the tree so as to not make your conflicts
worse (especially https://codereview.chromium.org/2282213002/)

[Gleb Lanbin, 2016-08-30 18:58:22]

lgtm

a couple of minor comments. thanks.

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:38:
TEST_F(NGBlockLayoutAlgorithmTest, Children) {
may be make the unit test name to be a bit more descriptive.
Example:
// Verifies that 2 block's children were lied out with correct sizes.
TEST_F(NGBlockLayoutAlgorithmTest, LayoutBlockChildren)

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:45:
first_style->setHeight(Length(20, Fixed));
I would use file level constants
kHeight1, kMarginBottom1 etc. and then

EXPECT_EQ(child->BlockOffset(), kMarginBottom1);

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:63:
EXPECT_EQ(frag->NumChildren(), 2ul);
.nit should be in uppercase 2UL

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:63:
EXPECT_EQ(frag->NumChildren(), 2ul);
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/113.md?cl=head

you should use ASSERT_EQ here

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_box.h (right):

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_box.h:38: void
SetFirstChild(NGBox*);
how these 2 methods will be used? If it's only for tests then maybe move it to
private and declare a friendly class?

[eae, 2016-08-30 22:43:48]

LGTM once glebs comments have been addressed.

[cbiesinger, 2016-08-31 19:34:49]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2016-08-31 19:34:50]

The patchset sent to the CQ was uploaded after l-g-t-m from glebl@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2291273002/#ps20001
(title: "review comments")

[cbiesinger, 2016-08-31 19:35:10]

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:38:
TEST_F(NGBlockLayoutAlgorithmTest, Children) {
On 2016/08/30 18:58:22, glebl wrote:
> may be make the unit test name to be a bit more descriptive.
> Example:
> // Verifies that 2 block's children were lied out with correct sizes.
> TEST_F(NGBlockLayoutAlgorithmTest, LayoutBlockChildren)

Done.

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:45:
first_style->setHeight(Length(20, Fixed));
On 2016/08/30 18:58:22, glebl wrote:
> I would use file level constants
> kHeight1, kMarginBottom1 etc. and then
> 
> EXPECT_EQ(child->BlockOffset(), kMarginBottom1);

Done, except that I used function-level constants because I am not sure it makes
sense to reuse them across tests. If it does, we can move them when we add
another test later.

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:63:
EXPECT_EQ(frag->NumChildren(), 2ul);
On 2016/08/30 18:58:22, glebl wrote:
> https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/113.md?cl=head
> 
> you should use ASSERT_EQ here

Done.

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:63:
EXPECT_EQ(frag->NumChildren(), 2ul);
On 2016/08/30 18:58:22, glebl wrote:
> .nit should be in uppercase 2UL

Done. That seems like something git cl format ought to handle...

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_box.h (right):

https://codereview.chromium.org/2291273002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_box.h:38: void
SetFirstChild(NGBox*);
On 2016/08/30 18:58:22, glebl wrote:
> how these 2 methods will be used? If it's only for tests then maybe move it to
> private and declare a friendly class?

No, they will eventually be used to construct the NGBox tree when we no longer
need the same interop we have now, so I would like to leave them as-is for now.

[commit-bot: I haz the power, 2016-08-31 19:35:11]

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

[cbiesinger, 2016-08-31 19:39:21]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2016-08-31 19:39:22]

The patchset sent to the CQ was uploaded after l-g-t-m from glebl@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2291273002/#ps40001
(title: "fix bad merge -- private: was duplicated")

[commit-bot: I haz the power, 2016-08-31 19:39:47]

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-08-31 21:31:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-31 21:34:23]

Description was changed from

==========
[layoutng] Allow testing NGBox without a LayoutObject

And use that to add a test for ng_block_layout_algorithm that tests layout
with children.

BUG=635619
R=eae@chromium.org,glebl@chromium.org,ikilpatrick@chromium.org
==========

to

==========
[layoutng] Allow testing NGBox without a LayoutObject

And use that to add a test for ng_block_layout_algorithm that tests layout
with children.

BUG=635619
R=eae@chromium.org,glebl@chromium.org,ikilpatrick@chromium.org

Committed: https://crrev.com/f2e5768ae0a7b3514005522adcfc93781852a0ec
Cr-Commit-Position: refs/heads/master@{#415762}
==========

[commit-bot: I haz the power, 2016-08-31 21:34:24]

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