[LayoutNG] Fix orthogonal writing mode child margin strut from being used in collapsing margins cal…

[LayoutNG] Fix orthogonal writing mode child margin strut from being used in collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

BUG=635619
Committed: https://crrev.com/0f6bd94a8898f85dbc05152065c55a5fffcdcccb
Cr-Commit-Position: refs/heads/master@{#426106}

[ikilpatrick, 2016-10-13 23:41:23]

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

[ikilpatrick, 2016-10-13 23:41:24]

the margin strut move changes the bit packing in a bad why, but unsure.

[Gleb Lanbin, 2016-10-13 23:59:51]

lgtm

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

https://codereview.chromium.org/2417113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment.cc:12: return writing_mode_
==
it's better to use IF instead of a long ternary operator here.

https://codereview.chromium.org/2417113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment.cc:15: ?
toNGPhysicalFragment(physical_fragment_)->MarginStrut()
toNGPhysicalFragment(physical_fragment_)->MarginStrut() is a bit wordy way to
access MarginStrut, should we keep it in the base instead ?

https://codereview.chromium.org/2417113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h (right):

https://codereview.chromium.org/2417113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:48: unsigned
margin_strut_writing_mode_ : 2;
move writing_mode to NGMarginStrut ?

[Gleb Lanbin, 2016-10-13 23:59:53]

lgtm

[ikilpatrick, 2016-10-14 16:13:03]

this fix is actually wrong, b/c IsNewFormattingContext isn't getting called, I'm
going to rework this slightly but changing IsNewFormattingContext use the
appropriate styles.

[cbiesinger, 2016-10-14 16:57:38]

Description was changed from

==========
[LayoutNG] Fix orthoganal writing mode child margin strut from being used in
collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

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

to

==========
[LayoutNG] Fix orthogonal writing mode child margin strut from being used in
collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

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

[eae, 2016-10-14 19:05:24]

On 2016/10/14 16:13:03, ikilpatrick wrote:
> this fix is actually wrong, b/c IsNewFormattingContext isn't getting called,
I'm
> going to rework this slightly but changing IsNewFormattingContext use the
> appropriate styles.

OK, thanks for the update

[ikilpatrick, 2016-10-16 00:33:09]

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

[commit-bot: I haz the power, 2016-10-16 00:33:13]

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-10-16 01:17:11]

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

[commit-bot: I haz the power, 2016-10-16 01:17:12]

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

[ikilpatrick, 2016-10-17 03:46:41]

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

[commit-bot: I haz the power, 2016-10-17 03:46:47]

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-10-17 04:49:40]

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

[commit-bot: I haz the power, 2016-10-17 04:49:42]

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_...)

[ikilpatrick, 2016-10-17 17:23:28]

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

[commit-bot: I haz the power, 2016-10-17 17:23:49]

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

[ikilpatrick, 2016-10-17 17:24:54]

PTAL :)

There is an open question as to if this flag should be on the constraint space,
or if we should set the parent's writing mode on the constraint space... (in
order to compute the IsNewFC) thoughts?

[eae, 2016-10-17 17:26:46]

Thanks, I'll take a look right after our meeting.

[eae, 2016-10-17 18:03:04]

LGTM

https://codereview.chromium.org/2417113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2417113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:412:
TEST_F(NGBlockLayoutAlgorithmTest, CollapsingMarginsCase6) {
Great test, thanks for adding it!

[commit-bot: I haz the power, 2016-10-17 18:44:07]

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

[commit-bot: I haz the power, 2016-10-17 18:44:08]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2016-10-18 22:54:09]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-10-18 22:54:10]

lgtm

lgtm, feel free to ignore a minor style comment.

https://codereview.chromium.org/2417113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2417113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:27: //
Returns true if a in-flow block-level child creates a new formatting context.
.nit if a function returns boolean then google3 code style recommends to start a
doc string with "whether". Example: Whether an in-flow block-level child ...

[Gleb Lanbin, 2016-10-18 22:54:11]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-10-18 22:54:11]

lgtm

lgtm

feel free to ignore a minor style comment.

[Gleb Lanbin, 2016-10-18 22:54:24]

The CQ bit was unchecked by glebl@chromium.org

[ikilpatrick, 2016-10-18 22:59:28]

https://codereview.chromium.org/2417113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2417113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:27: //
Returns true if a in-flow block-level child creates a new formatting context.
On 2016/10/18 22:54:10, glebl wrote:
> .nit if a function returns boolean then google3 code style recommends to start
a
> doc string with "whether". Example: Whether an in-flow block-level child ...

Done, thanks!

[ikilpatrick, 2016-10-18 22:59:59]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-10-18 23:00:00]

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/2417113002/#ps120001
(title: "address comments")

[commit-bot: I haz the power, 2016-10-18 23:00:43]

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-19 01:36:30]

Description was changed from

==========
[LayoutNG] Fix orthogonal writing mode child margin strut from being used in
collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

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

to

==========
[LayoutNG] Fix orthogonal writing mode child margin strut from being used in
collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

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

[commit-bot: I haz the power, 2016-10-19 01:36:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-21 13:05:25]

Description was changed from

==========
[LayoutNG] Fix orthogonal writing mode child margin strut from being used in
collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

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

to

==========
[LayoutNG] Fix orthogonal writing mode child margin strut from being used in
collapsing margins calculation.

Also changes where the margin strut is stored, which may be a bad idea?

BUG=635619

Committed: https://crrev.com/0f6bd94a8898f85dbc05152065c55a5fffcdcccb
Cr-Commit-Position: refs/heads/master@{#426106}
==========

[commit-bot: I haz the power, 2016-10-21 13:05:26]

Patchset 7 (id:??) landed as
https://crrev.com/0f6bd94a8898f85dbc05152065c55a5fffcdcccb
Cr-Commit-Position: refs/heads/master@{#426106}