Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(92)

Issue 2540653003: Implement collection of out-of-flow descendants (Closed)

Created:
4 years ago by atotic
Modified:
4 years ago
Reviewers:
cbiesinger, ikilpatrick
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.

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 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 #

Messages

Total messages: 25 (10 generated)
atotic
4 years ago (2016-11-29 20:14:08 UTC) #3
atotic
On 2016/11/29 at 20:14:08, atotic2 wrote: > ptal
4 years ago (2016-12-02 01:08:45 UTC) #4
ikilpatrick
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 ...
4 years ago (2016-12-02 17:47:06 UTC) #5
atotic
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 ...
4 years ago (2016-12-02 19:55:16 UTC) #6
ikilpatrick
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, ...
4 years ago (2016-12-02 21:06:13 UTC) #7
atotic
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 = ...
4 years ago (2016-12-02 22:19:38 UTC) #8
ikilpatrick
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 ...
4 years ago (2016-12-02 22:23:43 UTC) #9
cbiesinger
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 ...
4 years ago (2016-12-02 22:37:03 UTC) #11
atotic
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 ...
4 years ago (2016-12-02 23:08:44 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2540653003/100001
4 years ago (2016-12-02 23:09:40 UTC) #15
commit-bot: I haz the power
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, ...
4 years ago (2016-12-02 23:12:19 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2540653003/120001
4 years ago (2016-12-02 23:53:54 UTC) #20
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years ago (2016-12-03 02:52:12 UTC) #22
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/e970904c78038db4d51fd4f911d08c40d9c59030 Cr-Commit-Position: refs/heads/master@{#436151}
4 years ago (2016-12-03 02:54:25 UTC) #24
cbiesinger
4 years ago (2016-12-04 15:32:02 UTC) #25
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!

Powered by Google App Engine
This is Rietveld 408576698