Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2462153002:
[layoutng] Support computing min-content and max-content (Closed)
|
Created:
4 years, 1 month ago by cbiesinger Modified:
4 years, 1 month 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. |
Description[layoutng] Support computing min-content and max-content
This is just the first step -- we still need to integrate this into
ng_length_utils as the next step, and optimize this by implementing
ComputeMinAndMaxContentSizes on the NGBLockLayoutAlgorithm.
In the meantime this implements the synthesized min-content and
max-content as described in the layoutng design doc:
https://docs.google.com/document/d/1uxbDh4uONFQOiGuiumlJBLGgO4KDWB8ZEkp7Rd47fw4/edit#heading=h.bkkc2a8yyuly
The unit test is very simple, but until we implement line layout we can't
really write a more comprehensive test.
R=ikilpatrick@chromium.org,atotic@chromium.org,eae@chromium.org
BUG=635619
Committed: https://crrev.com/2b4efe85b807d2fa034acd8108aa994148708e0a
Cr-Commit-Position: refs/heads/master@{#429492}
Patch Set 1 #
Total comments: 17
Patch Set 2 : review comments #
Total comments: 4
Patch Set 3 : more review comments #
Messages
Total messages: 25 (12 generated)
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:74: FromPlatformDirection(Style()->direction()), builder.ToConstraintSpace()); Presumably once Ian's work is done this will be builder.ToConstraintSpace.ToLogicalConstraintSpace or something, right? Might want to add a TODO. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.h:37: void ComputeMinAndMaxContentSizes(NGFragment** min_content, Shouldn't this return rects or values? Returning a fragment seems odd unless we plan to reuse the min/max fragments somehow.
https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.h:37: void ComputeMinAndMaxContentSizes(NGFragment** min_content, On 2016/10/31 19:30:26, eae wrote: > Shouldn't this return rects or values? Returning a fragment seems odd unless we > plan to reuse the min/max fragments somehow. I also prefer LayoutUnit but I thought we agreed in the meeting to return fragments? Ian, any thoughts?
glebl@chromium.org changed reviewers: + glebl@chromium.org
https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:71: builder.SetContainerSize(NGLogicalSize(LayoutUnit(), LayoutUnit())); nit. you can chain SetContainerSize call to line above. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:78: if (algorithm->ComputeMinAndMaxContentSizes(min_content, max_content) == IMO it's a bit confusing that we have 2 almost identical public functions with the same name. If a person is not familiar with the code then they would probably would be interested to know what's the difference between those 2 functions? https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:87: HeapVector<Member<const NGPhysicalFragmentBase>> empty; may be add an additional constructor to NGPhysicalFragment? otherwise it's not clear why we pass an empty, unused? list of children here and below. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.h:37: void ComputeMinAndMaxContentSizes(NGFragment** min_content, IMO returning std::pair<NGFragment*, NGFragment*> would be a little bit more appropriate here. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.h:36: enum MinAndMaxState { Success, NotImplemented }; may be use std::optional instead of one more enum? https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment_base.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_physical_fragment_base.h:41: NGPhysicalSize OverflowSize() { return overflow_; } shouldn't a getter function name for variable have the same name as the variable?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.h:37: void ComputeMinAndMaxContentSizes(NGFragment** min_content, On 2016/10/31 19:52:47, glebl wrote: > IMO returning std::pair<NGFragment*, NGFragment*> would be a little bit more > appropriate here. I'm ok with returning a pair of layout units if people think this is a little too much at the moment. This should be a pretty easy decision to revist later on. The idea with reusing fragments is that this would be easier for caching. I.e. if (most) things just go through the Layout() calls and produce fragments, we can re-use parts of the min/max content sizes as needed. One question, what (should) happen with mixed writing modes content sizes? Fragments here may be easier for mixed writing modes?
https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.h:37: void ComputeMinAndMaxContentSizes(NGFragment** min_content, This API will call Layout() in a tight loop, which makes it blocking layout. Is this final, or just temporary? I assume it is temporary because we do not want blocking.
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:71: builder.SetContainerSize(NGLogicalSize(LayoutUnit(), LayoutUnit())); On 2016/10/31 19:52:47, glebl wrote: > nit. you can chain SetContainerSize call to line above. I'd rather not chain something to the constructor like that https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:74: FromPlatformDirection(Style()->direction()), builder.ToConstraintSpace()); On 2016/10/31 19:30:25, eae wrote: > Presumably once Ian's work is done this will be > builder.ToConstraintSpace.ToLogicalConstraintSpace or something, right? Might > want to add a TODO. Ah, is that the plan? Added a TODO. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:78: if (algorithm->ComputeMinAndMaxContentSizes(min_content, max_content) == On 2016/10/31 19:52:47, glebl wrote: > IMO it's a bit confusing that we have 2 almost identical public functions with > the same name. If a person is not familiar with the code then they would > probably would be interested to know what's the difference between those 2 > functions? Renamed this one to ComputeOrSynthesizeMinAndMaxContentSizes https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.cc:87: HeapVector<Member<const NGPhysicalFragmentBase>> empty; On 2016/10/31 19:52:47, glebl wrote: > may be add an additional constructor to NGPhysicalFragment? otherwise it's not > clear why we pass an empty, unused? list of children here and below. Removed this entirely due to other changes. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_box.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_box.h:37: void ComputeMinAndMaxContentSizes(NGFragment** min_content, On 2016/11/01 16:59:00, atotic2 wrote: > This API will call Layout() in a tight loop, which makes it blocking layout. > > Is this final, or just temporary? I assume it is temporary because we do not > want blocking. Collective answer to all the comments: - I dislike pair because it doesn't tell you which is which but I did make it a struct, which has some other advantages (ie. can pass to ResolveInlineSize, etc.) - Not convinced how much Fragments would help with caching -- you can't mix them with "regular" fragments as there's no constraint space. If you synthesize a constraint space for caching, you'd still have problems because this function transforms the layout fragments a bit so that Width and Height work as expected. I realized this does not address writing modes (oops), adding a TODO for now. But I think that is better solved by the caller deciding between Layout and ComputeMinAndMax because this function would still want to avoid computing block sizes in the common case for performance reasons. - Good point on making this function interruptible. I think you're right that I should do that. I made the enum a tri-state one (this also means I can't use the std::optional suggestion) and changed the NGBox function accordingly. https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_physical_fragment_base.h (right): https://codereview.chromium.org/2462153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_physical_fragment_base.h:41: NGPhysicalSize OverflowSize() { return overflow_; } On 2016/10/31 19:52:47, glebl wrote: > shouldn't a getter function name for variable have the same name as the > variable? It turns out I no longer need this function, so I removed it.
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2462153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2462153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:67: bool NGBox::ComputeOrSynthesizeMinAndMaxContentSizes( This routine name is 41 chars long, 1/2 of our line budget. Suggestion: // Use comment to describe Synthesize Rename to ComputeMinMaxContentSizes https://codereview.chromium.org/2462153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:82: algorithm_ = algorithm_ is used in 2 functions (Layout & MinMax) for different purposes. This can lead to hard-to-debug error if caller interleaves them (as you noted in .h:37 docs). Suggestion: have two different algorithm_ members (_layout_algorithm, _minmax_algorithm). Inside Layout, DCHECK(_minmax == null) Inside MinMax, DCHECK(_layout == null) This would catch the error if it happens.
This is great, we can move to fragments later if we need to for caching. In general I prefer to keep things as simple as they can be :) LGTM
https://codereview.chromium.org/2462153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2462153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:67: bool NGBox::ComputeOrSynthesizeMinAndMaxContentSizes( On 2016/11/01 21:56:02, atotic2 wrote: > This routine name is 41 chars long, 1/2 of our line budget. > Suggestion: > // Use comment to describe Synthesize > Rename to ComputeMinMaxContentSizes Ok, I renamed it back to ComputeMinAndMaxContentSizes. Note that Gleb requested the rename earlier. However, NGBox::Layout does have differences from NGBlockLayoutAlgorithm::Layout as well, so maybe the same name is not so bad. https://codereview.chromium.org/2462153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_box.cc:82: algorithm_ = On 2016/11/01 21:56:02, atotic2 wrote: > algorithm_ is used in 2 functions (Layout & MinMax) for different purposes. This > can lead to hard-to-debug error if caller interleaves them (as you noted in > .h:37 docs). Suggestion: > > have two different algorithm_ members (_layout_algorithm, _minmax_algorithm). > > Inside Layout, DCHECK(_minmax == null) > Inside MinMax, DCHECK(_layout == null) > > This would catch the error if it happens. OK -- I will do that for now. Once we have the state machine we can maybe use the state variable for a similar assert and don't need to store two layout algorithms.
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2462153002/#ps40001 (title: "more review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [layoutng] Support computing min-content and max-content This is just the first step -- we still need to integrate this into ng_length_utils as the next step, and optimize this by implementing ComputeMinAndMaxContentSizes on the NGBLockLayoutAlgorithm. In the meantime this implements the synthesized min-content and max-content as described in the layoutng design doc: https://docs.google.com/document/d/1uxbDh4uONFQOiGuiumlJBLGgO4KDWB8ZEkp7Rd47f... The unit test is very simple, but until we implement line layout we can't really write a more comprehensive test. R=ikilpatrick@chromium.org,atotic@chromium.org,eae@chromium.org BUG=635619 ========== to ========== [layoutng] Support computing min-content and max-content This is just the first step -- we still need to integrate this into ng_length_utils as the next step, and optimize this by implementing ComputeMinAndMaxContentSizes on the NGBLockLayoutAlgorithm. In the meantime this implements the synthesized min-content and max-content as described in the layoutng design doc: https://docs.google.com/document/d/1uxbDh4uONFQOiGuiumlJBLGgO4KDWB8ZEkp7Rd47f... The unit test is very simple, but until we implement line layout we can't really write a more comprehensive test. R=ikilpatrick@chromium.org,atotic@chromium.org,eae@chromium.org BUG=635619 Committed: https://crrev.com/2b4efe85b807d2fa034acd8108aa994148708e0a Cr-Commit-Position: refs/heads/master@{#429492} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2b4efe85b807d2fa034acd8108aa994148708e0a Cr-Commit-Position: refs/heads/master@{#429492} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
