|
|
Created:
4 years, 1 month ago by ikilpatrick Modified:
4 years, 1 month ago CC:
chromium-reviews, cbiesinger, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Add NGConstraintSpaceBuilder class.
This patch just introduces the builder without changing the API surface of the
constraint space.
Once everything is moved to using the builder; I'll start changing the API
surface of the constraint space to have PercentageSize() etc.
BUG=635619
Committed: https://crrev.com/b1409531577199ba07628761ac4c00385177c850
Cr-Commit-Position: refs/heads/master@{#427769}
Patch Set 1 #Patch Set 2 : . #
Total comments: 14
Patch Set 3 : address comments. #
Total comments: 2
Patch Set 4 : it's -> its \o/ #
Total comments: 2
Messages
Total messages: 20 (8 generated)
ikilpatrick@chromium.org changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, glebl@chromium.org
https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: class CORE_EXPORT NGConstraintSpaceBuilder final Should this be PhysicalConstraintSpaceBuilder? Also do we really need a builder for this given that we expect most constraint spaces to share a NGPhysicalConstraintSpace. For NGConstraintSpace I clearly see the need but for NGPhysicalConstraintSpace? Not quite as clear-cut.
just a couple of minor comments about the style. https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc (right): https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:56: } else { you don't need 'else' here. https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:14: : public GarbageCollectedFinalized<NGConstraintSpaceBuilder> { .nit you don't have anything that needs to be finalized here. just GarbageCollected https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:23: NGConstraintSpaceBuilder& SetFragmentationType(NGFragmentationType); Can we use SetFragmentType here and have a TODO to rename NGFragmentationType to NGFragmentType https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:26: // Creates a new constraint space. Can be called multiple times. can you provide a bit more details here about why it's expected to be called multiple times, how those constraint spaces are different from each other. https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:34: unsigned writing_mode_ : 2; should we separate mutable fields from const fields? // const bit fields. const unsigned writing_mode_ : 2; const unsigned fixed_inline_ : 1; // mutable bit fields. unsigned inline_direction_triggers_scrollbar_ : 1; unsigned block_direction_triggers_scrollbar_ : 1; https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:39: unsigned fragmentation_type_ : 2; .nit fragment_type_ ?
I'll leave the detailed review to Emil and Gleb but this lgtm in principle. https://codereview.chromium.org/2446243003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:31: // - Has it's size is determined by it's parent layout (flex, abs-pos). it's -> its
https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: class CORE_EXPORT NGConstraintSpaceBuilder final On 2016/10/25 17:26:27, eae wrote: > Should this be PhysicalConstraintSpaceBuilder? Also do we really need a builder > for this given that we expect most constraint spaces to share a > NGPhysicalConstraintSpace. > For NGConstraintSpace I clearly see the need but for NGPhysicalConstraintSpace? > Not quite as clear-cut. Right, yeah there are more cases than I originally thought when we are going to need to change the constraint space for a child. I.e. Est. new formatting context (eventually should be moved to style, but complex due to multi-col), Inside fragmentation context (where to fragment depends on your siblings position). Size is determined by parent. I could add a bool on this like "has_changed_" which if false, just returns the previous space? https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: class CORE_EXPORT NGConstraintSpaceBuilder final On 2016/10/25 17:26:27, eae wrote: > Should this be PhysicalConstraintSpaceBuilder? Can rename to that; was following ng_fragment_builder which also returns a PhysicalFragment; up to you :). https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:14: : public GarbageCollectedFinalized<NGConstraintSpaceBuilder> { On 2016/10/25 17:37:27, glebl wrote: > .nit you don't have anything that needs to be finalized here. just > GarbageCollected Done. https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:23: NGConstraintSpaceBuilder& SetFragmentationType(NGFragmentationType); On 2016/10/25 17:37:27, glebl wrote: > Can we use SetFragmentType here and have a TODO to rename NGFragmentationType to > NGFragmentType FragmentationType and FragmentType are different, (currently not using FragmentationType yet). It might be better named FragmentationBreakType? See fragmentation break in: https://drafts.csswg.org/css-break/ Thoughts? https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:26: // Creates a new constraint space. Can be called multiple times. On 2016/10/25 17:37:27, glebl wrote: > can you provide a bit more details here about why it's expected to be called > multiple times, how those constraint spaces are different from each other. Done. https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:34: unsigned writing_mode_ : 2; On 2016/10/25 17:37:27, glebl wrote: > should we separate mutable fields from const fields? > // const bit fields. > const unsigned writing_mode_ : 2; > const unsigned fixed_inline_ : 1; > > // mutable bit fields. > unsigned inline_direction_triggers_scrollbar_ : 1; > unsigned block_direction_triggers_scrollbar_ : 1; Done. https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:39: unsigned fragmentation_type_ : 2; On 2016/10/25 17:37:27, glebl wrote: > .nit fragment_type_ ? See above. https://codereview.chromium.org/2446243003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:31: // - Has it's size is determined by it's parent layout (flex, abs-pos). On 2016/10/25 19:53:07, cbiesinger wrote: > it's -> its Done.
On 2016/10/25 23:20:43, ikilpatrick wrote: > https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h > (right): > > https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: class > CORE_EXPORT NGConstraintSpaceBuilder final > On 2016/10/25 17:26:27, eae wrote: > > Should this be PhysicalConstraintSpaceBuilder? Also do we really need a > builder > > for this given that we expect most constraint spaces to share a > > NGPhysicalConstraintSpace. > > For NGConstraintSpace I clearly see the need but for > NGPhysicalConstraintSpace? > > Not quite as clear-cut. > > Right, yeah there are more cases than I originally thought when we are going to > need to change the constraint space for a child. I.e. > > Est. new formatting context (eventually should be moved to style, but complex > due to multi-col), > Inside fragmentation context (where to fragment depends on your siblings > position). > Size is determined by parent. Oh, that makes sense. OK > > https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: class > CORE_EXPORT NGConstraintSpaceBuilder final > On 2016/10/25 17:26:27, eae wrote: > > Should this be PhysicalConstraintSpaceBuilder? > > Can rename to that; was following ng_fragment_builder which also returns a > PhysicalFragment; up to you :). Do we want to add a builder for NGConstraintSpaces too or should the only way to create those be by methods *on* PhysicalConstraintSpace? If the latter then the current name for the builder is fine. LGTM
On 2016/10/25 23:54:47, eae wrote: > On 2016/10/25 23:20:43, ikilpatrick wrote: > > > https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h > > (right): > > > > > https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: > class > > CORE_EXPORT NGConstraintSpaceBuilder final > > On 2016/10/25 17:26:27, eae wrote: > > > Should this be PhysicalConstraintSpaceBuilder? Also do we really need a > > builder > > > for this given that we expect most constraint spaces to share a > > > NGPhysicalConstraintSpace. > > > For NGConstraintSpace I clearly see the need but for > > NGPhysicalConstraintSpace? > > > Not quite as clear-cut. > > > > Right, yeah there are more cases than I originally thought when we are going > to > > need to change the constraint space for a child. I.e. > > > > Est. new formatting context (eventually should be moved to style, but complex > > due to multi-col), > > Inside fragmentation context (where to fragment depends on your siblings > > position). > > Size is determined by parent. > > Oh, that makes sense. OK > > > > > > https://codereview.chromium.org/2446243003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:13: > class > > CORE_EXPORT NGConstraintSpaceBuilder final > > On 2016/10/25 17:26:27, eae wrote: > > > Should this be PhysicalConstraintSpaceBuilder? > > > > Can rename to that; was following ng_fragment_builder which also returns a > > PhysicalFragment; up to you :). > > Do we want to add a builder for NGConstraintSpaces too or should the only way to > create those be by methods *on* PhysicalConstraintSpace? If the latter then the > current name for the builder is fine. > > > LGTM Yup envisioning the latter at the moment.
The CQ bit was checked by ikilpatrick@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/2446243003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:19: NGConstraintSpaceBuilder& SetFixedSize(bool fixed_inline, bool fixed_block); you don't need to do this in this patch but lets split this and other methods below for readability sake, i.e. NGConstraintSpaceBuilder& SetIsFixedSizeInline(bool is_fixed_size_inline) { this.is_fixed_size_inline = is_fixed_size_inline } please compare the old and new version OLD NGConstraintSpaceBuilder builder; // It's not clear what we're setting to true/false here. builder.SetFixedSize(true, false); NEW NGConstraintSpaceBuilder builder; builder .SetIsFixedSizeInline(true) .SetIsFixedSizeBlock(true); also I recommend to use IS prefix, this is because SetFixedSize sounds like we're setting a fixed size, rectangle or something but in reality we want to set some options.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2446243003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h (right): https://codereview.chromium.org/2446243003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:19: NGConstraintSpaceBuilder& SetFixedSize(bool fixed_inline, bool fixed_block); On 2016/10/26 18:22:14, glebl wrote: > you don't need to do this in this patch but lets split this and other methods > below for readability sake, i.e. > > NGConstraintSpaceBuilder& SetIsFixedSizeInline(bool is_fixed_size_inline) { > this.is_fixed_size_inline = is_fixed_size_inline > } > > please compare the old and new version > OLD > NGConstraintSpaceBuilder builder; > // It's not clear what we're setting to true/false here. > builder.SetFixedSize(true, false); > > NEW > NGConstraintSpaceBuilder builder; > builder > .SetIsFixedSizeInline(true) > .SetIsFixedSizeBlock(true); > > also I recommend to use IS prefix, this is because SetFixedSize sounds like > we're setting a fixed size, rectangle or something but in reality we want to set > some options. ^ sgtm.
The CQ bit was checked by ikilpatrick@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/2446243003/#ps60001 (title: "it's -> its \o/")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Add NGConstraintSpaceBuilder class. This patch just introduces the builder without changing the API surface of the constraint space. Once everything is moved to using the builder; I'll start changing the API surface of the constraint space to have PercentageSize() etc. BUG=635619 ========== to ========== [LayoutNG] Add NGConstraintSpaceBuilder class. This patch just introduces the builder without changing the API surface of the constraint space. Once everything is moved to using the builder; I'll start changing the API surface of the constraint space to have PercentageSize() etc. BUG=635619 Committed: https://crrev.com/b1409531577199ba07628761ac4c00385177c850 Cr-Commit-Position: refs/heads/master@{#427769} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b1409531577199ba07628761ac4c00385177c850 Cr-Commit-Position: refs/heads/master@{#427769} |