Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568743005:
Place the out of flow positioned blocks (Closed)
|
Created:
4 years ago by atotic Modified:
3 years, 11 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, 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. |
DescriptionPlace the out of flow positioned blocks.
Connects ng_block_algorithm with ng_absolute_utils. block_algorithm
collects all the oof blocks, and positions them using
ng_out_of_flow_layout_part.
Future work:
1) use ResolveInline/Block Length to compute width/height inside
ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent
lengths.
2) Investigate what happens when out_of_flow blocks cross the old/new layout
boundary. There might be cases we are not handling.
3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical.
They are almost the same function (Ian would like this).
BUG=635619
[ng_place]
Committed: https://crrev.com/a572b1d7a0042ab8974e4cd8e7873200446ccc6b
Cr-Commit-Position: refs/heads/master@{#441347}
Patch Set 1 #Patch Set 2 : Out of flow positioning: placing the elements #
Total comments: 25
Patch Set 3 : CR fixes #Patch Set 4 : block_estimate_: fix buggy response to CR #
Total comments: 5
Patch Set 5 : CR fixes #Patch Set 6 : skip failing tests #Messages
Total messages: 37 (15 generated)
Description was changed from ========== Out of flow positioning: placing the elements minor: use static position legacy out-of-flow distribution BUG=635619 ========== to ========== Major addition: Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Minor additions: - use static position inside ng_absolute_utils - legacy out-of-flow LayoutObject distribution inside layout_ng_block_flow. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ==========
atotic@chromium.org changed reviewers: + cbiesinger@chromium.org, ikilpatrick@chromium.org
ptal.
mstensho@opera.com changed reviewers: + mstensho@opera.com
Please be sure to repeat the subject in the description here in rietveld. Otherwise the commit message will lack it. This has happened quite a few times recently, and it's annoying when browsing the change log. See e.g. https://chromium.googlesource.com/chromium/src/+/daaaf8661aa98f3f3eaa69a30816... This isn't very intuitive behavior of a review tool, but at least you get it for free if you just do git cl upload.
On 2016/12/13 08:07:25, mstensho wrote: > Please be sure to repeat the subject in the description here in rietveld. > Otherwise the commit message will lack it. This has happened quite a few times > recently, and it's annoying when browsing the change log. > > See e.g. > https://chromium.googlesource.com/chromium/src/+/daaaf8661aa98f3f3eaa69a30816... > > This isn't very intuitive behavior of a review tool, but at least you get it for > free if you just do git cl upload. Also please wrap the lines in the message at 80 chars or so. :)
Description was changed from ========== Major addition: Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Minor additions: - use static position inside ng_absolute_utils - legacy out-of-flow LayoutObject distribution inside layout_ng_block_flow. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ========== to ========== Major addition: Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ==========
Description was changed from ========== Major addition: Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ========== to ========== Major addition: Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ==========
On 2016/12/13 at 08:14:48, mstensho wrote: > On 2016/12/13 08:07:25, mstensho wrote: > > Please be sure to repeat the subject in the description here in rietveld. > > Otherwise the commit message will lack it. This has happened quite a few times > > recently, and it's annoying when browsing the change log. > > > > See e.g. > > https://chromium.googlesource.com/chromium/src/+/daaaf8661aa98f3f3eaa69a30816... > > > > This isn't very intuitive behavior of a review tool, but at least you get it for > > free if you just do git cl upload. > > Also please wrap the lines in the message at 80 chars or so. :) done. Thanks, I was unfamilar with this.
ptal. I present to you this slimmer, test passing patch.
On 2016/12/14 02:29:04, atotic wrote: > On 2016/12/13 at 08:14:48, mstensho wrote: > > On 2016/12/13 08:07:25, mstensho wrote: > > > Please be sure to repeat the subject in the description here in rietveld. > > > Otherwise the commit message will lack it. This has happened quite a few > times > > > recently, and it's annoying when browsing the change log. > > > > > > See e.g. > > > > https://chromium.googlesource.com/chromium/src/+/daaaf8661aa98f3f3eaa69a30816... > > > > > > This isn't very intuitive behavior of a review tool, but at least you get it > for > > > free if you just do git cl upload. > > > > Also please wrap the lines in the message at 80 chars or so. :) > > done. Thanks, I was unfamilar with this. Still one wide line, and the subject still hasn't been copied into the description.
Description was changed from ========== Major addition: Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ========== to ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ==========
On 2016/12/14 at 06:40:37, mstensho wrote: > Still one wide line, and the subject still hasn't been copied into the description. Wide line, yes. Subject was: "Place the out of flow positioned blocks." Description was: "Major addition: Place the out of flow positioned blocks." Made them the same now.
On 2016/12/14 07:24:57, atotic wrote: > On 2016/12/14 at 06:40:37, mstensho wrote: > > Still one wide line, and the subject still hasn't been copied into the > description. > > Wide line, yes. > > Subject was: "Place the out of flow positioned blocks." > Description was: "Major addition: Place the out of flow positioned blocks." > > Made them the same now. Looking great. Thanks! I should probably stay away from reviewing the actual code changes, though. I only wanted to post some annoying comments about the comment. :)
Sorry for the very late review! https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:253: } else { No need for an "else" after a return. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:300: LayoutUnit block_size = ComputeBlockSizeForFragment( You don't need this anymore since you're now calling it above, right? https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:333: bool NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() { Maybe use NGLayoutStatus as the return type so it's not necessary to remember whether true means finished or more work. (Of course, kChildAlgorithmRequired would not be used here) https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:354: // TODO(atotic) Do we need to adjust builder content size here? abspos does not change the width/height of the containing block, but it does affect the overflow rect: http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQOCGwESERFRWRwBfJ1CVlVQpIud... But, that can be done later. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:20: contains_absolute_ = container_style->canContainAbsolutePositionObjects(); Neat, I didn't know about that function! But I think for your purpose, you need to || this with contains_fixed_, see e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... or the comment in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:24: space_size.block_size -= borders.block_start + borders.block_end; -= borders.BlockSum() https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:140: // 2. To compute final block fragment, when block size is knows knows -> known https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:144: if (block_estimate_) When do we enter this if? We don't call this again after we compute the block estimate, right? https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:23: class CORE_EXPORT NGOutOfFlowLayoutPart Can you add a comment to this class in general describing what it's for? https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:28: // @return true if layout can be performed. Add something like: "If false, this fragment should be passed up the tree for layout by an ancestor" https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:31: bool Layout(NGFragmentBase**, NGLogicalOffset*); Here too consider using NGLayoutStatus https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (left): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.h:307: Why did you delete this line?
By the way -- loading google.com with this patch leads to this assertion, may be worth investigating: [1:1:1221/171515.611778:3122745603354:FATAL:ng_units.h(352)] Check failed: margin_start >= LayoutUnit() ("-20" vs. "0") #0 0x7fdfef5fd72e base::debug::StackTrace::StackTrace() #1 0x7fdfef66ae2f logging::LogMessage::~LogMessage() #2 0x7fdfe6acb99a blink::NGStaticPosition::GenericPosition() #3 0x7fdfe6acbb59 blink::NGStaticPosition::TopPosition() #4 0x7fdfe6acad0e blink::(anonymous namespace)::ComputeAbsoluteVertical() #5 0x7fdfe6acb27b blink::ComputeFullAbsoluteWithChildBlockSize() #6 0x7fdfe6af93c0 blink::NGOutOfFlowLayoutPart::Layout() #7 0x7fdfe6acdb5f blink::NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() #8 0x7fdfe6acd42a blink::NGBlockLayoutAlgorithm::Layout() #9 0x7fdfe6aed084 blink::NGLayoutCoordinator::Tick() [...]
On 2016/12/21 at 22:16:28, cbiesinger wrote: > By the way -- loading google.com with this patch leads to this assertion, may be worth investigating: > > [1:1:1221/171515.611778:3122745603354:FATAL:ng_units.h(352)] Check failed: margin_start >= LayoutUnit() ("-20" vs. "0") > #0 0x7fdfef5fd72e base::debug::StackTrace::StackTrace() > #1 0x7fdfef66ae2f logging::LogMessage::~LogMessage() > #2 0x7fdfe6acb99a blink::NGStaticPosition::GenericPosition() > #3 0x7fdfe6acbb59 blink::NGStaticPosition::TopPosition() > #4 0x7fdfe6acad0e blink::(anonymous namespace)::ComputeAbsoluteVertical() > #5 0x7fdfe6acb27b blink::ComputeFullAbsoluteWithChildBlockSize() > #6 0x7fdfe6af93c0 blink::NGOutOfFlowLayoutPart::Layout() > #7 0x7fdfe6acdb5f blink::NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() > #8 0x7fdfe6acd42a blink::NGBlockLayoutAlgorithm::Layout() > #9 0x7fdfe6aed084 blink::NGLayoutCoordinator::Tick() > [...] This is new. It used to trigger a different assert, it was "child has not been positioned". I'll investigate.
On 2016/12/25 at 07:56:59, atotic wrote: > On 2016/12/21 at 22:16:28, cbiesinger wrote: > > By the way -- loading google.com with this patch leads to this assertion, may be worth investigating: > > > > [1:1:1221/171515.611778:3122745603354:FATAL:ng_units.h(352)] Check failed: margin_start >= LayoutUnit() ("-20" vs. "0") > > #0 0x7fdfef5fd72e base::debug::StackTrace::StackTrace() > > #1 0x7fdfef66ae2f logging::LogMessage::~LogMessage() > > #2 0x7fdfe6acb99a blink::NGStaticPosition::GenericPosition() > > #3 0x7fdfe6acbb59 blink::NGStaticPosition::TopPosition() > > #4 0x7fdfe6acad0e blink::(anonymous namespace)::ComputeAbsoluteVertical() > > #5 0x7fdfe6acb27b blink::ComputeFullAbsoluteWithChildBlockSize() > > #6 0x7fdfe6af93c0 blink::NGOutOfFlowLayoutPart::Layout() > > #7 0x7fdfe6acdb5f blink::NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() > > #8 0x7fdfe6acd42a blink::NGBlockLayoutAlgorithm::Layout() > > #9 0x7fdfe6aed084 blink::NGLayoutCoordinator::Tick() > > [...] > > This is new. It used to trigger a different assert, it was "child has not been positioned". I'll investigate. This exposed a deeper bug. I was using NGSizeIndefinite, which is -1 to indicate unknown margins. But -1 is a legal margin value. Will fix. The -1 margin fix will be a separate patch.
ptal. Fixed CR comments. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:253: } else { On 2016/12/21 at 21:38:50, cbiesinger wrote: > No need for an "else" after a return. done https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:300: LayoutUnit block_size = ComputeBlockSizeForFragment( On 2016/12/21 at 21:38:50, cbiesinger wrote: > You don't need this anymore since you're now calling it above, right? done. Thanks, nice catch, I blame this on persistent merge conflicts. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:333: bool NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() { On 2016/12/21 at 21:38:50, cbiesinger wrote: > Maybe use NGLayoutStatus as the return type so it's not necessary to remember whether true means finished or more work. > (Of course, kChildAlgorithmRequired would not be used here) Unsure about this. Pros: not necessary to remember what true means Cons: introducing state machine knowledge into this routine. Currently, all it knows is how to layout oof children. Only routine that knows about layout_state_ is Layout(). What do you think about just adding documentation for the return value? https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:354: // TODO(atotic) Do we need to adjust builder content size here? On 2016/12/21 at 21:38:50, cbiesinger wrote: > abspos does not change the width/height of the containing block, but it does affect the overflow rect: > http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQOCGwESERFRWRwBfJ1CVlVQpIud... > > But, that can be done later. done. Added a comment. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:20: contains_absolute_ = container_style->canContainAbsolutePositionObjects(); done. Good catch. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:24: space_size.block_size -= borders.block_start + borders.block_end; On 2016/12/21 at 21:38:50, cbiesinger wrote: > -= borders.BlockSum() done. Also fixed InlineSum() https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:140: // 2. To compute final block fragment, when block size is knows On 2016/12/21 at 21:38:50, cbiesinger wrote: > knows -> known done https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:144: if (block_estimate_) On 2016/12/21 at 21:38:50, cbiesinger wrote: > When do we enter this if? We don't call this again after we compute the block estimate, right? done. Another good catch, code was a leftover from implementation. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:23: class CORE_EXPORT NGOutOfFlowLayoutPart On 2016/12/21 at 21:38:50, cbiesinger wrote: > Can you add a comment to this class in general describing what it's for? done. Added comment, and modifed sample code in NGFragmentBuilder::AddOutOfFlowChildCandidate to properly use this api. https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:28: // @return true if layout can be performed. On 2016/12/21 at 21:38:50, cbiesinger wrote: > Add something like: "If false, this fragment should be passed up the tree for layout by an ancestor" done https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:31: bool Layout(NGFragmentBase**, NGLogicalOffset*); On 2016/12/21 at 21:38:50, cbiesinger wrote: > Here too consider using NGLayoutStatus done https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (left): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.h:307: On 2016/12/21 at 21:38:50, cbiesinger wrote: > Why did you delete this line? done. Probably a merge conflict artifact.
ptal My original response to CR was buggy: On 2016/12/21 at 21:38:50, cbiesinger wrote: >> When do we enter this if? We don't call this again after we compute the block estimate, right? > atotic replied: >done. Another good catch, code was a leftover from implementation. I was wrong. block_estimate is needed here because size computed by abspos algorithm needs to be applied to constraint space used to layout oof block. https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:145: if (block_estimate_) On 2016/12/21 at 21:38:50, cbiesinger wrote: >> When do we enter this if? We don't call this again after we compute the block estimate, right? > atotic replied: >done. Another good catch, code was a leftover from implementation. I was wrong. block_estimate is needed here because size computed by abspos algorithm needs to be applied to constraint space used to layout oof block.
Description was changed from ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 ========== to ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 [ng_place] ==========
lgtm https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2568743005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:333: bool NGBlockLayoutAlgorithm::LayoutOutOfFlowChild() { On 2016/12/28 19:36:45, atotic wrote: > On 2016/12/21 at 21:38:50, cbiesinger wrote: > > Maybe use NGLayoutStatus as the return type so it's not necessary to remember > whether true means finished or more work. > > (Of course, kChildAlgorithmRequired would not be used here) > > Unsure about this. > Pros: not necessary to remember what true means > Cons: introducing state machine knowledge into this routine. Currently, all it > knows is how to layout oof children. Only routine that knows about layout_state_ > is Layout(). > > What do you think about just adding documentation for the return value? Just documentation is fine with me, though it seems that this function will implicitly know about state machines via NGOutOfFlowLayoutPart::Layout. https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:145: if (block_estimate_) On 2016/12/28 20:35:16, atotic wrote: > On 2016/12/21 at 21:38:50, cbiesinger wrote: > >> When do we enter this if? We don't call this again after we compute the block > estimate, right? > > atotic replied: > >done. Another good catch, code was a leftover from implementation. > > I was wrong. block_estimate is needed here because size computed by abspos > algorithm needs to be applied to constraint space used to layout oof block. I misread the code, I think. We set block_estimate_ in kGenerateFragment even if we did not previously call ComputeNodeFragment. https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h (right): https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:36: // @return true if layout is complete. This is now outdated
The CQ bit was checked by atotic@chromium.org
https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc (right): https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.cc:145: if (block_estimate_) On 2017/01/04 at 00:13:16, cbiesinger wrote: > On 2016/12/28 20:35:16, atotic wrote: > > On 2016/12/21 at 21:38:50, cbiesinger wrote: > > >> When do we enter this if? We don't call this again after we compute the block > > estimate, right? > > > atotic replied: > > >done. Another good catch, code was a leftover from implementation. > > > > I was wrong. block_estimate is needed here because size computed by abspos > > algorithm needs to be applied to constraint space used to layout oof block. > > I misread the code, I think. We set block_estimate_ in kGenerateFragment even if we did not previously call ComputeNodeFragment. ack. https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h (right): https://codereview.chromium.org/2568743005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part.h:36: // @return true if layout is complete. On 2017/01/04 at 00:13:16, cbiesinger wrote: > This is now outdated removed.
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2568743005/#ps80001 (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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/01/04 at 03:26:10, commit-bot wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) Fixed by commenting out 3 failing tests: virtual/layout_ng/css2.1/20110323/absolute-non-replaced-max-height-008.htm virtual/layout_ng/css2.1/20110323/absolute-non-replaced-height-008.htm virtual/layout_ng/css2.1/20110323/absolute-non-replaced-height-007.htm Most of the absolute test suite is already commented out, adding 3 more did not feel out of place.
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 Link to the patchset: https://codereview.chromium.org/2568743005/#ps100001 (title: "skip failing tests")
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": 100001, "attempt_start_ts": 1483516001638050,
"parent_rev": "c7a0e7506585a82c4813f99f7123c7c33b321806", "commit_rev":
"14aecd91aa03f219e818f3f5668b715da493d1b0"}
Message was sent while issue was closed.
Description was changed from ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 [ng_place] ========== to ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 [ng_place] Review-Url: https://codereview.chromium.org/2568743005 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 [ng_place] Review-Url: https://codereview.chromium.org/2568743005 ========== to ========== Place the out of flow positioned blocks. Connects ng_block_algorithm with ng_absolute_utils. block_algorithm collects all the oof blocks, and positions them using ng_out_of_flow_layout_part. Future work: 1) use ResolveInline/Block Length to compute width/height inside ng_absolute_utils. This is needed to properly resolve MinContent/MaxContent lengths. 2) Investigate what happens when out_of_flow blocks cross the old/new layout boundary. There might be cases we are not handling. 3?) Unify ComputeAbsoluteHorizontal and ComputeAbsoluteVertical. They are almost the same function (Ian would like this). BUG=635619 [ng_place] Committed: https://crrev.com/a572b1d7a0042ab8974e4cd8e7873200446ccc6b Cr-Commit-Position: refs/heads/master@{#441347} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a572b1d7a0042ab8974e4cd8e7873200446ccc6b Cr-Commit-Position: refs/heads/master@{#441347} |
