Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267383003:
[LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". (Closed)
|
Created:
4 years, 4 months ago by ikilpatrick Modified:
4 years, 3 months ago CC:
chromium-reviews, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view".
NGConstraintSpace now has a backing NGPhysicalConstaintSpace and
provides abstract coordinate system accessors for everything.
Makes NG*ConstraintSpace GarbagedCollected as well as we'll need
this once we move to a state machine / for caching fragment results.
BUG=635619
Committed: https://crrev.com/6f16535150edfba413105be7327ab4bb3ce630d7
Cr-Commit-Position: refs/heads/master@{#414745}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : .. #
Total comments: 21
Patch Set 4 : now with test. #Patch Set 5 : .. #Patch Set 6 : .. #
Total comments: 25
Patch Set 7 : address comments. #Patch Set 8 : . #Patch Set 9 : fix win compile? #Messages
Total messages: 59 (34 generated)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 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...
Description was changed from ========== [layout-ng] Create a view class for the constraint space. BUG= ========== to ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 ==========
Description was changed from ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 ========== to ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 ==========
ikilpatrick@chromium.org changed reviewers: + cbiesinger@chromium.org, eae@chromium.org
I'm not attached to any names is this patch, if y'all have a better name than NGPhysicalConstraintSpace lmk.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:26: box.layout(*constraintSpace); Mixing pointers and references like this is gross and leads to real bugs given the difference in behavior between the two. Can we change NGBox::layout to take a const pointer instead? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:13: NGConstraintSpace* NGConstraintSpace::fromLayoutObject(const LayoutBox& box) { This probably belongs on NGPhysicalConstraintSpace. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:28: NGConstraintSpace(NGWritingMode writing_mode) Do we need this constructor? When would a NGConstraintSpace not be backed by an actual NGPhysicalConstraintSpace? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void SetContainerSize(NGLogicalSize container_size); Shouldn't this be part of the constructor instead? Only layoutOpertunities will ever create NGConstraintSpace instances, right? If so setting the size should probably be a part of that API. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:74: void setFixedSize(bool inlineFixed, bool blockFixed); Move setters to NGPhysicalConstraintSpace? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:79: void setFragmentationType(NGFragmentationType); Move to NGPhysicalConstraintSpace? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:81: void addExclusion(const NGExclusion, unsigned options = 0); Move to NGPhysicalConstraintSpace?
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
I will have to resume this review later but here are some comments so far! https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:24: const auto constraintSpace = NGConstraintSpace::fromLayoutObject(*this); Please use auto* when this is for a pointer https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:40: physical_space_(new NGPhysicalConstraintSpace(*other.physical_space_)) { so a derived constraint space in other will become a non-derived space here? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void SetContainerSize(NGLogicalSize container_size); On 2016/08/24 17:49:51, eae wrote: > Shouldn't this be part of the constructor instead? Only layoutOpertunities will > ever create NGConstraintSpace instances, right? If so setting the size should > probably be a part of that API. While I second the constructor comment, I don't think only layoutOpportunities will create constraint spaces. Each Layout() implementation will have to create one so that the container size is correct. Maybe you can argue that it should reuse its layout opportunity's constraint space, but then you have to make the constraint space non-const (that's passed to Layout) https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:64: bool inlineTriggersScrollbar() const; Codestyle is to start with an uppercase letter; maybe change that while we're touching this?
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/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void SetContainerSize(NGLogicalSize container_size); On 2016/08/24 18:18:35, cbiesinger wrote: > On 2016/08/24 17:49:51, eae wrote: > > Shouldn't this be part of the constructor instead? Only layoutOpertunities > will > > ever create NGConstraintSpace instances, right? If so setting the size should > > probably be a part of that API. > > While I second the constructor comment, I don't think only layoutOpportunities > will create constraint spaces. Each Layout() implementation will have to create > one so that the container size is correct. Maybe you can argue that it should > reuse its layout opportunity's constraint space, but then you have to make the > constraint space non-const (that's passed to Layout) Presumably layouts should create PhysicalConstraintSpaces and not views like this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:24: const auto constraintSpace = NGConstraintSpace::fromLayoutObject(*this); On 2016/08/24 18:18:35, cbiesinger wrote: > Please use auto* when this is for a pointer Done. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:26: box.layout(*constraintSpace); On 2016/08/24 17:49:51, eae wrote: > Mixing pointers and references like this is gross and leads to real bugs given > the difference in behavior between the two. > > Can we change NGBox::layout to take a const pointer instead? Done. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:13: NGConstraintSpace* NGConstraintSpace::fromLayoutObject(const LayoutBox& box) { On 2016/08/24 17:49:51, eae wrote: > This probably belongs on NGPhysicalConstraintSpace. Makes more sense to build in logical coords? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:28: NGConstraintSpace(NGWritingMode writing_mode) On 2016/08/24 17:49:51, eae wrote: > Do we need this constructor? When would a NGConstraintSpace not be backed by an > actual NGPhysicalConstraintSpace? This is for the initial NGConstraintSpace (typically created by NGConstraintSpace::fromLayoutObject) or for a "synthetic" constraint space (like when you force a size on children). This just creates a backing NGPhysicalConstraintSpace for itself. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:40: physical_space_(new NGPhysicalConstraintSpace(*other.physical_space_)) { On 2016/08/24 18:18:35, cbiesinger wrote: > so a derived constraint space in other will become a non-derived space here? I didn't touch NGDerivedConstraintSpace in this patch as not entirely sure what we need yet; I think we'll want a NGDerivedPhysicalConstraintSpace to back the physical size coords? But not sure. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void SetContainerSize(NGLogicalSize container_size); On 2016/08/24 18:20:30, eae wrote: > On 2016/08/24 18:18:35, cbiesinger wrote: > > On 2016/08/24 17:49:51, eae wrote: > > > Shouldn't this be part of the constructor instead? Only layoutOpertunities > > will > > > ever create NGConstraintSpace instances, right? If so setting the size > should > > > probably be a part of that API. > > > > While I second the constructor comment, I don't think only layoutOpportunities > > will create constraint spaces. Each Layout() implementation will have to > create > > one so that the container size is correct. Maybe you can argue that it should > > reuse its layout opportunity's constraint space, but then you have to make the > > constraint space non-const (that's passed to Layout) > > Presumably layouts should create PhysicalConstraintSpaces and not views like > this? I don't think it makes sense for a layout to create a physical, was envisioning that they'd create a NGConstraintSpace which would get transformed inside performLayout(). https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:64: bool inlineTriggersScrollbar() const; On 2016/08/24 18:18:35, cbiesinger wrote: > Codestyle is to start with an uppercase letter; maybe change that while we're > touching this? Done & else where in file. https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:74: void setFixedSize(bool inlineFixed, bool blockFixed); On 2016/08/24 17:49:51, eae wrote: > Move setters to NGPhysicalConstraintSpace? So you typically want the setters in logical coords as this step typically happens in the current layout creating a new NGConstraintSpace for a child potentially. Would it make more sense to create a builder class? https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:81: void addExclusion(const NGExclusion, unsigned options = 0); On 2016/08/24 17:49:51, eae wrote: > Move to NGPhysicalConstraintSpace? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/24 19:17:41, ikilpatrick wrote: > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right): > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:24: const auto > constraintSpace = NGConstraintSpace::fromLayoutObject(*this); > On 2016/08/24 18:18:35, cbiesinger wrote: > > Please use auto* when this is for a pointer > > Done. > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:26: > box.layout(*constraintSpace); > On 2016/08/24 17:49:51, eae wrote: > > Mixing pointers and references like this is gross and leads to real bugs given > > the difference in behavior between the two. > > > > Can we change NGBox::layout to take a const pointer instead? > > Done. > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:13: > NGConstraintSpace* NGConstraintSpace::fromLayoutObject(const LayoutBox& box) { > On 2016/08/24 17:49:51, eae wrote: > > This probably belongs on NGPhysicalConstraintSpace. > > Makes more sense to build in logical coords? > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:28: > NGConstraintSpace(NGWritingMode writing_mode) > On 2016/08/24 17:49:51, eae wrote: > > Do we need this constructor? When would a NGConstraintSpace not be backed by > an > > actual NGPhysicalConstraintSpace? > > This is for the initial NGConstraintSpace (typically created by > NGConstraintSpace::fromLayoutObject) or for a "synthetic" constraint space (like > when you force a size on children). > > This just creates a backing NGPhysicalConstraintSpace for itself. > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:40: > physical_space_(new NGPhysicalConstraintSpace(*other.physical_space_)) { > On 2016/08/24 18:18:35, cbiesinger wrote: > > so a derived constraint space in other will become a non-derived space here? > > I didn't touch NGDerivedConstraintSpace in this patch as not entirely sure what > we need yet; > I think we'll want a NGDerivedPhysicalConstraintSpace to back the physical size > coords? But not sure. > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void > SetContainerSize(NGLogicalSize container_size); > On 2016/08/24 18:20:30, eae wrote: > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > On 2016/08/24 17:49:51, eae wrote: > > > > Shouldn't this be part of the constructor instead? Only layoutOpertunities > > > will > > > > ever create NGConstraintSpace instances, right? If so setting the size > > should > > > > probably be a part of that API. > > > > > > While I second the constructor comment, I don't think only > layoutOpportunities > > > will create constraint spaces. Each Layout() implementation will have to > > create > > > one so that the container size is correct. Maybe you can argue that it > should > > > reuse its layout opportunity's constraint space, but then you have to make > the > > > constraint space non-const (that's passed to Layout) > > > > Presumably layouts should create PhysicalConstraintSpaces and not views like > > this? > > I don't think it makes sense for a layout to create a physical, was envisioning > that they'd create a NGConstraintSpace which would get transformed inside > performLayout(). Ideally a layout should never create a constraint space but mutate an existing one or create a view into an existing one. For reverse hosting I can see it making sense as we need to represent the legacy-layout available space as a constraint space, that is in physical coordinates though. Perhaps I'm missing something here but outside of reverse hosting when would we ever want to create a new constraint space during layout?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
To clarify, the way I see it a layout would create a new derived constraint space from the existing (derived) constraint space and then pass when calling child->layout. The derived constraint space would have things like the offset and the restricted size, if applicable. Whether this is through a constructor that takes a (derived) constraint space and a set of limits, a helper method on (derived) constraint space, a helper on physical constraint space, or a part of layoutOpertunities is a little unclear. I'm leaning towards it being a helper on (derived) constraint space but don't feel strongly about it. What I am skeptical about though is having layout create entirely new constraint spaces. If we do that we'll loose track of exclusions and scroll triggering.
On 2016/08/24 20:01:47, eae wrote: > On 2016/08/24 19:17:41, ikilpatrick wrote: > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right): > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:24: const > auto > > constraintSpace = NGConstraintSpace::fromLayoutObject(*this); > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > Please use auto* when this is for a pointer > > > > Done. > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:26: > > box.layout(*constraintSpace); > > On 2016/08/24 17:49:51, eae wrote: > > > Mixing pointers and references like this is gross and leads to real bugs > given > > > the difference in behavior between the two. > > > > > > Can we change NGBox::layout to take a const pointer instead? > > > > Done. > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:13: > > NGConstraintSpace* NGConstraintSpace::fromLayoutObject(const LayoutBox& box) { > > On 2016/08/24 17:49:51, eae wrote: > > > This probably belongs on NGPhysicalConstraintSpace. > > > > Makes more sense to build in logical coords? > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:28: > > NGConstraintSpace(NGWritingMode writing_mode) > > On 2016/08/24 17:49:51, eae wrote: > > > Do we need this constructor? When would a NGConstraintSpace not be backed by > > an > > > actual NGPhysicalConstraintSpace? > > > > This is for the initial NGConstraintSpace (typically created by > > NGConstraintSpace::fromLayoutObject) or for a "synthetic" constraint space > (like > > when you force a size on children). > > > > This just creates a backing NGPhysicalConstraintSpace for itself. > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:40: > > physical_space_(new NGPhysicalConstraintSpace(*other.physical_space_)) { > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > so a derived constraint space in other will become a non-derived space here? > > > > I didn't touch NGDerivedConstraintSpace in this patch as not entirely sure > what > > we need yet; > > I think we'll want a NGDerivedPhysicalConstraintSpace to back the physical > size > > coords? But not sure. > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void > > SetContainerSize(NGLogicalSize container_size); > > On 2016/08/24 18:20:30, eae wrote: > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > On 2016/08/24 17:49:51, eae wrote: > > > > > Shouldn't this be part of the constructor instead? Only > layoutOpertunities > > > > will > > > > > ever create NGConstraintSpace instances, right? If so setting the size > > > should > > > > > probably be a part of that API. > > > > > > > > While I second the constructor comment, I don't think only > > layoutOpportunities > > > > will create constraint spaces. Each Layout() implementation will have to > > > create > > > > one so that the container size is correct. Maybe you can argue that it > > should > > > > reuse its layout opportunity's constraint space, but then you have to make > > the > > > > constraint space non-const (that's passed to Layout) > > > > > > Presumably layouts should create PhysicalConstraintSpaces and not views like > > > this? > > > > I don't think it makes sense for a layout to create a physical, was > envisioning > > that they'd create a NGConstraintSpace which would get transformed inside > > performLayout(). > > Ideally a layout should never create a constraint space but mutate an existing > one or create a view into an existing one. For reverse hosting I can see it > making sense as we need to represent the legacy-layout available space as a > constraint space, that is in physical coordinates though. > > Perhaps I'm missing something here but outside of reverse hosting when would we > ever want to create a new constraint space during layout? So one example is the stretch pass of flexbox, when you explicitly need a fragment to be a particular size. E.g. NGConstraintSpace* space = new NGConstraintSpace(currentLayoutWritingMode, childLogicalfixedSize); space.setFixedSize(true, true); flexChild->performLayout(space); an explicit builder for this might be better?
On 2016/08/24 20:18:39, ikilpatrick wrote: > On 2016/08/24 20:01:47, eae wrote: > > On 2016/08/24 19:17:41, ikilpatrick wrote: > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc > (right): > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:24: const > > auto > > > constraintSpace = NGConstraintSpace::fromLayoutObject(*this); > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > Please use auto* when this is for a pointer > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:26: > > > box.layout(*constraintSpace); > > > On 2016/08/24 17:49:51, eae wrote: > > > > Mixing pointers and references like this is gross and leads to real bugs > > given > > > > the difference in behavior between the two. > > > > > > > > Can we change NGBox::layout to take a const pointer instead? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc > (right): > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:13: > > > NGConstraintSpace* NGConstraintSpace::fromLayoutObject(const LayoutBox& box) > { > > > On 2016/08/24 17:49:51, eae wrote: > > > > This probably belongs on NGPhysicalConstraintSpace. > > > > > > Makes more sense to build in logical coords? > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:28: > > > NGConstraintSpace(NGWritingMode writing_mode) > > > On 2016/08/24 17:49:51, eae wrote: > > > > Do we need this constructor? When would a NGConstraintSpace not be backed > by > > > an > > > > actual NGPhysicalConstraintSpace? > > > > > > This is for the initial NGConstraintSpace (typically created by > > > NGConstraintSpace::fromLayoutObject) or for a "synthetic" constraint space > > (like > > > when you force a size on children). > > > > > > This just creates a backing NGPhysicalConstraintSpace for itself. > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:40: > > > physical_space_(new NGPhysicalConstraintSpace(*other.physical_space_)) { > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > so a derived constraint space in other will become a non-derived space > here? > > > > > > I didn't touch NGDerivedConstraintSpace in this patch as not entirely sure > > what > > > we need yet; > > > I think we'll want a NGDerivedPhysicalConstraintSpace to back the physical > > size > > > coords? But not sure. > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void > > > SetContainerSize(NGLogicalSize container_size); > > > On 2016/08/24 18:20:30, eae wrote: > > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > > On 2016/08/24 17:49:51, eae wrote: > > > > > > Shouldn't this be part of the constructor instead? Only > > layoutOpertunities > > > > > will > > > > > > ever create NGConstraintSpace instances, right? If so setting the size > > > > should > > > > > > probably be a part of that API. > > > > > > > > > > While I second the constructor comment, I don't think only > > > layoutOpportunities > > > > > will create constraint spaces. Each Layout() implementation will have to > > > > create > > > > > one so that the container size is correct. Maybe you can argue that it > > > should > > > > > reuse its layout opportunity's constraint space, but then you have to > make > > > the > > > > > constraint space non-const (that's passed to Layout) > > > > > > > > Presumably layouts should create PhysicalConstraintSpaces and not views > like > > > > this? > > > > > > I don't think it makes sense for a layout to create a physical, was > > envisioning > > > that they'd create a NGConstraintSpace which would get transformed inside > > > performLayout(). > > > > Ideally a layout should never create a constraint space but mutate an existing > > one or create a view into an existing one. For reverse hosting I can see it > > making sense as we need to represent the legacy-layout available space as a > > constraint space, that is in physical coordinates though. > > > > Perhaps I'm missing something here but outside of reverse hosting when would > we > > ever want to create a new constraint space during layout? > > So one example is the stretch pass of flexbox, when you explicitly need a > fragment to be a particular size. > > E.g. > NGConstraintSpace* space = new NGConstraintSpace(currentLayoutWritingMode, > childLogicalfixedSize); > space.setFixedSize(true, true); > flexChild->performLayout(space); > > an explicit builder for this might be better? Ah, forgot about the fixed use case. Presumably the same would hold true for intrinsic size as well. I still think that it would make sense to create that _from_ a derived constraint space for consistency. Even though it wouldn't necessarily retain any of the information from it in this case. NGDerrivedConstraintSpace* NGDerrivedConstraintSpace::ConstraintSpaceForChild(...) NGDerrivedConstraintSpace* NGDerrivedConstraintSpace::FixedSizeConstraitSpace(...) etc. I could be convinced otherwise though but given that the majority of constraint spaces will, presumably, retain properties of the parent I'd want a model where it does that by default and where it is very hard to accidentally break that link.
On 2016/08/24 20:24:20, eae wrote: > On 2016/08/24 20:18:39, ikilpatrick wrote: > > On 2016/08/24 20:01:47, eae wrote: > > > On 2016/08/24 19:17:41, ikilpatrick wrote: > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:24: const > > > auto > > > > constraintSpace = NGConstraintSpace::fromLayoutObject(*this); > > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > > Please use auto* when this is for a pointer > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:26: > > > > box.layout(*constraintSpace); > > > > On 2016/08/24 17:49:51, eae wrote: > > > > > Mixing pointers and references like this is gross and leads to real bugs > > > given > > > > > the difference in behavior between the two. > > > > > > > > > > Can we change NGBox::layout to take a const pointer instead? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:13: > > > > NGConstraintSpace* NGConstraintSpace::fromLayoutObject(const LayoutBox& > box) > > { > > > > On 2016/08/24 17:49:51, eae wrote: > > > > > This probably belongs on NGPhysicalConstraintSpace. > > > > > > > > Makes more sense to build in logical coords? > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:28: > > > > NGConstraintSpace(NGWritingMode writing_mode) > > > > On 2016/08/24 17:49:51, eae wrote: > > > > > Do we need this constructor? When would a NGConstraintSpace not be > backed > > by > > > > an > > > > > actual NGPhysicalConstraintSpace? > > > > > > > > This is for the initial NGConstraintSpace (typically created by > > > > NGConstraintSpace::fromLayoutObject) or for a "synthetic" constraint space > > > (like > > > > when you force a size on children). > > > > > > > > This just creates a backing NGPhysicalConstraintSpace for itself. > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:40: > > > > physical_space_(new NGPhysicalConstraintSpace(*other.physical_space_)) { > > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > > so a derived constraint space in other will become a non-derived space > > here? > > > > > > > > I didn't touch NGDerivedConstraintSpace in this patch as not entirely sure > > > what > > > > we need yet; > > > > I think we'll want a NGDerivedPhysicalConstraintSpace to back the physical > > > size > > > > coords? But not sure. > > > > > > > > > > > > > > https://codereview.chromium.org/2267383003/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:53: void > > > > SetContainerSize(NGLogicalSize container_size); > > > > On 2016/08/24 18:20:30, eae wrote: > > > > > On 2016/08/24 18:18:35, cbiesinger wrote: > > > > > > On 2016/08/24 17:49:51, eae wrote: > > > > > > > Shouldn't this be part of the constructor instead? Only > > > layoutOpertunities > > > > > > will > > > > > > > ever create NGConstraintSpace instances, right? If so setting the > size > > > > > should > > > > > > > probably be a part of that API. > > > > > > > > > > > > While I second the constructor comment, I don't think only > > > > layoutOpportunities > > > > > > will create constraint spaces. Each Layout() implementation will have > to > > > > > create > > > > > > one so that the container size is correct. Maybe you can argue that it > > > > should > > > > > > reuse its layout opportunity's constraint space, but then you have to > > make > > > > the > > > > > > constraint space non-const (that's passed to Layout) > > > > > > > > > > Presumably layouts should create PhysicalConstraintSpaces and not views > > like > > > > > this? > > > > > > > > I don't think it makes sense for a layout to create a physical, was > > > envisioning > > > > that they'd create a NGConstraintSpace which would get transformed inside > > > > performLayout(). > > > > > > Ideally a layout should never create a constraint space but mutate an > existing > > > one or create a view into an existing one. For reverse hosting I can see it > > > making sense as we need to represent the legacy-layout available space as a > > > constraint space, that is in physical coordinates though. > > > > > > Perhaps I'm missing something here but outside of reverse hosting when would > > we > > > ever want to create a new constraint space during layout? > > > > So one example is the stretch pass of flexbox, when you explicitly need a > > fragment to be a particular size. > > > > E.g. > > NGConstraintSpace* space = new NGConstraintSpace(currentLayoutWritingMode, > > childLogicalfixedSize); > > space.setFixedSize(true, true); > > flexChild->performLayout(space); > > > > an explicit builder for this might be better? > > Ah, forgot about the fixed use case. Presumably the same would hold true for > intrinsic size as well. > I still think that it would make sense to create that _from_ a derived > constraint space for consistency. Even though it wouldn't necessarily retain any > of the information from it in this case. So the only time this is valid is when a child is a formatting context; I.e. flex children etc. these don't inherit their parent constraints, we probably want to add a DCHECK for that (a "root" constraint space is only applicable for children which establish a formatting context). > NGDerrivedConstraintSpace* > NGDerrivedConstraintSpace::ConstraintSpaceForChild(...) > NGDerrivedConstraintSpace* > NGDerrivedConstraintSpace::FixedSizeConstraitSpace(...) > etc. > > I could be convinced otherwise though but given that the majority of constraint > spaces will, presumably, retain properties of the parent I'd want a model where > it does that by default and where it is very hard to accidentally break that > link. So i could buy into moving this to NGDerviedConstraintSpace for now; and having Create* methods for each of the cases. Christian: thoughts?
Sounds great to me, thanks for listening!
I'm actually not even sure that you need to create a new constraint space even for flex children, as long as layoutOpportunities return a mutable constraint space. Though, then again, maybe flexbox really doesn't want to use layout opportunities in the first place because it's such a different layout model?
Also, previous comments here made me wonder if the constraint space we pass to the child should be non-const? Then the child will set its computed size as the container size for passing this on to children. Good/bad idea?
On 2016/08/24 21:24:39, cbiesinger wrote: > Also, previous comments here made me wonder if the constraint space we pass to > the child should be non-const? Then the child will set its computed size as the > container size for passing this on to children. Good/bad idea? Either that or the child creates a copy. The downside of having it be mutable would be that passing the "same" constraint space to two children would produce different results (as the first child would modify it). That might not be a problem however.
On 2016/08/24 21:49:43, eae wrote: > On 2016/08/24 21:24:39, cbiesinger wrote: > > Also, previous comments here made me wonder if the constraint space we pass to > > the child should be non-const? Then the child will set its computed size as > the > > container size for passing this on to children. Good/bad idea? > > Either that or the child creates a copy. The downside of having it be mutable > would be that passing the "same" constraint space to two children would produce > different results (as the first child would modify it). That might not be a > problem however. Don't you have to call LayoutOpportunities() again for the next child anyway?
On 2016/08/24 22:03:10, cbiesinger wrote: > On 2016/08/24 21:49:43, eae wrote: > > On 2016/08/24 21:24:39, cbiesinger wrote: > > > Also, previous comments here made me wonder if the constraint space we pass > to > > > the child should be non-const? Then the child will set its computed size as > > the > > > container size for passing this on to children. Good/bad idea? > > > > Either that or the child creates a copy. The downside of having it be mutable > > would be that passing the "same" constraint space to two children would > produce > > different results (as the first child would modify it). That might not be a > > problem however. > > Don't you have to call LayoutOpportunities() again for the next child anyway? I *think* so.
So the NGDerivedConstraintSpace will require follow up patches to get offset, etc working correctly; but at the moment it just has the FromLayoutObject and a public constructor where you can set everything initially. I've kept the setters on NGConstriantSpace as protected as didn't want to spread that logical->physical conversion around everythere.
https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_box.cc:29: NGConstraintSpace* childConstraintSpace = new NGConstraintSpace( For new code, can we make sure to follow the Google style guide for naming? I.e. child_constraint_space, FromPlatformWritingMode. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:27: ? NGLogicalSize(physical_space_->container_size_.width, could just make this line be physical_space_->container_size_, right? https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:29: : NGLogicalSize(physical_space_->container_size_.height, I wonder if it would help to offer a .Transposed() method on NGLogicalSize. May not be worth it. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:107: DCHECK(physical_space_->width_direction_fragmentation_type_ == DCHECK_EQ? https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:90: unsigned writing_mode_ : 3; Prefer bitfields at the end of the class for better packing https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:17: true, false, true, false, FragmentColumn); That list of true and false is so hard to understand :/ Eventually we may want to switch those to enums. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_derived_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_derived_constraint_space.cc:14: bool fixedInline = false, fixedBlock = false; fixed_inline, fixed_block https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_length_utils_test.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_length_utils_test.cc:24: static NGConstraintSpace* constructConstraintSpace(int inline_size, Maybe rename to ConstructConstraintSpace while you're touching it https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc:21: const NGPhysicalConstraintSpace& other) You're not copying the exclusions...? https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.h:46: void addExclusion(const NGExclusion, unsigned options = 0); Maybe name AddExclusion()/Exlusions() while we're not having any callers yet https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:21: CORE_EXPORT NGWritingMode fromPlatformWritingMode(WritingMode); FromPlatformWritingMode
LGTM
The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run
https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_box.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_box.cc:29: NGConstraintSpace* childConstraintSpace = new NGConstraintSpace( On 2016/08/25 22:22:15, cbiesinger wrote: > For new code, can we make sure to follow the Google style guide for naming? I.e. > child_constraint_space, FromPlatformWritingMode. Yup done. Sorry, hopefully wont take that long to change habits :) https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:27: ? NGLogicalSize(physical_space_->container_size_.width, On 2016/08/25 22:22:16, cbiesinger wrote: > could just make this line be physical_space_->container_size_, right? Need explicit conversion between NGPhysicalSize and NGLogicalSize? https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:29: : NGLogicalSize(physical_space_->container_size_.height, On 2016/08/25 22:22:16, cbiesinger wrote: > I wonder if it would help to offer a .Transposed() method on NGLogicalSize. May > not be worth it. Yeah i thought about it, and decided could extract out later if needed. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:107: DCHECK(physical_space_->width_direction_fragmentation_type_ == On 2016/08/25 22:22:15, cbiesinger wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:90: unsigned writing_mode_ : 3; On 2016/08/25 22:22:16, cbiesinger wrote: > Prefer bitfields at the end of the class for better packing Done. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc:17: true, false, true, false, FragmentColumn); On 2016/08/25 22:22:16, cbiesinger wrote: > That list of true and false is so hard to understand :/ Eventually we may want > to switch those to enums. +1. Ack. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_derived_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_derived_constraint_space.cc:14: bool fixedInline = false, fixedBlock = false; On 2016/08/25 22:22:16, cbiesinger wrote: > fixed_inline, fixed_block Done. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_length_utils_test.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_length_utils_test.cc:24: static NGConstraintSpace* constructConstraintSpace(int inline_size, On 2016/08/25 22:22:16, cbiesinger wrote: > Maybe rename to ConstructConstraintSpace while you're touching it Done. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc:21: const NGPhysicalConstraintSpace& other) On 2016/08/25 22:22:16, cbiesinger wrote: > You're not copying the exclusions...? I don't think this makes sense as for a NGDerivedCS we'll want to explicitly add exclusions to the parent. E.g. NGDerivedCS::AddExclusion(ex) { if (parent_) { parent_->AddExclusion(ex); return; } NGCS::AddExclusion(ex); } I.e. there should only be one source of truth? Alt: could store a ptr to a vector instead? https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.h (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.h:46: void addExclusion(const NGExclusion, unsigned options = 0); On 2016/08/25 22:22:16, cbiesinger wrote: > Maybe name AddExclusion()/Exlusions() while we're not having any callers yet Done. https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:21: CORE_EXPORT NGWritingMode fromPlatformWritingMode(WritingMode); On 2016/08/25 22:22:16, cbiesinger wrote: > FromPlatformWritingMode Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:27: ? NGLogicalSize(physical_space_->container_size_.width, On 2016/08/25 23:14:29, ikilpatrick wrote: > On 2016/08/25 22:22:16, cbiesinger wrote: > > could just make this line be physical_space_->container_size_, right? > > Need explicit conversion between NGPhysicalSize and NGLogicalSize? Er, oops. Of course, thanks for reminding me! https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc:21: const NGPhysicalConstraintSpace& other) On 2016/08/25 23:14:29, ikilpatrick wrote: > On 2016/08/25 22:22:16, cbiesinger wrote: > > You're not copying the exclusions...? > > I don't think this makes sense as for a NGDerivedCS we'll want to explicitly add > exclusions to the parent. E.g. > > NGDerivedCS::AddExclusion(ex) { > if (parent_) { > parent_->AddExclusion(ex); > return; > } > > NGCS::AddExclusion(ex); > } > > I.e. there should only be one source of truth? > > Alt: could store a ptr to a vector instead? Oh right. Can you add a comment explaining that, so that I won't forget again? :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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...
The CQ bit was checked by eae@chromium.org
The CQ bit was unchecked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org, cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2267383003/#ps160001 (title: "fix win compile?")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc (right): https://codereview.chromium.org/2267383003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_physical_constraint_space.cc:21: const NGPhysicalConstraintSpace& other) On 2016/08/25 23:17:59, cbiesinger wrote: > On 2016/08/25 23:14:29, ikilpatrick wrote: > > On 2016/08/25 22:22:16, cbiesinger wrote: > > > You're not copying the exclusions...? > > > > I don't think this makes sense as for a NGDerivedCS we'll want to explicitly > add > > exclusions to the parent. E.g. > > > > NGDerivedCS::AddExclusion(ex) { > > if (parent_) { > > parent_->AddExclusion(ex); > > return; > > } > > > > NGCS::AddExclusion(ex); > > } > > > > I.e. there should only be one source of truth? > > > > Alt: could store a ptr to a vector instead? > > Oh right. Can you add a comment explaining that, so that I won't forget again? > :-) Added comment to derived constraint space. :)
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 ========== to ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 ========== to ========== [LayoutNG] Introduces NGPhysicalConstraintSpace and makes NGConstraintSpace a "view". NGConstraintSpace now has a backing NGPhysicalConstaintSpace and provides abstract coordinate system accessors for everything. Makes NG*ConstraintSpace GarbagedCollected as well as we'll need this once we move to a state machine / for caching fragment results. BUG=635619 Committed: https://crrev.com/6f16535150edfba413105be7327ab4bb3ce630d7 Cr-Commit-Position: refs/heads/master@{#414745} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6f16535150edfba413105be7327ab4bb3ce630d7 Cr-Commit-Position: refs/heads/master@{#414745} |
