CS of out-of-flow positioned objects should have is_new_fc == true

CS of out-of-flow positioned objects should have is_new_fc == true

List of changes:
1) IsNewFormattingContextForInFlowBlockLevelChild returns true for
   out-of-flow positioned child's style.
2) Add is_parent_new_fc_ to NGConstraintSpaceBuilder.
   We need need this bit to determine whether we need to pass exclusions
   to child's CS. Consider an example:
   <div id="container" style="width/height: 200px;">
     <float1><float-child></float1>
     <float2></float2>
   </div>
   float1 and float2 CSs will have available size == 200x200 and that
   size will be used to search for layout opportunities. So instead of
   passing arround the list of exclusions this patch introduces
   is_parent_new_fc that tells space_builder whether we need to pass
   exclusions to a child's CS.
3) Delete test lines that check NGLayoutOpportunityTree.
   That's because we can't access it anymore and we check its correctness
   in other places.


BUG=635619
Review-Url: https://codereview.chromium.org/2770483002
Cr-Commit-Position: refs/heads/master@{#459697}
Committed: https://chromium.googlesource.com/chromium/src/+/b91d302ec822f5a1900e2e0479d98741c0fb6d73

[Gleb Lanbin, 2017-03-21 23:24:47]

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

[Gleb Lanbin, 2017-03-21 23:24:57]

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

[commit-bot: I haz the power, 2017-03-21 23:25: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, 2017-03-21 23:29:07]

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

[commit-bot: I haz the power, 2017-03-21 23:29:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ikilpatrick, 2017-03-23 16:39:59]

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:180:
is_parent_new_fc_ ? std::make_shared<NGExclusions>() : exclusions_);
yeah i'm not sure that I understand this, exclusions are only relevant to a
block formatting context, they don't affect layout of a child's BFC if it
establishes one... e.g. even if you have an inner float which is large, it wont
avoid other floats in the parent BFC; for example:

https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...

I think the issues is that the layout opportunity iterator shouldn't be using
the available inline size? I.e. it should be using the resolved inline size of
the fragment?

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_space_utils.cc:22: bool
isOutFlowPositioned(const EPosition& position) {
IsOutOfFlowPositioned

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_space_utils.h:20: CORE_EXPORT bool
IsNewFormattingContextForInFlowBlockLevelChild(
rename now to:

IsNewFormattingContextForBlockLevelChild

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_space_utils_test.cc:26:
TEST_F(NGSpaceUtilsTest, NewFormattingContextForOutOfFlowChild) {
thanks for this :)

[Gleb Lanbin, 2017-03-24 20:10:05]

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:180:
is_parent_new_fc_ ? std::make_shared<NGExclusions>() : exclusions_);
On 2017/03/23 16:39:59, ikilpatrick wrote:
> yeah i'm not sure that I understand this, exclusions are only relevant to a
> block formatting context, they don't affect layout of a child's BFC if it
> establishes one... e.g. even if you have an inner float which is large, it
wont
> avoid other floats in the parent BFC; for example:
> 
>
https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...
> 
> I think the issues is that the layout opportunity iterator shouldn't be using
> the available inline size? I.e. it should be using the resolved inline size of
> the fragment? 

updated based on the offline discussion.

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_space_utils.cc:22: bool
isOutFlowPositioned(const EPosition& position) {
On 2017/03/23 16:39:59, ikilpatrick wrote:
> IsOutOfFlowPositioned

Done.

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_space_utils.h:20: CORE_EXPORT bool
IsNewFormattingContextForInFlowBlockLevelChild(
On 2017/03/23 16:39:59, ikilpatrick wrote:
> rename now to:
> 
> IsNewFormattingContextForBlockLevelChild

Done.

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

https://codereview.chromium.org/2770483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_space_utils_test.cc:26:
TEST_F(NGSpaceUtilsTest, NewFormattingContextForOutOfFlowChild) {
On 2017/03/23 16:39:59, ikilpatrick wrote:
> thanks for this :)

Acknowledged.

[ikilpatrick, 2017-03-24 21:17:34]

ok, lgtm; I'd prefer that we add this information to NGFloatingObject and
perform the adjustment, but ok with leaving TODOs where we need to fix this.

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

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:197:
curr_bfc_offset_ = {};
can you add a todo here that this should be a
DCHECK_EQ(ConstraintSpace().BfcOffset(), {}); ?

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(left):

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:179:
NGLogicalOffset bfc_offset = is_new_fc_ ? NGLogicalOffset() : bfc_offset_;
can you add a todo here as well to add this back in?

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

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floats_utils.cc:48: const
NGLogicalOffset& origin_point,
so what i was imagining is that the caller to this (PositionFloat) would adjust
this argument for the inline_size, and that inline_size adjustment would live on
NGFloatingObject.

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floats_utils.cc:146:
new_parent_space, float_fragment, origin_point, floating_object->margins,
from above: e.g.

NGLogicalOffset adjusted_point({origin_point.inline_offset +
floating_object->inline_offset_adjustment, origin_point.block_offset});

[Gleb Lanbin, 2017-03-24 21:49:36]

thanks for the review

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

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:197:
curr_bfc_offset_ = {};
On 2017/03/24 21:17:34, ikilpatrick wrote:
> can you add a todo here that this should be a
> DCHECK_EQ(ConstraintSpace().BfcOffset(), {}); ?

Done.

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(left):

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:179:
NGLogicalOffset bfc_offset = is_new_fc_ ? NGLogicalOffset() : bfc_offset_;
On 2017/03/24 21:17:34, ikilpatrick wrote:
> can you add a todo here as well to add this back in?

Done.

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

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floats_utils.cc:48: const
NGLogicalOffset& origin_point,
On 2017/03/24 21:17:34, ikilpatrick wrote:
> so what i was imagining is that the caller to this (PositionFloat) would
adjust
> this argument for the inline_size, and that inline_size adjustment would live
on
> NGFloatingObject.

Acknowledged.

https://codereview.chromium.org/2770483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floats_utils.cc:146:
new_parent_space, float_fragment, origin_point, floating_object->margins,
On 2017/03/24 21:17:34, ikilpatrick wrote:
> from above: e.g.
> 
> NGLogicalOffset adjusted_point({origin_point.inline_offset +
> floating_object->inline_offset_adjustment, origin_point.block_offset});

I find this additional adjustment a bit confusing. We already store float's
constraint space at FloatingObject that has BfcOffset that we can use to compute
our origin_point. Lets wait for the fragmentation support for floats which may
remove "space" from FloatingObject and make it use inline_offset_adjustment
instead.

[Gleb Lanbin, 2017-03-24 21:52:22]

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

[commit-bot: I haz the power, 2017-03-24 21:53:04]

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

[Gleb Lanbin, 2017-03-24 22:27:06]

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

[commit-bot: I haz the power, 2017-03-24 22:27:44]

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, 2017-03-24 23:35:02]

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

[commit-bot: I haz the power, 2017-03-24 23:35:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Gleb Lanbin, 2017-03-24 23:37:24]

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

[commit-bot: I haz the power, 2017-03-24 23:37:54]

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, 2017-03-25 00:02:47]

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

[commit-bot: I haz the power, 2017-03-25 00:02:48]

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, 2017-03-25 02:23:56]

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

[commit-bot: I haz the power, 2017-03-25 02:24:06]

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, 2017-03-25 04:28:59]

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

[commit-bot: I haz the power, 2017-03-25 04:29:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gleb Lanbin, 2017-03-25 13:46:41]

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

[commit-bot: I haz the power, 2017-03-25 13:46:52]

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, 2017-03-25 17:44:35]

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

[commit-bot: I haz the power, 2017-03-25 17:44:36]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-03-25 19:12:33]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-03-25 19:12:33]

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/2770483002/#ps100001
(title: "update TestExpectations")

[commit-bot: I haz the power, 2017-03-25 19:12:39]

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, 2017-03-25 19:17:30]

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

[commit-bot: I haz the power, 2017-03-25 19:17:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Gleb Lanbin, 2017-03-25 19:54:58]

Patchset #4 (id:60001) has been deleted

[Gleb Lanbin, 2017-03-25 19:55:16]

Patchset #4 (id:80001) has been deleted

[Gleb Lanbin, 2017-03-25 19:56:10]

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

[Gleb Lanbin, 2017-03-25 19:56:11]

Christian, I need your approval for third_party/WebKit/Source/core/BUILD.gn

thanks in advance.

[cbiesinger, 2017-03-26 22:24:57]

rs-lgtm

[Gleb Lanbin, 2017-03-26 22:25:28]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2017-03-26 22:25:42]

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, 2017-03-27 00:12:12]

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

[commit-bot: I haz the power, 2017-03-27 00:12:13]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gleb Lanbin, 2017-03-27 00:13:12]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2017-03-27 00:13:21]

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, 2017-03-27 01:16:47]

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

[commit-bot: I haz the power, 2017-03-27 01:16:49]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Gleb Lanbin, 2017-03-27 01:19:31]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2017-03-27 01:19: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, 2017-03-27 03:27:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490577571453020,
"parent_rev": "6625b2f764ae4efb0637215660500ba721b8a43d", "commit_rev":
"b91d302ec822f5a1900e2e0479d98741c0fb6d73"}

[commit-bot: I haz the power, 2017-03-27 03:28:15]

Description was changed from

==========
CS of out-of-flow positioned objects should have is_new_fc == true

List of changes:
1) IsNewFormattingContextForInFlowBlockLevelChild returns true for
   out-of-flow positioned child's style.
2) Add is_parent_new_fc_ to NGConstraintSpaceBuilder.
   We need need this bit to determine whether we need to pass exclusions
   to child's CS. Consider an example:
   <div id="container" style="width/height: 200px;">
     <float1><float-child></float1>
     <float2></float2>
   </div>
   float1 and float2 CSs will have available size == 200x200 and that
   size will be used to search for layout opportunities. So instead of
   passing arround the list of exclusions this patch introduces
   is_parent_new_fc that tells space_builder whether we need to pass
   exclusions to a child's CS.
3) Delete test lines that check NGLayoutOpportunityTree.
   That's because we can't access it anymore and we check its correctness
   in other places.


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

to

==========
CS of out-of-flow positioned objects should have is_new_fc == true

List of changes:
1) IsNewFormattingContextForInFlowBlockLevelChild returns true for
   out-of-flow positioned child's style.
2) Add is_parent_new_fc_ to NGConstraintSpaceBuilder.
   We need need this bit to determine whether we need to pass exclusions
   to child's CS. Consider an example:
   <div id="container" style="width/height: 200px;">
     <float1><float-child></float1>
     <float2></float2>
   </div>
   float1 and float2 CSs will have available size == 200x200 and that
   size will be used to search for layout opportunities. So instead of
   passing arround the list of exclusions this patch introduces
   is_parent_new_fc that tells space_builder whether we need to pass
   exclusions to a child's CS.
3) Delete test lines that check NGLayoutOpportunityTree.
   That's because we can't access it anymore and we check its correctness
   in other places.


BUG=635619

Review-Url: https://codereview.chromium.org/2770483002
Cr-Commit-Position: refs/heads/master@{#459697}
Committed:
https://chromium.googlesource.com/chromium/src/+/b91d302ec822f5a1900e2e0479d9...
==========

[commit-bot: I haz the power, 2017-03-27 03:28:17]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b91d302ec822f5a1900e2e0479d9...