|
|
Created:
4 years, 1 month ago by atotic 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. |
DescriptionAbsolutePositioning utilities with tests
These are the routines that implement absolute positioning specification.
ComputeAbsoluteHorizontal and ComputeAbsoluteVertical implement
the spec. They will be used by block_algorithm to position abspos elements.
BUG=635619
Committed: https://crrev.com/5836e40a4c5491a4b7351a8f0835f3a4d36c92d8
Cr-Commit-Position: refs/heads/master@{#433402}
Patch Set 1 #
Total comments: 14
Patch Set 2 : CR fix + %size test #Patch Set 3 : fix box-sizing, take child writing-mode into account #
Total comments: 3
Patch Set 4 : border-box, and slimmer position #Patch Set 5 : remove LOG() #
Total comments: 74
Patch Set 6 : CR fixes #
Total comments: 22
Patch Set 7 : CR fixes #
Total comments: 9
Patch Set 8 : CR fixes #
Total comments: 3
Patch Set 9 : Final CR fixes #
Messages
Total messages: 27 (5 generated)
Description was changed from ========== AbsolutePositioning utilities with tests These are the routines that implement absolute positioning specification. ComputeAbsoluteHorizontal and ComputeAbsoluteVertical implement the spec. They will be used by block_algorithm to position abspos elements. BUG=635619 ========== to ========== AbsolutePositioning utilities with tests These are the routines that implement absolute positioning specification. ComputeAbsoluteHorizontal and ComputeAbsoluteVertical implement the spec. They will be used by block_algorithm to position abspos elements. BUG=635619 ==========
atotic@chromium.org changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, ikilpatrick@chromium.org
ptal. I am using these for abspos positioning, coming up soon.
https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:107: LayoutUnit border_padding = result.border.left + result.border.right + do we need to include the border & padding here? If the output of this function is being used to set a constraint space FixedInline & FixedBlock then this should just be computing the border-box size directly? https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:233: CORE_EXPORT void ComputeAbsoluteVertical( is there a reason this isn't being done in logical coordinate space initially? (Same with ComputeAbsoluteHorizontal) https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:244: LOG(INFO) << "before vertical: " << result->ToString(); remove LOG(INFO) https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:248: // standard: "If all three of top, height, and bottom are auto:" what does standard mean here? https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:41: CORE_EXPORT bool AbsoluteHorizontalNeedsMinMax(const ComputedStyle& style); add newline below.
comments addressed. I've also added a test for % resolution, which was missing in original tests. https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:107: LayoutUnit border_padding = result.border.left + result.border.right + On 2016/11/14 at 19:23:27, ikilpatrick wrote: > do we need to include the border & padding here? > > If the output of this function is being used to set a constraint space FixedInline & FixedBlock then this should just be computing the border-box size directly? These functions just compute the spec. The caller decides what to do with the spec output. This keeps spec logic separate from "how we use this to generate layout" https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:233: CORE_EXPORT void ComputeAbsoluteVertical( On 2016/11/14 at 19:23:27, ikilpatrick wrote: > is there a reason this isn't being done in logical coordinate space initially? > > (Same with ComputeAbsoluteHorizontal) The abspos spec operates in the physical space. An absolutely positioned 10x100 rect visually looks the same inside a horizontal-tb, and vertical-lr container. If these routines were operating in logical space, there would be two conversions, one to convert from logical to physical before algorithm, then back to logical after algo runs. The caller can convert output to Logical with a single call. Another option is for ComputeAbsoluteVertical to return Logical result. https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:244: LOG(INFO) << "before vertical: " << result->ToString(); On 2016/11/14 at 19:23:27, ikilpatrick wrote: > remove LOG(INFO) Oops. Done. https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:248: // standard: "If all three of top, height, and bottom are auto:" On 2016/11/14 at 19:23:27, ikilpatrick wrote: > what does standard mean here? means that style is: "top:auto;height:auto;bottom:auto;" Does wording need change? https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:41: CORE_EXPORT bool AbsoluteHorizontalNeedsMinMax(const ComputedStyle& style); On 2016/11/14 at 19:23:28, ikilpatrick wrote: > add newline below. done
https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:107: LayoutUnit border_padding = result.border.left + result.border.right + On 2016/11/14 20:10:12, atotic2 wrote: > On 2016/11/14 at 19:23:27, ikilpatrick wrote: > > do we need to include the border & padding here? > > > > If the output of this function is being used to set a constraint space > FixedInline & FixedBlock then this should just be computing the border-box size > directly? > > These functions just compute the spec. The caller decides what to do with the > spec output. > > This keeps spec logic separate from "how we use this to generate layout" Unless we need this I'd prefer to keep it out and specify that this gives answers in border-box size. https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:233: CORE_EXPORT void ComputeAbsoluteVertical( On 2016/11/14 20:10:12, atotic2 wrote: > On 2016/11/14 at 19:23:27, ikilpatrick wrote: > > is there a reason this isn't being done in logical coordinate space initially? > > > > (Same with ComputeAbsoluteHorizontal) > > The abspos spec operates in the physical space. An absolutely positioned 10x100 > rect visually looks the same inside a horizontal-tb, and vertical-lr container. > > If these routines were operating in logical space, there would be two > conversions, one to convert from logical to physical before algorithm, then back > to logical after algo runs. > > The caller can convert output to Logical with a single call. > > Another option is for ComputeAbsoluteVertical to return Logical result. So the problem here is that Horizontal doesn't always have min_max_sizes (only needed for inline calculation) and similarly for Vertical, e.g. https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E... I think these calculations need to be performed in the child's writing-mode?, then reported in the parents writing mode? Christian does that match your mental model?
https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:107: LayoutUnit border_padding = result.border.left + result.border.right + On 2016/11/14 at 20:37:12, ikilpatrick wrote: > On 2016/11/14 20:10:12, atotic2 wrote: > > On 2016/11/14 at 19:23:27, ikilpatrick wrote: > > > do we need to include the border & padding here? > > > > > > If the output of this function is being used to set a constraint space > > FixedInline & FixedBlock then this should just be computing the border-box size > > directly? > > > > These functions just compute the spec. The caller decides what to do with the > > spec output. > > > > This keeps spec logic separate from "how we use this to generate layout" > > Unless we need this I'd prefer to keep it out and specify that this gives answers in border-box size. would adding border_box_size to NGAbsolutePhysicalPosition be ok? plain width is needed for height computation. https://codereview.chromium.org/2497863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:233: CORE_EXPORT void ComputeAbsoluteVertical( This made me look at my code again and find a bug: NGAbsolutePhysicalPosition::Init always computes pixel values based on InlineSize. It should be BlockSize for verticals. Fix is on its way.
Fixes for 2 things: ### Take child writing mode into account for estimated width height. This one was a major rework. The problem is that child writing mode determines the order in which absolute width/height can be computed. In horizontal_tb, you have to: 1) estimate childs inline_dimension 2) compute abs widths (which depend on child inline_dimension) 3) estimate child block_dimensions (which depend on abs widths) 4) compute abs heights In vertical-lr, you do: 1) estimate childs inline_dimension 2) compute abs heights 3) estimate child block_dimensions 4) compute abs widths To do this, ComputeAbsoluteHorizontal and ComputeAbsoluteVertical are passed through layer of indirection ComputePartialAbsoluteWithChildInlineSize (computes step 2) ComputeFullAbsoluteWithChildBlockSize (computes step 4) ### Take box-sizing into account - Minor fix I am not sure if everything is perfect, but I think the API looks reasonable. Some superficial tests look good. content_shell testing is tricky, because abspos algorithm is still work in progress, and I hit asserts. I'd like to continue polishing this as I progress with abspos algorithm. There is one big API unknown, static_position, and we'll have to talk about it in person. I just realized that if top/left are left blank, they default to element's original static position. Yay for specs pointing this out early NOT!
https://codereview.chromium.org/2497863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:28: void ComputeAbsoluteHorizontal(const NGConstraintSpace& space, are we able to split this out into two separate things? E.g. LayoutUnit ComputeAbsoluteSize(const NGConstraintSpace& space, const ComputedStyle& style, const Optional<LayoutUnit> auto_size) { if (use_auto_size /* from above */) { DCHECK(auto_size); return auto_size; } else { const insets = ComputeAbsoluteInsets(space, style); return space_size - insets.Sum() - margins.Sum(); } } and NGEdge/NGInset ComputeAbsoluteInsets(const NGConstraintSpace& space, const ComputedStyle& style, const Optional<LayoutUnit> auto_size) { // As before but DCHECKS that we don't reach size branches. } https://codereview.chromium.org/2497863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:133: ASSERT((container_size.width - border_padding - p->inset.left - the sizes (inline_size, block_size) are going to be directly used with SetFixedInlineSize/Block for the constraint space right? I'd prefer if we just compute the border-box directly instead of having to re-add the border_padding back on later. https://codereview.chromium.org/2497863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:267: inset.left = CustomValueForLength(style.left(), inline_size); We should only really be using the insets and size as outputs right? I dislike reusing the result of the first pass for the second pass, it makes it harder to follow in certain circumstances. If we do above, we can add a todo to add in the memorization for the insets if we need later; i don't think we need this optimization today.
ptal CR fixes: ComputeVertical/Horizontal now use border-box size NGAbsoluteLogicalPosition only stores size, and insets.
lgtm with nits below. We can refine later if necessary. As a general comment, I am not sure what your plans are for this but just to be clear, you should not need to lay out the box twice. You always know the logical width ahead of time. And for the logical height, you either know it ahead of time *or* it is the height that Layout will compute for you (and you only need to set the resulting position). Right? Do I understand it correctly that you would use these helpers somewhat like this? Optional<LayoutUnit> inline_size; if (AbsoluteNeedsChildInlineSize()) inline_size = child.ComputeMinAndMax().ShrinkToFit(); AbsolutePosition partial_abs = ComputePartialAbsoluteWithChildInlineSize(inline_size); if (AbsoluteNeedsChildBlockSize()) { Fragment f = child.Layout(new ConstraintSpace(partial_abs)); AbsolutePosition pos = ComputeAbsoluteWithChildBlockSize(f.block_size); ApplyPosition(f, pos); } else { AbsolutePosition pos = ComputeAbsoluteWithChildBlockSize(f); f = child.Layout(new ConstraintSpace(pos.size)); ApplyPosition(f, pos); } Is that about right? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:21: return valueForLength(length, max_value); I'm not sure this does something reasonable when length is a percentage value and max_value is NGSizeIndefinite, maybe add a DCHECK if that can never happen? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:40: // Always use inline_size for percent resolution. Well, that's only true for border/padding, for top/bottom/height you would use the vertical dimension. But since this function only does horizontal this should be fine. (Not sure how this works for vertical writing modes, offhand) https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:45: style.paddingLeft(), space_inline_size, LayoutUnit(0)); Just use LayoutUnit(), wherever you need a 0 (slightly more efficient) https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:61: width = width + border_padding; += https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:87: if (left != NGSizeIndefinite && right != NGSizeIndefinite && I think it will be clearer to make this an "else if" https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:131: ASSERT(right != NGSizeIndefinite); DCHECK_NE https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:136: ASSERT(width != NGSizeIndefinite); DCHECK_NE https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:155: DCHECK((container_size.width - left - right - margin_left - margin_right - DCHECK_EQ. I might write it as: DCHECK_EQ(container_size.width, left + right + margin_left + margin_right + width); https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:159: width = std::max(width, LayoutUnit(0)); You need max(width, border_and_padding) -- that's an assumption we have in various places in our code, though we occasionally violate it :( https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:168: void ComputeAbsoluteVertical(const NGConstraintSpace& space, (please make the corresponding changes here, like for the horizontal version) https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:298: // NGLogicalSize NGAbsoluteLogicalPosition::BorderBoxAdjustedSize() { ? What's up with this commented-out function? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:18: struct CORE_EXPORT NGAbsoluteLogicalPosition { I'm OK with this name but not super-excited -- it describes the logical position for an abspos (or fixed-pos) element; it does not describe an absolute position for something
On 2016/11/18 at 17:54:20, cbiesinger wrote: > As a general comment, I am not sure what your plans are for this but just to be clear, you should not need to lay out the box twice. You always know the logical width ahead of time. And for the logical height, you either know it ahead of time *or* it is the height that Layout will compute for you (and you only need to set the resulting position). Right? > > Do I understand it correctly that you would use these helpers somewhat like this? > > Optional<LayoutUnit> inline_size; > if (AbsoluteNeedsChildInlineSize()) > inline_size = child.ComputeMinAndMax().ShrinkToFit(); > AbsolutePosition partial_abs = ComputePartialAbsoluteWithChildInlineSize(inline_size); > if (AbsoluteNeedsChildBlockSize()) { > Fragment f = child.Layout(new ConstraintSpace(partial_abs)); > AbsolutePosition pos = ComputeAbsoluteWithChildBlockSize(f.block_size); > ApplyPosition(f, pos); > } else { > AbsolutePosition pos = ComputeAbsoluteWithChildBlockSize(f); > f = child.Layout(new ConstraintSpace(pos.size)); > ApplyPosition(f, pos); > } Exactly! Aleks
i'd like to have one more pass over this, after comments are fixed if that's ok. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:16: LayoutUnit CustomValueForLength(const Length& length, add comment for what this is doing. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:41: LayoutUnit space_inline_size = space.PercentageResolutionSize().inline_size; use 'percentage_resolution_size' same as ng_length_utils.cc https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:56: LayoutUnit width = CustomValueForLength(style.width(), space_inline_size); add TODO(atotic): This will resolve size for fit-content incorrectly. ( I believe?) https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:58: // Normalize width to border-box Normalize width to border-box sizing. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:111: // are values overconstrained? captial 'A' https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:173: space.Size().ConvertToPhysical(space.WritingMode()); should this be available_size ? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:177: LayoutUnit space_block_size = space.PercentageResolutionSize().block_size; percent_size similar to ng_length_utils.cc https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:234: // are values overconstrained? make all of the comments in this function consistent with above. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:251: ASSERT(bottom != NGSizeIndefinite); DCHECK_NE https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:256: ASSERT(height != NGSizeIndefinite); DCHECK_NE https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:271: ASSERT((container_size.height - top - bottom - margin_top - margin_bottom - DCHECK_EQ https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:327: NGAbsolutePhysicalPosition ComputePartialAbsoluteWithChildInlineSize( it might be cleaner to expect the caller to provide the NGAbsolutePhysicalPosition instead of setting it, e.g. child_position_ = {}; ComputePartialAbsoluteWithChildInlineSize(..., &child_position_) for consistency. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:18: struct CORE_EXPORT NGAbsoluteLogicalPosition { do we actually need this class? Most of the time we'll just be doing: physical_pos.size.ConvertToLogical() right? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:19: NGBoxStrut inset; // top, right, bottom, left remove comment? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:27: // Initializes values from style. remove. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:33: // The following routines implement absolute size resolution algorithm nit. (everywhere) Use proper punctuation everywhere, e.g. commas, fullstops etc. https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:43: // layout out block inside estimated_inline_size If AbsoluteNeedChildBlockSize is true, compute the estimated block_size by performing layout with the inline_size calculated from (2). or similar. at this stage it isn't using the estimated_inline_size, it's resolved. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:45: // upon child's block_size' remove ' add full stop. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:48: // estimated inline size full stop. & elsewhere. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:95: logging::SetLogItems(false, false, false, false); ? https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:133: // all auto => width is estimated_inline, left is 0 capital 'A' & elsewhere. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:239: // remove line. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:276: LayoutUnit top(5); should these be of the form kTop ? not sure. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:366: // Rule 4: top is unknonwn spelling. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:373: // Rule 5: bottom is unknonwn spelling.
all CR comments addressed. ptal. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:16: LayoutUnit CustomValueForLength(const Length& length, On 2016/11/18 at 20:00:23, ikilpatrick wrote: > add comment for what this is doing. done. // Extends Length::valueForLength with default value if length type is auto. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:21: return valueForLength(length, max_value); On 2016/11/18 at 17:54:20, cbiesinger wrote: > I'm not sure this does something reasonable when length is a percentage value and max_value is NGSizeIndefinite, maybe add a DCHECK if that can never happen? Done. I've thought about this. I don't know how ConstraintSpaces derive their size, so I do not know when NGSizeIndefinite might happen. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:40: // Always use inline_size for percent resolution. On 2016/11/18 at 17:54:20, cbiesinger wrote: > Well, that's only true for border/padding, for top/bottom/height you would use the vertical dimension. But since this function only does horizontal this should be fine. (Not sure how this works for vertical writing modes, offhand) I've added a TODO comment for vertical writing modes. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:41: LayoutUnit space_inline_size = space.PercentageResolutionSize().inline_size; On 2016/11/18 at 20:00:22, ikilpatrick wrote: > use 'percentage_resolution_size' same as ng_length_utils.cc done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:45: style.paddingLeft(), space_inline_size, LayoutUnit(0)); On 2016/11/18 at 17:54:20, cbiesinger wrote: > Just use LayoutUnit(), wherever you need a 0 (slightly more efficient) Done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:56: LayoutUnit width = CustomValueForLength(style.width(), space_inline_size); On 2016/11/18 at 20:00:23, ikilpatrick wrote: > add TODO(atotic): This will resolve size for fit-content incorrectly. > > ( I believe?) Good catch. I'll have to fix this, and add a test case. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:58: // Normalize width to border-box On 2016/11/18 at 20:00:23, ikilpatrick wrote: > Normalize width to border-box sizing. done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:61: width = width + border_padding; On 2016/11/18 at 17:54:20, cbiesinger wrote: > += Done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:87: if (left != NGSizeIndefinite && right != NGSizeIndefinite && On 2016/11/18 at 17:54:20, cbiesinger wrote: > I think it will be clearer to make this an "else if" done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:111: // are values overconstrained? On 2016/11/18 at 20:00:23, ikilpatrick wrote: > captial 'A' done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:131: ASSERT(right != NGSizeIndefinite); On 2016/11/18 at 17:54:20, cbiesinger wrote: > DCHECK_NE Done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:136: ASSERT(width != NGSizeIndefinite); On 2016/11/18 at 17:54:20, cbiesinger wrote: > DCHECK_NE Done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:155: DCHECK((container_size.width - left - right - margin_left - margin_right - On 2016/11/18 at 17:54:19, cbiesinger wrote: > DCHECK_EQ. I might write it as: > DCHECK_EQ(container_size.width, left + right + margin_left + margin_right + width); Done. Yes, better. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:159: width = std::max(width, LayoutUnit(0)); On 2016/11/18 at 17:54:20, cbiesinger wrote: > You need max(width, border_and_padding) -- that's an assumption we have in various places in our code, though we occasionally violate it :( Done. Good catch. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:168: void ComputeAbsoluteVertical(const NGConstraintSpace& space, On 2016/11/18 at 17:54:20, cbiesinger wrote: > (please make the corresponding changes here, like for the horizontal version) Done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:173: space.Size().ConvertToPhysical(space.WritingMode()); On 2016/11/18 at 20:00:23, ikilpatrick wrote: > should this be available_size ? yes. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:177: LayoutUnit space_block_size = space.PercentageResolutionSize().block_size; On 2016/11/18 at 20:00:23, ikilpatrick wrote: > percent_size similar to ng_length_utils.cc done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:234: // are values overconstrained? On 2016/11/18 at 20:00:23, ikilpatrick wrote: > make all of the comments in this function consistent with above. done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:251: ASSERT(bottom != NGSizeIndefinite); On 2016/11/18 at 20:00:23, ikilpatrick wrote: > DCHECK_NE done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:256: ASSERT(height != NGSizeIndefinite); On 2016/11/18 at 20:00:23, ikilpatrick wrote: > DCHECK_NE done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:271: ASSERT((container_size.height - top - bottom - margin_top - margin_bottom - On 2016/11/18 at 20:00:23, ikilpatrick wrote: > DCHECK_EQ done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:298: // NGLogicalSize NGAbsoluteLogicalPosition::BorderBoxAdjustedSize() { On 2016/11/18 at 17:54:20, cbiesinger wrote: > ? What's up with this commented-out function? Gone. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:327: NGAbsolutePhysicalPosition ComputePartialAbsoluteWithChildInlineSize( On 2016/11/18 at 20:00:22, ikilpatrick wrote: > it might be cleaner to expect the caller to provide the NGAbsolutePhysicalPosition instead of setting it, e.g. > > child_position_ = {}; > > ComputePartialAbsoluteWithChildInlineSize(..., &child_position_) > > for consistency. I think that not being consistent here is good. It draws attention to the fact that these two functions should be called in particular order. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:18: struct CORE_EXPORT NGAbsoluteLogicalPosition { On 2016/11/18 at 17:54:20, cbiesinger wrote: > I'm OK with this name but not super-excited -- it describes the logical position for an abspos (or fixed-pos) element; it does not describe an absolute position for something The name is just a Logical counterpart to NGAbsolutePhysicalPosition. I am not thrilled with it, but it is consistent. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:18: struct CORE_EXPORT NGAbsoluteLogicalPosition { On 2016/11/18 at 20:00:23, ikilpatrick wrote: > do we actually need this class? Most of the time we'll just be doing: > > physical_pos.size.ConvertToLogical() right? We are also converting the inset. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:19: NGBoxStrut inset; // top, right, bottom, left On 2016/11/18 at 20:00:23, ikilpatrick wrote: > remove comment? done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:27: // Initializes values from style. On 2016/11/18 at 20:00:23, ikilpatrick wrote: > remove. done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:33: // The following routines implement absolute size resolution algorithm On 2016/11/18 at 20:00:23, ikilpatrick wrote: > nit. (everywhere) > > Use proper punctuation everywhere, e.g. commas, fullstops etc. > > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... k. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:43: // layout out block inside estimated_inline_size On 2016/11/18 at 20:00:23, ikilpatrick wrote: > If AbsoluteNeedChildBlockSize is true, compute the estimated block_size by performing layout with the inline_size calculated from (2). > > or similar. at this stage it isn't using the estimated_inline_size, it's resolved. done. https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:45: // upon child's block_size' On 2016/11/18 at 20:00:23, ikilpatrick wrote: > remove ' > > add full stop. done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:48: // estimated inline size On 2016/11/18 at 20:00:23, ikilpatrick wrote: > full stop. & elsewhere. done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:133: // all auto => width is estimated_inline, left is 0 On 2016/11/18 at 20:00:23, ikilpatrick wrote: > capital 'A' & elsewhere. done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:239: // On 2016/11/18 at 20:00:23, ikilpatrick wrote: > remove line. done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:276: LayoutUnit top(5); On 2016/11/18 at 20:00:23, ikilpatrick wrote: > should these be of the form kTop ? > > not sure. leave as is https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:366: // Rule 4: top is unknonwn On 2016/11/18 at 20:00:23, ikilpatrick wrote: > spelling. done https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:373: // Rule 5: bottom is unknonwn On 2016/11/18 at 20:00:23, ikilpatrick wrote: > spelling. done
https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h (right): https://codereview.chromium.org/2497863002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.h:18: struct CORE_EXPORT NGAbsoluteLogicalPosition { On 2016/11/18 22:56:10, atotic2 wrote: > On 2016/11/18 at 20:00:23, ikilpatrick wrote: > > do we actually need this class? Most of the time we'll just be doing: > > > > physical_pos.size.ConvertToLogical() right? > > We are also converting the inset. but we can just do physical_pos_inset.ConvertToLogical() right? I don't think there is a case where we need to do both at the same time. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:93: // Compute margins period. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:101: // Margins are negative. // Margin space is negative. ? https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:132: // Rules 1::3, 2 out of 3 are unknown. comment differs from below. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:151: // Rules 4::6, 1 out of 3 are unknown. s/4::6/4:6/ ? https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:176: NGPhysicalSize container_size = move to same place in fn as above. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:203: if (style.boxSizing() == EBoxSizing::BoxSizingContentBox && add comment as above. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:256: // Rule 1 period. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:264: } else if (height == NGSizeIndefinite && bottom == NGSizeIndefinite) { // Rule 3. ? https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:280: height = std::max(height, border_padding); add comment as above? https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc (right): https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:21: // if not set, border widths will always be 0 capitalize & period. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:99: return; remove?
done. ptal https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:93: // Compute margins On 2016/11/18 at 23:58:28, ikilpatrick wrote: > period. done. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:101: // Margins are negative. On 2016/11/18 at 23:58:27, ikilpatrick wrote: > // Margin space is negative. > > ? switched to "Margin space is over-constrained.", the spec language. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:132: // Rules 1::3, 2 out of 3 are unknown. On 2016/11/18 at 23:58:27, ikilpatrick wrote: > comment differs from below. // Rules 1 through 3, 2 out of 3 are unknown. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:151: // Rules 4::6, 1 out of 3 are unknown. On 2016/11/18 at 23:58:28, ikilpatrick wrote: > s/4::6/4:6/ ? // Rules 4 through 6, 1 out of 3 are unknown. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:176: NGPhysicalSize container_size = On 2016/11/18 at 23:58:27, ikilpatrick wrote: > move to same place in fn as above. done https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:203: if (style.boxSizing() == EBoxSizing::BoxSizingContentBox && On 2016/11/18 at 23:58:27, ikilpatrick wrote: > add comment as above. done https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:256: // Rule 1 On 2016/11/18 at 23:58:27, ikilpatrick wrote: > period. done https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:264: } else if (height == NGSizeIndefinite && bottom == NGSizeIndefinite) { On 2016/11/18 at 23:58:27, ikilpatrick wrote: > // Rule 3. > > ? done https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:280: height = std::max(height, border_padding); On 2016/11/18 at 23:58:27, ikilpatrick wrote: > add comment as above? done https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc (right): https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:21: // if not set, border widths will always be 0 On 2016/11/18 at 23:58:28, ikilpatrick wrote: > capitalize & period. done. https://codereview.chromium.org/2497863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils_test.cc:99: return; On 2016/11/18 at 23:58:28, ikilpatrick wrote: > remove? done
https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:22: DCHECK(!length.isPercent() || max_value != NGSizeIndefinite); isPercentOrCalc https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:62: // TODO(atotic): This will resolve size for fit-content incorrectly. Can you change that to say "This should use Resolve{Inline,Block}Length"? I believe there's a couple other things it gets a bit wrong. https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:71: LayoutUnit static_start(0); // TODO(atotic) needs to be passed in. here too, just use LayoutUnit static_start; and leave off the (0) https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:196: // TODO(atotic): This will resolve size for fit-content incorrectly. same here
CR fixes done, ptal https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:22: DCHECK(!length.isPercent() || max_value != NGSizeIndefinite); On 2016/11/19 at 00:30:29, cbiesinger wrote: > isPercentOrCalc done https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:62: // TODO(atotic): This will resolve size for fit-content incorrectly. On 2016/11/19 at 00:30:29, cbiesinger wrote: > Can you change that to say "This should use Resolve{Inline,Block}Length"? I believe there's a couple other things it gets a bit wrong. done https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:71: LayoutUnit static_start(0); // TODO(atotic) needs to be passed in. On 2016/11/19 at 00:30:29, cbiesinger wrote: > here too, just use LayoutUnit static_start; and leave off the (0) done https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:196: // TODO(atotic): This will resolve size for fit-content incorrectly. On 2016/11/19 at 00:30:29, cbiesinger wrote: > same here done
lgtm, thanks! https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:101: // Margins are negative. make same as comment below.
lgtm https://codereview.chromium.org/2497863002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:18: LayoutUnit max_value, I forgot to answer that question, but an NGConstraintSpace can have an indefinite block size (when there is no explicit height on the parent). The inline size should never be indefinite. https://codereview.chromium.org/2497863002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:62: // TODO(atotic): This should useResolve{Inline,Block}Length Missing space after "use" in both occurences of this comment
https://codereview.chromium.org/2497863002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right): https://codereview.chromium.org/2497863002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:62: // TODO(atotic): This should useResolve{Inline,Block}Length On 2016/11/19 at 00:51:09, cbiesinger wrote: > Missing space after "use" in both occurences of this comment done
The CQ bit was checked by atotic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2497863002/#ps160001 (title: "Final CR fixes")
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 #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== AbsolutePositioning utilities with tests These are the routines that implement absolute positioning specification. ComputeAbsoluteHorizontal and ComputeAbsoluteVertical implement the spec. They will be used by block_algorithm to position abspos elements. BUG=635619 ========== to ========== AbsolutePositioning utilities with tests These are the routines that implement absolute positioning specification. ComputeAbsoluteHorizontal and ComputeAbsoluteVertical implement the spec. They will be used by block_algorithm to position abspos elements. BUG=635619 Committed: https://crrev.com/5836e40a4c5491a4b7351a8f0835f3a4d36c92d8 Cr-Commit-Position: refs/heads/master@{#433402} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5836e40a4c5491a4b7351a8f0835f3a4d36c92d8 Cr-Commit-Position: refs/heads/master@{#433402} |