|
|
Created:
4 years, 4 months ago by eae Modified:
4 years, 4 months ago Reviewers:
ikilpatrick CC:
chromium-reviews, 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] Add NGConstraintSpace class definition
Define initial outline of the constraint space API for LayoutNG by means
of the NG[Derived]ConstraintSpace & NGLayoutOpportunityIterator classes.
BUG=591099
R=ikilpatrick@chromium.org
Committed: https://crrev.com/d32fdb7a2428764717bddae953628b07eba2afdd
Cr-Commit-Position: refs/heads/master@{#409035}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address reviewer comments #Patch Set 3 : Add NGLayoutOpportunityIterator #
Total comments: 9
Patch Set 4 : test #Patch Set 5 : Rebase w/HEAD #
Messages
Total messages: 27 (17 generated)
https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h (right): https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:16: class PLATFORM_EXPORT NGConstraintSpace { CORE_EXPORT:) https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:16: class PLATFORM_EXPORT NGConstraintSpace { We'll also want to think about how this will work when passing into a child which has a different writing mode. E.g. <div> <div style="writing-mode: vertical; height: 10px"> a b c d </div> </div> // Here inline is childDerviedConstraintSpace.blockSize is 100px say. child.performLayout(childDerivedConstraintSpace); // Inside layout fn this should map over to inlineSize right? NGChildLayout::layout(ConstraintSpace cs) { cs.inlineSize() } Thought 1: between performLayout & layout we wrap up a "view" object. class ConstraintSpaceView? : public ConstraintSpace { public: ConstraintSpaceView(ConstraintSpace, Direction); // redirect all inline/block calls to correct... // ... or crazyer is not allowed to query for these unless going through a view or something. }; Thought 2: All constraint spaces need a Direction. But this is confusing in the call side code? VERY RAW THOUGHTS. https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:56: // The size threashold in the inline and block directions respesvitely that respectively https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:61: LayoutUnit blockOverflowSize() const; Same as below... any time when there would be different to inline/blockSize? https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:65: LayoutUnit blockFragmentationSize() const; So when working on the spec I couldn't think of a case where this was different than the blockSize. Are there any? If so we can just get away with Fragmentation type below. https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:71: List<const DerivedNGConstraintSpace> layoutOpportunities( TODO(layout-dev): move to generator eventually? https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:73: NGClearExclusion = ExcludeNone) const; Do we have any place today where the "step"/"jump-size" is different than 1em? May want to pass that info in as well (not needed atm). Would also have to teach it about what 1em means... https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:73: NGClearExclusion = ExcludeNone) const; Is NGClearExclusion meant to be "avoid"?
Thanks! https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h (right): https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:16: class PLATFORM_EXPORT NGConstraintSpace { On 2016/07/27 21:14:12, ikilpatrick wrote: > CORE_EXPORT:) Done. https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:16: class PLATFORM_EXPORT NGConstraintSpace { On 2016/07/27 21:14:12, ikilpatrick wrote: > We'll also want to think about how this will work when passing into a child > which has a different writing mode. > > E.g. > <div> > <div style="writing-mode: vertical; height: 10px"> > a b c d > </div> > </div> > > // Here inline is childDerviedConstraintSpace.blockSize is 100px say. > child.performLayout(childDerivedConstraintSpace); > > // Inside layout fn this should map over to inlineSize right? > NGChildLayout::layout(ConstraintSpace cs) > { > cs.inlineSize() > } > > Thought 1: between performLayout & layout we wrap up a "view" object. > > class ConstraintSpaceView? : public ConstraintSpace { > public: > ConstraintSpaceView(ConstraintSpace, Direction); > > // redirect all inline/block calls to correct... > // ... or crazyer is not allowed to query for these unless going through a > view or something. > }; > > Thought 2: All constraint spaces need a Direction. But this is confusing in the > call side code? > > VERY RAW THOUGHTS. I'd argue that we should have constraint spaces be direction agnostic either by having a view object or by having some shared convenience methods for layout methods. Either way, the constraint space itself shouldn't have to be aware of writing modes or directionality. https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:61: LayoutUnit blockOverflowSize() const; On 2016/07/27 21:14:12, ikilpatrick wrote: > Same as below... any time when there would be different to inline/blockSize? Yes. For a regular document the body would have an infinity blockSize but the blockOverflowSize would be the height of the inner viewport. Similarly for fixed-size elements with overflow auto. <div style="height: 200px; overflow: auto"> <p>Lots of content</p> </div> When laying out the paragraph the blockSize would be Infinite while the overflowBlockSize would be 200. https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:65: LayoutUnit blockFragmentationSize() const; On 2016/07/27 21:14:11, ikilpatrick wrote: > So when working on the spec I couldn't think of a case where this was different > than the blockSize. Are there any? > > If so we can just get away with Fragmentation type below. None comes to mind. I'll remove it. https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:73: NGClearExclusion = ExcludeNone) const; On 2016/07/27 21:14:12, ikilpatrick wrote: > Is NGClearExclusion meant to be "avoid"? > Yeah https://codereview.chromium.org/2176343006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:73: NGClearExclusion = ExcludeNone) const; On 2016/07/27 21:14:12, ikilpatrick wrote: > Do we have any place today where the "step"/"jump-size" is different than 1em? > > May want to pass that info in as well (not needed atm). > Would also have to teach it about what 1em means... Not today. I intentionally skipped the step-size argument for now.
Description was changed from ========== [LayoutNG] Add NGConstraintSpace class definition First draft of the constraint space definition. BUG=591099 R=ikilpatrick@chromium.org ========== to ========== [LayoutNG] Add NGConstraintSpace class definition Define initial outline of the constraint space API for LayoutNG by means of the NG[Derived]ConstraintSpace & NGLayoutOpportunityIterator classes. BUG=591099 R=ikilpatrick@chromium.org ==========
The CQ bit was checked by eae@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by eae@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: This issue passed the CQ dry run.
Broadly lgtm. We can polish/fixup as required. https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h (right): https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:19: enum NGClearExclusion { probably NGExclusionTag or NGExclusionType better? https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:33: enum NGExclusionType { NGExclusionExcludeType? or... NGExclusionFlowType? https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:97: LayoutUnit m_blockOverflowSize; I'm pretty sure we only need: m_{block,inline}Size m_percentage{Block,Inline}Size and bitfields: m_fixed{Block,Inline} m_scrollTrigger{Block,Inline} m_blockFragmentationType. But can discuss in person on Tues if you like. https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:136: // resolution to work correctly. I think we'll need this on the actual NGConstraintSpace b/c: 1) max-content calculation sizeForPercentageRes == 0, other sizes == Inf. 2) Passing derived NGCS into child will be common and will resolve its size against this instead of inlineSize().
Thanks! https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h (right): https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:19: enum NGClearExclusion { On 2016/07/29 17:18:44, ikilpatrick wrote: > probably NGExclusionTag or NGExclusionType better? Good idea. NGExclusionType it is. https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:33: enum NGExclusionType { On 2016/07/29 17:18:44, ikilpatrick wrote: > NGExclusionExcludeType? or... NGExclusionFlowType? Done. https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:97: LayoutUnit m_blockOverflowSize; On 2016/07/29 17:18:44, ikilpatrick wrote: > I'm pretty sure we only need: > m_{block,inline}Size > m_percentage{Block,Inline}Size > > and bitfields: > m_fixed{Block,Inline} > m_scrollTrigger{Block,Inline} > m_blockFragmentationType. > > But can discuss in person on Tues if you like. OK, will land as is but let's discuss this next week. https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:136: // resolution to work correctly. On 2016/07/29 17:18:44, ikilpatrick wrote: > I think we'll need this on the actual NGConstraintSpace b/c: > 1) max-content calculation sizeForPercentageRes == 0, other sizes == Inf. For NGConstraintSpace wouldn't inlineSizeForPercentageResolution and inlineSize be guaranteed to always be the same (and the same for block)? > 2) Passing derived NGCS into child will be common and will resolve its size > against this instead of inlineSize(). Right so having it on the derived CS should suffice. Or am I missing something here?
https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h (right): https://codereview.chromium.org/2176343006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/NGConstraintSpace.h:136: // resolution to work correctly. On 2016/07/29 17:41:02, eae wrote: > On 2016/07/29 17:18:44, ikilpatrick wrote: > > I think we'll need this on the actual NGConstraintSpace b/c: > > 1) max-content calculation sizeForPercentageRes == 0, other sizes == Inf. > > > For NGConstraintSpace wouldn't inlineSizeForPercentageResolution and inlineSize > be guaranteed to always be the same (and the same for block)? We'll need this for percentage height resolution in quirks mode right? (I'm not sure what happens when changing writing mode in quirks mode). > > 2) Passing derived NGCS into child will be common and will resolve its size > > against this instead of inlineSize(). > > Right so having it on the derived CS should suffice. Or am I missing something > here? This is tied to in person discussion above, but can discuss then :)
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2176343006/#ps80001 (title: "test")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2176343006/#ps100001 (title: "Rebase w/HEAD")
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.
Description was changed from ========== [LayoutNG] Add NGConstraintSpace class definition Define initial outline of the constraint space API for LayoutNG by means of the NG[Derived]ConstraintSpace & NGLayoutOpportunityIterator classes. BUG=591099 R=ikilpatrick@chromium.org ========== to ========== [LayoutNG] Add NGConstraintSpace class definition Define initial outline of the constraint space API for LayoutNG by means of the NG[Derived]ConstraintSpace & NGLayoutOpportunityIterator classes. BUG=591099 R=ikilpatrick@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Add NGConstraintSpace class definition Define initial outline of the constraint space API for LayoutNG by means of the NG[Derived]ConstraintSpace & NGLayoutOpportunityIterator classes. BUG=591099 R=ikilpatrick@chromium.org ========== to ========== [LayoutNG] Add NGConstraintSpace class definition Define initial outline of the constraint space API for LayoutNG by means of the NG[Derived]ConstraintSpace & NGLayoutOpportunityIterator classes. BUG=591099 R=ikilpatrick@chromium.org Committed: https://crrev.com/d32fdb7a2428764717bddae953628b07eba2afdd Cr-Commit-Position: refs/heads/master@{#409035} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d32fdb7a2428764717bddae953628b07eba2afdd Cr-Commit-Position: refs/heads/master@{#409035} |