|
|
Created:
4 years ago by atotic Modified:
4 years ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement 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
Committed: https://crrev.com/e970904c78038db4d51fd4f911d08c40d9c59030
Cr-Commit-Position: refs/heads/master@{#436151}
Patch Set 1 #Patch Set 2 : Reqorked fragment builder per CR #Patch Set 3 : Removed unused members from block_layout #
Total comments: 43
Patch Set 4 : Collection of out-of-flow descendants #
Total comments: 17
Patch Set 5 : CR fixes: rename Corner to StaticPosition #
Total comments: 11
Patch Set 6 : CR fixes #Patch Set 7 : Merge conflicts resolved #Dependent Patchsets: Messages
Total messages: 25 (10 generated)
atotic@chromium.org changed reviewers: + ikilpatrick@chromium.org
Description was changed from ========== Implement collection of out-of-flow descendants Next step in abspos: collect abspos descendants and their position 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) BUG=635619 ========== to ========== 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 ==========
On 2016/11/29 at 20:14:08, atotic2 wrote: > ptal
https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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... https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.h:282: Type type; // Logical corner that corresponds to physical top left. remove this comment?
CR fixes done, ptal https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (left): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2540653003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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).
Renamed NGCorner to NGStaticPosition, etc. ptal https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h (right): https://codereview.chromium.org/2540653003/diff/60001/third_party/WebKit/Sour... 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
lgtm https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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.
cbiesinger@chromium.org changed reviewers: + cbiesinger@chromium.org
lgtm https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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?
CR fixes are in https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment.h (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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
The CQ bit was checked by atotic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2540653003/#ps100001 (title: "CR fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by atotic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2540653003/#ps120001 (title: "Merge conflicts resolved")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480722781701960, "parent_rev": "f298f78e77b53d12ef3f9a0a35cc77850f1d6278", "commit_rev": "7ef5acbe1792675c2fb3d0a062f601a32dd1ee43"}
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/e970904c78038db4d51fd4f911d08c40d9c59030 Cr-Commit-Position: refs/heads/master@{#436151} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e970904c78038db4d51fd4f911d08c40d9c59030 Cr-Commit-Position: refs/heads/master@{#436151}
Message was sent while issue was closed.
https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2540653003/diff/80001/third_party/WebKit/Sour... 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! |