Implement collection of out-of-flow descendants (issue 2540653003 by atotic@chromium.org)

ato...@chromium.org

unread,

Nov 29, 2016, 3:14:09 PM11/29/16

to ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Reviewers: ikilpatrick
CL: https://codereview.chromium.org/2540653003/

Description:
Implement collection of out-of-flow descendants

Out-of-flow descendants need to know what their static position would
have been. This is because abspos spec defaults to static position if
TLBL are not specified.

Oof descendant's dimensions are not known during propagation, only
it's logical top/left offset. The offset is propagated as NGCorner,
which is a PhysicalOffset + vertex location (topLeft, bottomLeft, etc)

Next step will be positioning of oof descendants.

BUG=635619

Affected files (+140, -11 lines):
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
M third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h
M third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc
M third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h
M third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.cc
M third_party/WebKit/Source/core/layout/ng/ng_units.h
M third_party/WebKit/Source/core/layout/ng/ng_units.cc

ato...@chromium.org

unread,

Dec 1, 2016, 8:08:46 PM12/1/16

to ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

On 2016/11/29 at 20:14:08, atotic2 wrote:
>

ptal

https://codereview.chromium.org/2540653003/

ikilp...@chromium.org

unread,

Dec 2, 2016, 12:47:06 PM12/2/16

to ato...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
File
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(left):

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#oldcode200
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:200:

remove from this patch.

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode166
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:166:

remove from this patch.

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode52
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:52: //
Collect child's out of flow descendants
.nit add period to end.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode54
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:54:
NGPhysicalFragmentBase::kFragmentBox) {
this can be fixed here or TODO+another patch, but i think we actually
want out_of_flow_descendants to live on fragment_base, i.e.

https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Aspan%20%7B%20border%3A%201px%20solid%20green%3B%20%7D%0A%23a%20%7B%20position%3A%20absolute%3B%20top%3A%2020px%3B%20%7D%0A%3C%2Fstyle%3E%0A%3Cspan%3Etext%20%D9%87%3Cspan%20id%3D%22a%22%3Ebar%3C%2Fspan%3E%D9%8A%20text%3C%2Fspan%3E

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode61
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:61:
out_of_flow_descendant_candidates_.add(block_node);
can we do:
s/block_/oof_/ here?

block is a little overloaded, oof_ prefix would be better.

(on block_offsets, block_index, block_node, block_corner).

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode81
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:81:
Vector<NGCorner>& descendant_corners) {
change ref to pointer as out param:
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode83
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:83:
descendant_corners.clear();
I would do a:
DCHECK(descendants->isEmpty());
DCHECK(descendant_corners->isEmpty());

instead.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode86
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:86:
DCHECK_GE(size_.block_size, LayoutUnit(0));
just LayoutUnit() for above lines I think.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode90
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:90: for
(auto& block_node : out_of_flow_descendant_candidates_) {
s/block_node/oof_node/

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode99
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:99:
child_offset + oof_offset.descendant_corner.offset;
This is great, a lot simpler :)

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode31
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:31: //
Builder is not dumb when handling out of flow descendants.
Maybe:

The builder isn't "simple" when handling out of flow (OOF) descendants.
It will collect OOF descendants from:
- AddChild (will add all descendants from the appended fragment).
- AddOutOfFlowDescendantCandidate (directs adds a descendant).

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode34
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:34: //
if (child->position == (Absolute or Fixed)
missing closing ')'

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode37
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:37: //
child->Layout()
i'd write:
fragment = child->Layout()

instead. (make clear that not adding input node).

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode41
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:41: //
while (builder->GetOutOfFlowDescendantCandidates(oof_candidates)
while loop bad?

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode50
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:50: // }
There are two cases:
- The descendant came from a child fragment.
- The descendant is a direct child of the current fragment.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode51
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:51:
NGFragmentBuilder& AddOutOfFlowCandidateChild(NGBlockNode*,
NGLogicalOffset);
AddOutOfFlowDescendantCandidate for consistency.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode70
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:70: //
Descendant offset information
Maybe:

Descendant offset information. We need information from two places to
calculate the NGCorner of the descendant correctly.
- The inner physical offset (child_offset). This is set if the
descendant came from a child fragment. It is the offset within that
fragment.
- The outer logical offset (descendant_corner). This is set in both
cases (see above). It is the offset to the descendant, or the child
fragment as appropriate. We store the outer logical offset here
separately as it is impossible to convert to physical without the
fragment size.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode89
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:89:
Vector<OutOfFlowOffset> oof_candidate_offsets_;
s/oof/out_of_flow/

to be consistent with rest of variable naming.

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode100
third_party/WebKit/Source/core/layout/ng/ng_units.h:100:
NGPhysicalOffset operator+(const NGPhysicalOffset& other) const {
also define:
NGPhysicalOffset& operator+=(const NGPhysicalOffset other) const;

also probably move to .cc file to be consistent with NGLogicalOffset.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode278
third_party/WebKit/Source/core/layout/ng/ng_units.h:278: // Represents
rectangle's physical corner.
Maybe:

This struct is used to represent the static position of an out of flow
descendant.

(similar to NGMarginStruct, specifies which part of layout it's used
for).

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode282
third_party/WebKit/Source/core/layout/ng/ng_units.h:282: Type type; //
Logical corner that corresponds to physical top left.
remove this comment?

https://codereview.chromium.org/2540653003/

ato...@chromium.org

unread,

Dec 2, 2016, 2:55:16 PM12/2/16

to ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CR fixes done, ptal

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
File
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(left):

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#oldcode200
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:200:

On 2016/12/02 at 17:47:05, ikilpatrick wrote:
> remove from this patch.

done

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode166
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:166:

On 2016/12/02 at 17:47:05, ikilpatrick wrote:
> remove from this patch.

done

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode52
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:52: //
Collect child's out of flow descendants

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> .nit add period to end.

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode54
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:54:
NGPhysicalFragmentBase::kFragmentBox) {

On 2016/12/02 at 17:47:05, ikilpatrick wrote:
> this can be fixed here or TODO+another patch, but i think we actually
want out_of_flow_descendants to live on fragment_base, i.e.
>
>
https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Aspan%20%7B%20border%3A%201px%20solid%20green%3B%20%7D%0A%23a%20%7B%20position%3A%20absolute%3B%20top%3A%2020px%3B%20%7D%0A%3C%2Fstyle%3E%0A%3Cspan%3Etext%20%D9%87%3Cspan%20id%3D%22a%22%3Ebar%3C%2Fspan%3E%D9%8A%20text%3C%2Fspan%3E

To rephrase:

All fragments can carry oof descendants. Therefore, oof descendants
should move from NGPhysicalFragment to NGPhysicalFragmentBase

Added TODO, this will be my next patch.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode61
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:61:
out_of_flow_descendant_candidates_.add(block_node);

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> can we do:
> s/block_/oof_/ here?
>
> block is a little overloaded, oof_ prefix would be better.
>
> (on block_offsets, block_index, block_node, block_corner).

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode81
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:81:
Vector<NGCorner>& descendant_corners) {

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> change ref to pointer as out param:
>
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode83
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:83:
descendant_corners.clear();

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> I would do a:
> DCHECK(descendants->isEmpty());
> DCHECK(descendant_corners->isEmpty());
>
> instead.

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode86
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:86:
DCHECK_GE(size_.block_size, LayoutUnit(0));

On 2016/12/02 at 17:47:05, ikilpatrick wrote:
> just LayoutUnit() for above lines I think.

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode90
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:90: for
(auto& block_node : out_of_flow_descendant_candidates_) {

On 2016/12/02 at 17:47:05, ikilpatrick wrote:
> s/block_node/oof_node/

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode99
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:99:
child_offset + oof_offset.descendant_corner.offset;

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> This is great, a lot simpler :)

yay

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode31
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:31: //
Builder is not dumb when handling out of flow descendants.

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> Maybe:
>
> The builder isn't "simple" when handling out of flow (OOF)
descendants. It will collect OOF descendants from:
> - AddChild (will add all descendants from the appended fragment).
> - AddOutOfFlowDescendantCandidate (directs adds a descendant).

entire comment revised for clarity

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode34
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:34: //
if (child->position == (Absolute or Fixed)

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> missing closing ')'

entire comment revised for clarity

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode37
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:37: //
child->Layout()

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> i'd write:
> fragment = child->Layout()
>
> instead. (make clear that not adding input node).

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode41
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:41: //
while (builder->GetOutOfFlowDescendantCandidates(oof_candidates)

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> while loop bad?

entire comment revised for clarity

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode50
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:50: // }

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> There are two cases:
> - The descendant came from a child fragment.
> - The descendant is a direct child of the current fragment.

entire comment revised for clarity

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode51
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:51:
NGFragmentBuilder& AddOutOfFlowCandidateChild(NGBlockNode*,
NGLogicalOffset);

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> AddOutOfFlowDescendantCandidate for consistency.

It can only be a child, so will not rename to Descendant.

Did rename to AddOutOfFlowChildCandidate for consistency with
AddOutOfFlowDescendant, AddChild

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode70
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:70: //
Descendant offset information

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> Maybe:
>
> Descendant offset information. We need information from two places to
calculate the NGCorner of the descendant correctly.
> - The inner physical offset (child_offset). This is set if the
descendant came from a child fragment. It is the offset within that
fragment.
> - The outer logical offset (descendant_corner). This is set in both
cases (see above). It is the offset to the descendant, or the child
fragment as appropriate. We store the outer logical offset here
separately as it is impossible to convert to physical without the
fragment size.

Renamed OutOfFlowOffset to OutOfFlowPlacement for clarity, and wrote a
lengthy comment.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode89
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:89:
Vector<OutOfFlowOffset> oof_candidate_offsets_;

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> s/oof/out_of_flow/
>
> to be consistent with rest of variable naming.

Will do.

Note: this is an example where "explicit variable naming" and "80 cols"
guidelines produce less readable code.
out_of_flow_of_candidate_placements_ is 34 characters. This leaves only
47 characters left for other code, causing
lots and
lots of
line
wrapping

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode100
third_party/WebKit/Source/core/layout/ng/ng_units.h:100:
NGPhysicalOffset operator+(const NGPhysicalOffset& other) const {

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> also define:
> NGPhysicalOffset& operator+=(const NGPhysicalOffset other) const;
>
> also probably move to .cc file to be consistent with NGLogicalOffset.

done.

+= is not used yet, our current policy seems to be "define some
operators, but not other". Also, some are defined inline (NGBoxStrut),
some not.

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode278
third_party/WebKit/Source/core/layout/ng/ng_units.h:278: // Represents
rectangle's physical corner.

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> Maybe:
>
> This struct is used to represent the static position of an out of flow
descendant.
>
> (similar to NGMarginStruct, specifies which part of layout it's used
for).

done

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode282
third_party/WebKit/Source/core/layout/ng/ng_units.h:282: Type type; //
Logical corner that corresponds to physical top left.

On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> remove this comment?

Not done. I will not know what type is a year from now if this comment
is removed.

https://codereview.chromium.org/2540653003/

ikilp...@chromium.org

unread,

Dec 2, 2016, 4:06:13 PM12/2/16

to ato...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode100
third_party/WebKit/Source/core/layout/ng/ng_units.h:100:
NGPhysicalOffset operator+(const NGPhysicalOffset& other) const {

On 2016/12/02 19:55:16, atotic2 wrote:
> On 2016/12/02 at 17:47:06, ikilpatrick wrote:
> > also define:
> > NGPhysicalOffset& operator+=(const NGPhysicalOffset other) const;
> >
> > also probably move to .cc file to be consistent with
NGLogicalOffset.
>
> done.
>
> += is not used yet, our current policy seems to be "define some
operators, but
> not other". Also, some are defined inline (NGBoxStrut), some not.

Thanks - this comes from:
https://google.github.io/styleguide/cppguide.html#Operator_Overloading
"If you define an operator, also define any related operators that make
sense, and make sure they are defined consistently."

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

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode60
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:60:
Vector<NGCorner> oof_offsets = physical_child->OutOfFlowOffsets();
This is inducing a copy?

Vector<NGCorner>& oof_offsets?

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

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode37
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:37: //
Part 1: layout algorithm positions positioned children.
s/positioned/in-flow ?

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode38
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:38: //
out-of-flow children, and out-of-flow descendants of positioned childen
.... and out-of-flow descendants of fragments

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode55
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:55: //
canditate = oof_candidates.shift
.nit s/shift/shift()/

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode84
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:84: //
Generated fragment must compute NGCorner for all out-of-flow
descendants.
The generated fragment ...

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode85
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:85: //
NGCorner gets derived from:
The resulting NGCorner? maybe?

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode86
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:86: // 1.
Offset of fragment's child wrt fragment.
The logical offset of the child within this fragment. Either:
a) The offset of the descendant's fragment.
b) The offset of the child descendant

? maybe?

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

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h#newcode37
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:37:
const Vector<NGCorner>& OutOfFlowOffsets() const {
offsets isn't a good name now, can you fix now or in a followup patch?

e.g.
OutOfFlowStaticPositions

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_units.h
File third_party/WebKit/Source/core/layout/ng/ng_units.h (right):

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_units.h#newcode278
third_party/WebKit/Source/core/layout/ng/ng_units.h:278: struct
CORE_EXPORT NGCorner {
I half wonder if this should be named NGStaticPosition, but can think
about it later.
(no action required).

https://codereview.chromium.org/2540653003/

ato...@chromium.org

unread,

Dec 2, 2016, 5:19:38 PM12/2/16

to ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Renamed NGCorner to NGStaticPosition, etc. ptal

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

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode60
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:60:
Vector<NGCorner> oof_offsets = physical_child->OutOfFlowOffsets();

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> This is inducing a copy?
>
> Vector<NGCorner>& oof_offsets?

done

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

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode37
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:37: //
Part 1: layout algorithm positions positioned children.

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> s/positioned/in-flow ?

done

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode38
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:38: //
out-of-flow children, and out-of-flow descendants of positioned childen

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> .... and out-of-flow descendants of fragments

done

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode55
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:55: //
canditate = oof_candidates.shift

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> .nit s/shift/shift()/

done

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode84
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:84: //
Generated fragment must compute NGCorner for all out-of-flow
descendants.

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> The generated fragment ...

done

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode85
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:85: //
NGCorner gets derived from:

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> The resulting NGCorner? maybe?

done

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode86
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:86: // 1.
Offset of fragment's child wrt fragment.

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> The logical offset of the child within this fragment. Either:
> a) The offset of the descendant's fragment.
> b) The offset of the child descendant
>
> ? maybe?

fixed

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

https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h#newcode37
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:37:
const Vector<NGCorner>& OutOfFlowOffsets() const {

On 2016/12/02 at 21:06:13, ikilpatrick wrote:
> offsets isn't a good name now, can you fix now or in a followup patch?
>
> e.g.
> OutOfFlowStaticPositions

done

https://codereview.chromium.org/2540653003/

ikilp...@chromium.org

unread,

Dec 2, 2016, 5:23:43 PM12/2/16

to ato...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

lgtm

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h#newcode25
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:25:
Vector<NGStaticPosition> out_of_flow_positions,
this should be a Vector<NGStaticPosition>& out_of_flow_positions

but can fix later.

https://codereview.chromium.org/2540653003/

cbies...@chromium.org

unread,

Dec 2, 2016, 5:37:04 PM12/2/16

to ato...@chromium.org, ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

lgtm

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode84
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:84: void
NGFragmentBuilder::GetOutOfFlowDescendantCandidates(
please rename to GetAndClear..., so it is more obvious that there is a
side effect here.

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode55
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:55: //
canditate = oof_candidates.shift()
typo: candidate

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode66
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:66: void
GetOutOfFlowDescendantCandidates(WeakBoxList*,
Maybe better to return a vector of pairs (or, a struct with the two
values)?

I suppose the point is that you want a weak list for the boxes, but you
could just define your own typedef for that

as a side note, I just noticed that this is a hash set. I'm not sure
that's a good idea, doesn't it have to be ordered for correct painting
order?

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode93
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:93: // 1.
A descendant itself. In this case, descendant position is (0,0).
descendant -> direct child, I think?

https://codereview.chromium.org/2540653003/

ato...@chromium.org

unread,

Dec 2, 2016, 6:08:44 PM12/2/16

to cbies...@chromium.org, ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CR fixes are in

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode84
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:84: void
NGFragmentBuilder::GetOutOfFlowDescendantCandidates(

On 2016/12/02 at 22:37:03, cbiesinger wrote:
> please rename to GetAndClear..., so it is more obvious that there is a
side effect here.

done. Good catch, this was bugging me too. Now I know that GetAndClear
is the standard.

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode55
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:55: //
canditate = oof_candidates.shift()

On 2016/12/02 at 22:37:03, cbiesinger wrote:
> typo: candidate

done

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode66
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:66: void
GetOutOfFlowDescendantCandidates(WeakBoxList*,

On 2016/12/02 at 22:37:03, cbiesinger wrote:
> Maybe better to return a vector of pairs (or, a struct with the two
values)?
>
> I suppose the point is that you want a weak list for the boxes, but
you could just define your own typedef for that

I also dislike objects being split between two collections, it makes
traversal awkward. Oilpan forces this separation right now, because
WeakMembers cannot be a member of a struct stored inside a collection.
When boxes move away from Oilpan, I'll redo all of these as proper
structs.

> as a side note, I just noticed that this is a hash set. I'm not sure
that's a good idea, doesn't it have to be ordered for correct painting
order?

Not a hash set, a HeapLinkedHashSet. This is the only ordered GC
collection that will accept WeakMembers.

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h#newcode93
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:93: // 1.
A descendant itself. In this case, descendant position is (0,0).

On 2016/12/02 at 22:37:03, cbiesinger wrote:
> descendant -> direct child, I think?

It does. The comment repeats this information because it is a bit
confusing. Here we have two objects: child, and descendant, and
sometimes child is also the descendant, and sometimes descendant is a
descendant of a child.

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h#newcode25
third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h:25:
Vector<NGStaticPosition> out_of_flow_positions,

On 2016/12/02 at 22:23:43, ikilpatrick wrote:
> this should be a Vector<NGStaticPosition>& out_of_flow_positions
>
> but can fix later.

done

https://codereview.chromium.org/2540653003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 6:09:40 PM12/2/16

to ato...@chromium.org, cbies...@chromium.org, ikilp...@chromium.org, commi...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2540653003/100001

https://codereview.chromium.org/2540653003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 6:12:20 PM12/2/16

to ato...@chromium.org, cbies...@chromium.org, ikilp...@chromium.org, commi...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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/116429)
ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/117621)
mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/347219)

https://codereview.chromium.org/2540653003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 6:53:54 PM12/2/16

to ato...@chromium.org, cbies...@chromium.org, ikilp...@chromium.org, commi...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2540653003/120001

https://codereview.chromium.org/2540653003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 9:52:12 PM12/2/16

to ato...@chromium.org, cbies...@chromium.org, ikilp...@chromium.org, commi...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Committed patchset #7 (id:120001)

https://codereview.chromium.org/2540653003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 9:54:26 PM12/2/16

to ato...@chromium.org, cbies...@chromium.org, ikilp...@chromium.org, commi...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, cbies...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2540653003/

cbies...@chromium.org

unread,

Dec 4, 2016, 10:32:02 AM12/4/16

to ato...@chromium.org, ikilp...@chromium.org, atotic+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, glebl+...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, ojan+...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode84
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:84: void
NGFragmentBuilder::GetOutOfFlowDescendantCandidates(

On 2016/12/02 23:08:44, atotic2 wrote:
> On 2016/12/02 at 22:37:03, cbiesinger wrote:
> > please rename to GetAndClear..., so it is more obvious that there is
a side
> effect here.
>
> done. Good catch, this was bugging me too. Now I know that GetAndClear
is the
> standard.

I mean, I don't know about "standard" but it seems reasonably common in
Chrome code.

Thanks for the explanations!

https://codereview.chromium.org/2540653003/

Reply all

Reply to author

Forward