Introduce NGFloatingObject

Introduce NGFloatingObject

NGFloatingObject holds all information needed to position floats in 
old/new layout tree.

If a float can not determine its position in space (for example because 
of margin collapsing case) it is added to the fragment's 
UnpositionedFloats list. When we reach the fragment that can resolve 
its position in space all pending floats will be positioned and added 
to the PositionedFloats list. PositionedFloats list is used to 
create/place  FloatingObject in old layout tree.

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

[Gleb Lanbin, 2017-01-19 19:28:26]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2017-01-19 19:30:56]

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

[mstensho (USE GERRIT), 2017-01-19 19:37:52]

Could you wrap the text in the description at max 80 chars or so per line,
please?

[Gleb Lanbin, 2017-01-19 19:51:03]

Description was changed from

==========
Introduce NGFloatingObject

NGFloatingObject holds all information needed to position floats in old/new
layout tree.

If a float can not determine its position in space (for example because of
margin collapsing case) it is added to the fragment's UnpositionedFloats list.
When we reach the fragment that can resolve its position in space all pending
floats will be positioned and added to the PositionedFloats list.
PositionedFloats list is used to create/place  FloatingObject in old layout
tree.

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

to

==========
Introduce NGFloatingObject

NGFloatingObject holds all information needed to position floats in 
old/new layout tree.

If a float can not determine its position in space (for example because 
of margin collapsing case) it is added to the fragment's 
UnpositionedFloats list. When we reach the fragment that can resolve 
its position in space all pending floats will be positioned and added 
to the PositionedFloats list. PositionedFloats list is used to 
create/place  FloatingObject in old layout tree.

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

[Gleb Lanbin, 2017-01-19 19:51:12]

On 2017/01/19 19:37:52, mstensho wrote:
> Could you wrap the text in the description at max 80 chars or so per line,
> please?

done

[ikilpatrick, 2017-01-20 00:03:17]

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

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:90: void
FloatPositionUpdated(LayoutObject* parent);
maybe additional comment here or within the function.

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

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:20: struct
CORE_EXPORT NGFloatingObject
NGFloatingNode a better name?

(assuming a future where this is pre-layout).

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:33:
Member<NGPhysicalFragment> fragment;
Maybe add a note we have to layout after we know the fragmentation line to
handle multicolumn in future world where we paint fragments. This isn't
important for today but will be in the future.

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:34:
Member<NGConstraintSpace> space;
const NGConstraintSpace

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:35:
Member<NGBlockNode> node;
TODO(ikilpatrick): remove this.

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

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:79: // can
determine its position in space.
... this may occurs in situation X.

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:84: // List of
positioned float that need to be copied to the old layout tree.
maybe... TODO remove this once we change painting code to handle floats
differently?

[Gleb Lanbin, 2017-01-20 00:45:28]

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

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:90: void
FloatPositionUpdated(LayoutObject* parent);
On 2017/01/20 00:03:17, ikilpatrick wrote:
> maybe additional comment here or within the function.

Done.

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

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:20: struct
CORE_EXPORT NGFloatingObject
On 2017/01/20 00:03:17, ikilpatrick wrote:
> NGFloatingNode a better name?
> 
> (assuming a future where this is pre-layout). 

I wanted to keep this name to be consistent with legacy FloatingObject.

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:33:
Member<NGPhysicalFragment> fragment;
On 2017/01/20 00:03:17, ikilpatrick wrote:
> Maybe add a note we have to layout after we know the fragmentation line to
> handle multicolumn in future world where we paint fragments. This isn't
> important for today but will be in the future.

Acknowledged.

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:34:
Member<NGConstraintSpace> space;
On 2017/01/20 00:03:17, ikilpatrick wrote:
> const NGConstraintSpace

Done.

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:35:
Member<NGBlockNode> node;
On 2017/01/20 00:03:17, ikilpatrick wrote:
> TODO(ikilpatrick): remove this.

Done.

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

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:79: // can
determine its position in space.
On 2017/01/20 00:03:17, ikilpatrick wrote:
> ... this may occurs in situation X.

Done.

https://codereview.chromium.org/2642823008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:84: // List of
positioned float that need to be copied to the old layout tree.
On 2017/01/20 00:03:17, ikilpatrick wrote:
> maybe... TODO remove this once we change painting code to handle floats
> differently?

Done.

[ikilpatrick, 2017-01-20 18:11:14]

lgtm

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_floating_object.h (right):

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:36:
Member<NGBlockNode> node;
I think this should be a WeakMember?

NGBlockNode -> Fragment -> NGFloatingObject -> NGBlockNode?

Also i realize from our discussion yesterday that if we end up doing this
post-layout that todo is irrelevant.

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:207:
visitor->trace(break_token_);
.nit ordering if changed in header.

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right):

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:152:
Member<NGBreakToken> break_token_;
.nit keeping ordering consistent bt fragment/builder break_token_ should go
last.

[Gleb Lanbin, 2017-01-20 21:21:59]

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_floating_object.h (right):

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_floating_object.h:36:
Member<NGBlockNode> node;
On 2017/01/20 18:11:14, ikilpatrick wrote:
> I think this should be a WeakMember?
> 
> NGBlockNode -> Fragment -> NGFloatingObject -> NGBlockNode?
> 
> Also i realize from our discussion yesterday that if we end up doing this
> post-layout that todo is irrelevant.

according to Oilpan documentation it's better to use Member here because
- use of WeakMember<T> incurs some overhead in garbage collector's performance
- weak members are not necessary at all, because reference cycles with members
are allowed
- WeakMember<T> should be used only if the owner of a weak member can outlive
the pointed object.

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:207:
visitor->trace(break_token_);
On 2017/01/20 18:11:14, ikilpatrick wrote:
> .nit ordering if changed in header.

Done.

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right):

https://codereview.chromium.org/2642823008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:152:
Member<NGBreakToken> break_token_;
On 2017/01/20 18:11:14, ikilpatrick wrote:
> .nit keeping ordering consistent bt fragment/builder break_token_ should go
> last.

Done.

[Gleb Lanbin, 2017-01-20 21:22:07]

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

[commit-bot: I haz the power, 2017-01-20 21:22:44]

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

[ikilpatrick, 2017-01-20 21:41:33]

thanks, lgtm

[commit-bot: I haz the power, 2017-01-20 22:37:45]

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

[commit-bot: I haz the power, 2017-01-20 22:37:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Gleb Lanbin, 2017-01-23 18:26:27]

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

[commit-bot: I haz the power, 2017-01-23 18:27:27]

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-01-23 20:53:56]

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

[commit-bot: I haz the power, 2017-01-23 20:53:57]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-01-23 21:10:35]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-01-23 21:10:35]

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/2642823008/#ps80001
(title: "Update TestExpectations")

[commit-bot: I haz the power, 2017-01-23 21:11: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, 2017-01-23 21:16:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485205835223470,
"parent_rev": "1bb1c78bef7565222274dd05ba49cfbbed1978ec", "commit_rev":
"38e16a89fed5f85376cbb78ade464cc4960d433e"}

[commit-bot: I haz the power, 2017-01-23 21:17:18]

Description was changed from

==========
Introduce NGFloatingObject

NGFloatingObject holds all information needed to position floats in 
old/new layout tree.

If a float can not determine its position in space (for example because 
of margin collapsing case) it is added to the fragment's 
UnpositionedFloats list. When we reach the fragment that can resolve 
its position in space all pending floats will be positioned and added 
to the PositionedFloats list. PositionedFloats list is used to 
create/place  FloatingObject in old layout tree.

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

to

==========
Introduce NGFloatingObject

NGFloatingObject holds all information needed to position floats in 
old/new layout tree.

If a float can not determine its position in space (for example because 
of margin collapsing case) it is added to the fragment's 
UnpositionedFloats list. When we reach the fragment that can resolve 
its position in space all pending floats will be positioned and added 
to the PositionedFloats list. PositionedFloats list is used to 
create/place  FloatingObject in old layout tree.

BUG=635619

Review-Url: https://codereview.chromium.org/2642823008
Cr-Commit-Position: refs/heads/master@{#445488}
Committed:
https://chromium.googlesource.com/chromium/src/+/38e16a89fed5f85376cbb78ade46...
==========

[commit-bot: I haz the power, 2017-01-23 21:17:20]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/38e16a89fed5f85376cbb78ade46...