"Fragmenting Grid Layout" in http://dev.w3.org/csswg/css-grid/#pagination Note that the spec text currently contains phrases such as "The exact layout of a fragmented grid container is not defined" and "This is just a rough draft". So it probably makes sense to prioritize this bug low for now, as the spec matures.

FYI, there's 14 or so patches coming up. I'll start with 5 that are mostly refactoring code in preparation for the actual fragmentation. Each part in the series compiles and pass the css-grid reftests. In summary, this first part does: Move the mGridItems and mAbsPosItems members and the CalculateTrackSizes, ContainingBlockFor and ContainingBlockForAbsPos methods from nsGridContainerFrame to GridReflowState. Add a 'mFrame' member to GridReflowState. Create a new struct nsGridContainerFrame::Grid. Move all methods and members that deals with grid item placement to it. Move a few static functions that's only used during placement to it. This part just moves code around with a few minor tweaks to make it compile.

(I'll explain a bit more how it's used once we get to that code...)

The CellMap object is now a member of Grid which we create a new instance of whenever we place items.

This moves nsGridContainerFrame::TrackSize/LineRange/GridArea/GridItemInfo structs from the .h to the .cpp file. Also moves the static helper function IsMinContent into a static member function of TrackSize since it's only used there.

This makes the mIsNormalFlowInCSSOrder bool member into a frame bit instead. Also add another bit (unused so far) that I'll explain once we get to the code using it. This part is the last of the refactoring parts.

(That's all I have for the moment. More coming in a day or two.)

Sorry for the review-lag here -- taking a look at these today. [One observation/heads-up: looks like these were bitrotted by the frame-property-improvement tweaks in bug 1230034. The patches apply cleanly if I update to a cset before that, though -- e.g. 671d9025bd61]

Comment on attachment 8710770 [details] [diff] [review] part 1 - [css-grid] Refactor nsGridContainerFrame state and methods. Review of attachment 8710770 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 1, with the following addressed as you see fit. (I didn't sanity-check the code-moving too much; I'll take your word for it that nothing changed during the move) ::: layout/generic/nsGridContainerFrame.cpp @@ +1021,5 @@ > * min/max-content contributions when sizing tracks. > */ > const nsHTMLReflowState* mReflowState; > nsRenderingContext& mRenderingContext; > + nsGridContainerFrame* mFrame; Let's make this a const pointer (the pointer itself being const, not the frame), to enforce and document that we won't somehow change to point to another frame midway through this object's lifetime. So: nsGridContainerFrame* const mFrame; While you're at it, we might also want to change the mReflowState member up a few lines to be a const pointer as well (it's already got a const value, but not a const pointer): const nsHTMLReflowState* const mReflowState; @@ +1042,5 @@ > mGridStyle->mGridAutoRowsMin, > mGridStyle->mGridAutoRowsMax) > , mReflowState(aReflowState) > , mRenderingContext(aRenderingContext) > + , mFrame(aFrame) In the body of the GridReflowState constructor, please assert that either mReflowState is null, or mFrame == mReflowState.frame. (I was going to say "mFrame is technically redundant, because it's already on mReflowState". But mReflowState might be null, which means there's value in tracking the frame independently.) @@ +1047,5 @@ > , mWM(aWM) > {} > }; > > +struct MOZ_STACK_CLASS nsGridContainerFrame::Grid Two things: (1) This new struct needs a brief comment to explain what its purpose is (and how it differs from the other various grid-flavored classes here -- gridreflowstate, gridcontainerframe) (2) Are you sure you want to define so many of this struct's methods as inline inside of the struct body? IMO that makes the struct as a whole pretty hard to read/follow, since it's so long (> 1000 lines). This would be less of an issue if it had its own dedicated .h file, but as a helper-class in the middle of a .cpp file, it's a bit hard to follow. So: consider (either here or in another patch) moving some of the method definitions out-of-line. (I think a lot of them were originally out-of-line, so you could just leave them in their original place, add "Grid::" before their names at those spots, and put their decls in the struct here. That would have the secondary (minor) benefit of preserving hg blame on all these functions' implementations, too.) @@ +3396,5 @@ > } > } > > LogicalRect > +nsGridContainerFrame::GridReflowState::ContainingBlockFor(const GridArea& aArea) const If you like, lines like this one would benefit from a typedef at the top of this .cpp file: typedef nsGridContainerFrame::GridReflowState GridReflowState; ...so that these lines don't have to be quite so long. (This is 87 lines long right now, which is well over 80 characters).

Comment on attachment 8710772 [details] [diff] [review] part 2 - [css-grid] Make GridItemInfo::mFrame available also in non-DEBUG builds since we'll need it to support fragmentation. Review of attachment 8710772 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 2; just one nit: ::: layout/generic/nsGridContainerFrame.h @@ +337,5 @@ > mIsFlexing[0] = false; > mIsFlexing[1] = false; > } > > + nsIFrame* mFrame; Similar to part 1, could you make this "nsIFrame* const"? (This is a GridItemInfo member-var.)

Comment on attachment 8710774 [details] [diff] [review] part 4 - [css-grid] Move more local nsGridContainerFrame classes into .cpp file. Review of attachment 8710774 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8710775 [details] [diff] [review] part 5 - [css-grid] Create a couple of Grid container frame bits. Review of attachment 8710775 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 5; minor comment nits below: ::: layout/generic/nsFrameStateBits.h @@ +305,5 @@ > +// True iff the normal flow children are already in CSS 'order' in the > +// order they occur in the child frame list. > +FRAME_STATE_BIT(GridContainer, 20, NS_STATE_GRID_NORMAL_FLOW_CHILDREN_IN_CSS_ORDER) > + > +// True if some first-in-flow in-flow children we're pushed. Nit: "we're" --> "were" Also, maybe "if" --> "iff" (if appropriate), for consistency with your other comment above this one which uses "iff".

Comment on attachment 8710777 [details] [diff] [review] part 6 - [css-grid] Add support for creating Grid container continuations and deal with overflow containers. Review of attachment 8710777 [details] [diff] [review]: ----------------------------------------------------------------- Holding off on r+ for part 6, because I want to better understand the situation around my second note below (in BuildDisplayList) before I'm comfortable r+'ing. ::: layout/generic/nsGridContainerFrame.cpp @@ +3743,5 @@ > } > > + // Merge overflow container bounds and status. > + aDesiredSize.mOverflowAreas.UnionWith(ocBounds); > + NS_MergeReflowStatusInto(&aStatus, ocStatus); Do these lines really need to be all the way down here, separated by ~100 lines from the initialization of these variables? (ocBounds & ocStatus) If it's possible to move this chunk up to where these variables are declared & set (and it looks like it should be possible), that seems strictly better/cleaner. @@ +3966,5 @@ > const nsDisplayListSet& aLists) > { > + if (!(GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)) { > + DisplayBorderBackgroundOutline(aBuilder, aLists); > + } This state-check guarding the background/border painting makes superficial sense to me.... But, I'm confused -- I'd expect that we'd need this check in other BuildDisplayList implementations (nsBlockFrame's in particular), but there is no such check there. Do you know how nsBlockFrame avoids painting borders/backgrounds for its overflow containers? (and should we be doing whatever it does? Maybe we already do, and this check is unnecessary?) Code quote, for reference: 6451 void 6452 nsBlockFrame::BuildDisplayList(nsDisplayListBuilder* aBuilder, 6453 const nsRect& aDirtyRect, 6454 const nsDisplayListSet& aLists) 6455 { [...] 6476 6477 DisplayBorderBackgroundOutline(aBuilder, aLists); http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=d77374bc1315#6477

BTW, does Flexbox layout handle these? Let me know of if you want me to exclude flex containers too.

This patch creates a SharedGridData struct to share data between all Grid container continuations. The first-in-flow creates it at the end of its Reflow (only if it's *not* COMPLETE). At that point it has no further use for its GridReflowState data so we can cheaply move the arrays we need by swapping the elements. There's also a new GridReflowState::Initialize method (for continuations) that copies the data it needs for reflow from the SharedGridData. I also add a couple of members mFragBStart, mStartRow so that each fragment knows which row it starts on and what block-axis coord it starts at (these are both zero for the first-in-flow). I'm changing nsGridContainerFrame::Reflow so that the stuff that only applies to the first-in-flow (placement, track sizing etc) is skipped for continuations.

It's probably easier to review the Reflow() changes using this wdiff.

Don't worry about the multiple (virtual) calls to GetLogicalSkipSides() in various places. I'll cache that on the GridReflowState in a cleanup patch at the end.

I've written a fairly long explanation in a code comment on how the child lists behave under fragmentation. I think it's important that it's clear how this works - let me know if it's not. I'm using the NS_STATE_GRID_DID_PUSH_ITEMS frame bit here. It's set on any fragment that pushes a grid item (because it's placed on a row that will be reflowed in a later fragment). (Setting this flag comes in a later part though.)

This patch is meant to be idempotent, with just a few trivial changes to make it compile. I'll attach a wdiff for easier reviewing...

It's probably easier to review this part after glancing at the next part which will do the actual fragmentation.

Comment on attachment 8716055 [details] [diff] [review] part 11 - [css-grid] Add a GetNearestFragmentainer() method that collects some data from the nearest enclosing fragmentainer needed for fragmentation. I wrote some more tests for min-/max-height and discovered a minor bug here: + data->mCanBreakAtEnd = computedBSize > gridEnd && + computedBSize > aState.mFragBStart; The |computedBSize| value there should have had min-/max-height applied but it's missing. Please ignore that issue for now, I'll attach a separate patch with a bug fix.

Comment on attachment 8715900 [details] [diff] [review] part 7 - [css-grid] Don't create PageBreakFrames inside a Grid container. Review of attachment 8715900 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mats Palmgren (:mats) from comment #15) > BTW, does Flexbox layout handle these? Hmm, no -- it does not. > Let me know of if you want me to > exclude flex containers too. Thanks! Let's fix that separately, I think, to avoid broadening the scope of what's changing here too much. (Then we can fix the flex part with a testcase, too -- in particular, I can trigger assertion-failures when testing this with a flex container. I'll file a followup bug with a testcase after poking around some more, and I'll fix this in that followup, by promoting this patch's grid type-check to "IsFlexOrGridContainer".)

Comment on attachment 8715901 [details] [diff] [review] part 8 - [css-grid] Add a new track state flag, eBreakBefore, to record where breaks occur. Review of attachment 8715901 [details] [diff] [review]: ----------------------------------------------------------------- Commit message: > Bug 1144096 part 8 - [css-grid] Add a new track state flag [...] Nit: the phrase "track state" is ambiguous (since "state tracking" is a thing, and "track state" is a thing that people do which is completely unrelated to what you actually mean here) Consider replacing the word "track" here with either "grid-track", or with "row/column", to avoid that ambiguity. ::: layout/generic/nsGridContainerFrame.cpp @@ +4069,5 @@ > > void > nsGridContainerFrame::TrackSize::Dump() const > { > + printf("mPosition=%d mBase=%d mLimit=%d", mPosition, mBase, mLimit); This change (adding mPosition to what we log here) seems unrelated to the rest of this patch. Not a big deal, since it's trivial; only mentioning it in case you'd meant to include it somewhere else & accidentally qref'ed it into this patch.

Comment on attachment 8716049 [details] [diff] [review] part 9 - [css-grid] Create a SharedGridData object owned by the first-in-flow Grid container to share state between its continuations. Review of attachment 8716049 [details] [diff] [review]: ----------------------------------------------------------------- Partial review of part 9 ("wave 1" of review comments): ::: layout/generic/nsGridContainerFrame.cpp @@ +1251,5 @@ > +/** > + * Grid data shared by all continuations, owned by the first-in-flow. > + * The data is initialized from the first-in-flow's GridReflowState at > + * the end of its reflow. mRows.mSizes is modified when breaks occur > + * by modifying mPosition to remove the row gap at the break boundary I don't fully undrestand what this sentence is saying. Consider rewording it? The line of causality is a bit hard to follow, the way it's phrased right now. ("[foo] is modified when [conditions] occur by modifying [bar]"). Maybe restate it in the active voice? e.g. "[Some frame or function] will modify [something] when fragmentation breaks occur". Also: it's not clear which "mPosition" variable you're talking about here. This struct (SharedGridData) does not have its own "mPosition" member-var, so it's not that... Maybe you're talking about a mPosition member-var on some entry/entries within the mRows.mSizes array? If so, please reword to make that clearer. @@ +1252,5 @@ > + * Grid data shared by all continuations, owned by the first-in-flow. > + * The data is initialized from the first-in-flow's GridReflowState at > + * the end of its reflow. mRows.mSizes is modified when breaks occur > + * by modifying mPosition to remove the row gap at the break boundary > + * and setting the eBreakBefore flag. mBase is modified when we decide Please clarify which "mBase" you're talking about here. (We don't have a direct mBase member-variable, and there are multiple "indirect" member-variables that are named "mBase": mCols.mSizes[i].mBase mRows.mSizes[i].mBase mOriginalRowData[i].mBase Please clarify to specify "mWhatever[i].mBase" instead of just "mBase" here. @@ +1255,5 @@ > + * by modifying mPosition to remove the row gap at the break boundary > + * and setting the eBreakBefore flag. mBase is modified when we decide > + * to grow a row. mOriginalRowData is setup by the first-in-flow and > + * not modified after that. It's used for undoing the breaks in mRows. > + * mCols, mGridItems, mAbsPosItems is used for initializing the grid s/is used/are used/ @@ +1292,5 @@ > aFrame->GetWritingMode()) > {} > > /** > + * Initialize our track sizes and grid item info from aData. typo: "from aData" is wrong here. This function is no "aData" parameter. @@ +1298,5 @@ > + void Initialize(nsGridContainerFrame* aGridContainerFrame, > + nscoord aConsumedBSize) > + { > + MOZ_ASSERT(aGridContainerFrame->GetPrevInFlow(), > + "don't call this on the first-in-flow"); This function ("Initialize") is too normal/generic of a name, for something that's prohibited from being called for the first in flow. Maybe rename to "InitializeFromFirstInFlow" or "InitializeForContinuation" or something like that, to make it clearer that this is a continuation-specific thing? @@ +1310,5 @@ > + for (auto pif = aGridContainerFrame->GetPrevInFlow(); > + pif; pif = pif->GetPrevInFlow()) { > + ++fragment; > + firstInFlow = pif; > + } This loop means that printing a grid will be an N^2 algorithm -- continuation number "i" will have to loop "i" times here, so we end up doing 1+2+3+4+5+...+N work to set up all of the pages. Maybe that's unavoidable... But if you see an easy way to avoid it, seems like we should try. Strawman example: here, maybe we could store the "current fragment ID" as a uint32_t member-variable on the SharedGridData object, and each continuation can read that value, and then increment it for the next continuation? (That would work on the first reflow, though maybe not on incremental reflows. But, for printing, we might only ever have a single "first reflow" anyway -- so maybe it's worth optimizing for that?) @@ +1321,5 @@ > + const uint32_t numRows = rowSizes.Length(); > + mStartRow = numRows; > + for (uint32_t row = 0, breakCount = 0; row < numRows; ++row) { > + if (rowSizes[row].mState & TrackSize::eBreakBefore) { > + if (fragment == ++breakCount) { Hmm, looks like this will be n^2 too. (iterating with break-count going from 1 up to |fragment|, effectively) Maybe n^2 algorithms are unavoidable here?

New version that takes min-/max-bsize into account when determining if there is a break opportunity after the last row. (FYI, the last batch of patches are nearly complete. I expect to upload them for review in a day or two.)

Comment on attachment 8716049 [details] [diff] [review] part 9 - [css-grid] Create a SharedGridData object owned by the first-in-flow Grid container to share state between its continuations. Review of attachment 8716049 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Daniel Holbert [:dholbert] from comment #28) > Partial review of part 9 ("wave 1" of review comments): ...and here's "wave 2" (remaining) review comments on part 9. I'm a bit concerned about n^2 algorithms noted here, but I suppose this is r=me with these notes & comment 28 addressed how you see fit. (Though, feel free to re-request review if there are substantial changes.) ::: layout/generic/nsGridContainerFrame.cpp @@ +1341,5 @@ > + } > + } > + } > + if (mStartRow == numRows) { > + mFragBStart = aConsumedBSize; Consider adding a comment here like: // All of the grid's rows fit inside of previous grid-container fragments. (At least, I think that's what this case represents?) @@ +1346,5 @@ > + } > + > + // Copy the shared track state. > + mCols = mSharedGridData->mCols; > + mRows = mSharedGridData->mRows; Why do we need to make our own private copy of the mCols & mRows arrays here (distinct from the shared data)? If possible, it seems like we should just make use of (or take ownership of?) the shared versions. But if we really need to copy while leaving the shared versions intact, please expand this code-comment a bit, to explain/justify why. @@ +1357,5 @@ > + DebugOnly<size_t> len = mGridItems.Length(); > + for (auto& itemInfo : mSharedGridData->mGridItems) { > + if (itemInfo.mFrame == childFirstInFlow) { > + mGridItems.AppendElement(GridItemInfo(child, itemInfo.mArea)); > + break; This nested-loop seems like it's O(n^2), with n being the number of grid items. Can we avoid this? (Or at least cut it down to n^2 overall?) Maybe we should be storing the shared grid items using a hash map, for constant-time lookup, so that this nested loop will scale linearly instead of quadratically, as the number of items & children grow? Maybe at least worth dropping a comment alongside this loop saying "NOTE: This is O(n^2) in the number of items", as a hint for places that may need optimization later on... (These n^2 algorithms are particularly concerning because we do this work *for each page*, which means it's really O(n^3), when there are O(n) pages [which there would be with grid auto-layout, for e.g. a pinterest-style grid with lots of items.) @@ +1367,5 @@ > + nsFrameList children(aGridContainerFrame->GetChildList( > + aGridContainerFrame->GetAbsoluteListID())); > + if (!children.IsEmpty()) { > + for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) { > + nsIFrame* childFirstInFlow = e.get()->FirstInFlow(); Two things: (1) consider renaming "children" to "absPosChildren", or "absPosKids", since that's really what it is. (This function works with abspos as well as non-abspos children, so it's confusing to have a local var with a generic name "children" which really only contains the abspos kids.) (2) I believe we could simplify this a bit with a range-based for loop, e.g.: for (nsIFrame f : absPosChildren) { nsIFrame* childFirstInFlow = f->FirstInFlow(); @@ +1371,5 @@ > + nsIFrame* childFirstInFlow = e.get()->FirstInFlow(); > + DebugOnly<size_t> len = mAbsPosItems.Length(); > + for (auto& itemInfo : mSharedGridData->mAbsPosItems) { > + if (itemInfo.mFrame == childFirstInFlow) { > + mAbsPosItems.AppendElement(GridItemInfo(e.get(), itemInfo.mArea)); (Like the loop that I commented on above, this loop seems to be O(n^2) in the number of abspos items, worst case. Could be addressed by using a hash or tree map, potentially; otherwise, consider dropping a comment here to note this as an n^2 algorithm that may benefit from optimization.) @@ +1426,5 @@ > */ > const nsHTMLReflowState* mReflowState; > nsRenderingContext& mRenderingContext; > nsGridContainerFrame* mFrame; > + SharedGridData* mSharedGridData; Do we really need this pointer? I think it can be replaced with just a local variable, since we only use it in a single function. I'm a bit uneasy about adding a raw-pointer member-var that points to a structure whose lifetime is non-obvious. (Particularly in pagination code, which is notorious for having sec bugs from dangling pointers.) Please do one of the following for this member-var: (1) Remove it, and replace it with a local variable in the sole place where it's used it right now -- Initialize(). (And if we ever need this value somewhere else, we can just just look it up dynamically from the first-in-flow's frame's property table; no need to have it persistently saved somewhere else.) Or, if we really do need it here, (e.g. if you know we're going to be using this value in multiple places and that's too many property-table-lookup operations), then instead: (2) Just add a comment here making the ownership clear, e.g.: add this at the end of the line: // [weak] owned by mFrame's first-in-flow. (I'd prefer (1) if we can do it...) @@ +1434,5 @@ > + * or, if this fragment starts after the last row, the GetConsumedBSize(). > + */ > + nscoord mFragBStart; > + /** The start row for this fragment. */ > + uint32_t mStartRow; (I think the /** double-asterisk might be a typo here, in this 1-line comment? Not sure, maybe it has some significance I'm unaware of.) @@ +4018,5 @@ > + } > + bSize += gridReflowState.mRows.SumOfGridGaps(); > + } else { > + bSize = gridReflowState.mRows.GridLineEdge(numRows, > + GridLineSide::eAfterGridGap); It's not clear to me why this bSize calculation code behaves different for the first-in-flow vs. not first-in-flow scenario. (The two clauses seem to do approximately the same thing -- compute the position of the final row-line. But it's not clear what the functional difference between their two techniques is, or why one strategy is appropriate for the first-in-flow and the other strategy is needed for continuations.) Could you add an explanatory comment? (Maybe they do produce the same thing, and it's just that for the first-in-flow, we haven't computed the line-positions yet? not sure.) @@ +4062,5 @@ > + // Save the original row grid sizes and gaps so we can restore them later > + // in GridReflowState::Initialize for the continuations. > + auto& origRowData = sharedGridData->mOriginalRowData; > + origRowData.SetCapacity(sharedGridData->mRows.mSizes.Length()); > + nscoord prevTrackEnd(0); Nit: this should probably be "= 0" instead of "(0)", for consistency with how we normally set nscoord values. (I don't think we have any other code that initializes nscoords using function syntax like this.) @@ +4074,5 @@ > + sharedGridData->mGridItems.SwapElements(gridReflowState.mGridItems); > + sharedGridData->mAbsPosItems.Clear(); > + sharedGridData->mAbsPosItems.SwapElements(gridReflowState.mAbsPosItems); > + } else if (sharedGridData && !GetNextInFlow()) { > + Properties().Delete(SharedGridData::Prop()); Do we really need the GetNextInFlow() check here? If we're complete, we know that our container is about to delete our next-in-flow (if it exists), I think. So, I'd expect we should be OK to delete our sharedGridData regardless of whether we have a next-in-flow, because we know our next-in-flow is going away. The way this is structured right now, it looks like sharedGrid will stick around after it's needed, until the next time a reflow happens.

Comment on attachment 8716052 [details] [diff] [review] part 10 - [css-grid] Add a few helper methods to do a break before a row, and resize a row. Review of attachment 8716052 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 10, with nits addressed: ::: layout/generic/nsGridContainerFrame.cpp @@ +1237,5 @@ > + * gap before aRow to zero (and shift all rows after it by the removed gap). > + */ > + void BreakBeforeRow(uint32_t aRow) > + { > + MOZ_ASSERT(mAxis == eLogicalAxisBlock); Please add an assertion message to explain why this function is block-axis-specific (& row-specific). E.g. "Should only be fragmenting in block axis (between rows)"); @@ +1238,5 @@ > + */ > + void BreakBeforeRow(uint32_t aRow) > + { > + MOZ_ASSERT(mAxis == eLogicalAxisBlock); > + nscoord prevRowEndPos(0); Nit: as noted in previous comment, I don't think we use this "nscoord foo(0)" syntax anywhere else (literally) -- here's an MXR search, at least: http://mxr.mozilla.org/mozilla-central/search?string=nscoord+.*\%280&regexp=1&find=&findi=&filter=^[^\0]*%24 (I did a similar grep command locally, to avoid trusting MXR regexps too much, and I only discovered instances that you've added in the previous patch & this patch.) So: unless there's some reason to prefer & migrate towards this "(0)" syntax, probably better to stay consistent and stick with "= 0". @@ +1241,5 @@ > + MOZ_ASSERT(mAxis == eLogicalAxisBlock); > + nscoord prevRowEndPos(0); > + if (aRow != 0) { > + auto& sz = mSizes[aRow - 1]; > + prevRowEndPos = sz.mPosition + sz.mBase; Consider s/sz/prevSz/ inside this block, to make it a bit clearer what "sz" represents here. (and also to differentiate it from the other completely-different "sz" which gets declared 2 lines below this.) @@ +1256,5 @@ > + > + /** > + * Set the size of aRow to aSize and adjust the position of all rows after it. > + */ > + void SetRowSize(uint32_t aRow, nscoord aSize) Two nits: (1) Consider renaming to something like "AdjustRowSize", or "ResizeRow", or something like that. The current name, "SetRowSize", sounds like it's just a normal setter which stomps on the old value. (It also sounds like something we might use when we make our initial determination of row sizes, in the first pass of our grid layout algorithm. And it doesn't sound like it'd have side effects, necessarily.) In contrast, "AdjustRowSize" / "ResizeRow" are more clearly about making changes after-the-fact, & operating on pre-existing row sizes. (2) consider s/aSize/aNewSize/ -- that'll makes lines like "nscoord delta = aSize - sz.mBase;" a bit easier to understand. @@ +1258,5 @@ > + * Set the size of aRow to aSize and adjust the position of all rows after it. > + */ > + void SetRowSize(uint32_t aRow, nscoord aSize) > + { > + MOZ_ASSERT(mAxis == eLogicalAxisBlock); As above, please add an assertion message here, to explain why this is block-axis (and row) specific. The same message as suggested above would probably do. @@ +1265,5 @@ > + nscoord delta = aSize - sz.mBase; > + sz.mBase = aSize; > + const uint32_t numRows = mSizes.Length(); > + for (uint32_t r = aRow + 1; r < numRows; ++r) { > + mSizes[r].mPosition += delta; Note that this loop does nothing, if the new size and old size happen to be the same. Perhaps that'd be worth handling here with an early-return before the loop, just like the early-return for gap==0 in this patch's other function, BreakBeforeRow()? (Or, maybe we don't bother handling that because we shouldn't be calling this function in the first place when the size doesn't change? If that's the case, maybe we should add something like: NS_WARN_IF_FALSE(delta != 0, "Useless call to SetRowSize); ...to give ourselves a hint that we're spinning our wheels for no good reason.)

Comment on attachment 8720336 [details] [diff] [review] part 11 - [css-grid] Add a GetNearestFragmentainer() method that collects some data from the nearest enclosing fragmentainer needed for fragmentation. Review of attachment 8720336 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 11, nits below: ::: layout/generic/nsGridContainerFrame.cpp @@ +3816,5 @@ > > +/** > + * Return a Fragmentainer object if we have a fragmentainer frame in our > + * ancestor chain of containing block (CB) reflow states. All the CBs > + * must have the same writing-mode and have overflow:visible. This last sentence is a bit ominous. I'm left thinking, "OK, you say they *must* have the same writing-mode & overflow:visible... or else, what? What's going to happen if they violate this "must"? I think what you mean is: "We'll only continue traversing the ancestor chain as long as the CBs have the same writing-mode and have overflow:visible." Something like that would make it clearer what happens when this requirement is violated (we'll stop traversing & bail -- we're not going to crash or assert fatally or anything like that.) Consider rewording along those lines. @@ +3841,5 @@ > + data.emplace(); > + data->mIsTopOfPage = gridRS->mFlags.mIsTopOfPage; > + LogicalMargin bp = gridRS->ComputedLogicalBorderPadding(); > + const auto logicalSkipSides = GetLogicalSkipSides(); > + bp.ApplySkipSides(logicalSkipSides); logicalSkipSides is only used once; seems cleaner to just collapse it into its one usage, like so: bp.ApplySkipSides(GetLogicalSkipSides()); Saves you a lot of characters and gets rid of a somewhat-hand-wavy "const auto"-typed variable, at least. ::: layout/generic/nsGridContainerFrame.h @@ +164,5 @@ > + nscoord mToFragmentainerEnd; > + /** > + * True if the current fragment is not at the start of the fragmentainer. > + */ > + bool mIsTopOfPage; Maybe I'm misunderstanding, but it looks like the comment is wrong here -- I think you want to drop the word "not"? ("mIsTopOfPage" sounds like it should be true if our fragment *is* at the start of the page/fragmentainer. So the word "not" seems out of place here.)

Comment on attachment 8716061 [details] [diff] [review] part 12 - [css-grid] Collect and merge child frames we need for reflow. Review of attachment 8716061 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGridContainerFrame.cpp @@ +51,5 @@ > +ReparentFrames(nsFrameList& aFrameList, nsContainerFrame* aOldParent, > + nsContainerFrame* aNewParent) > +{ > + for (nsFrameList::Enumerator e(aFrameList); !e.AtEnd(); e.Next()) { > + ReparentFrame(e.get(), aOldParent, aNewParent); If you like, a range-based for loop would be a bit more concise: for (nsIFrame f : aFrameList) { ReparentFrame(f, aOldParent, aNewParent); } @@ +176,5 @@ > + * @param aCommonAncestor a hint for nsLayoutUtils::CompareTreePosition > + */ > +static void > +MergeSortedFrameLists(nsFrameList& aDest, nsFrameList& aSrc, > + nsIContent* aCommonAncestor) We already have nsIFrame::SortedMerge, which I think aims to do pretty much this same operation (merge sorted frame lists, given a comparison function). Do you know if it'd be possible to cleanly make use of that here, instead of adding a separate sorted-merge implementation? (That utility function is currently private, and you'd have to relax that in order to use it here -- but that seems probably fine.) That function's declaration: http://hg.mozilla.org/mozilla-central/annotate/0629918a09ae/layout/generic/nsIFrame.h#l3294 ...and definition: http://hg.mozilla.org/mozilla-central/annotate/0629918a09ae/layout/generic/nsIFrame.h#l3509 Seems superficially like we'd just want to call SortedMerge<YourCustomCompareFunc>(dest, src) and then reinitialize aDest with the result, or something like that.

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #10) > Comment on attachment 8710770 [details] [diff] [review] > part 1 - [css-grid] Refactor nsGridContainerFrame state and methods. > > + nsGridContainerFrame* mFrame; > > Let's make this a const pointer Fixed. > While you're at it, we might also want to change the mReflowState member up > a few lines to be a const pointer as well (it's already got a const value, > but not a const pointer): Fixed. > In the body of the GridReflowState constructor, please assert that either > mReflowState is null, or mFrame == mReflowState.frame. Fixed. > > (I was going to say "mFrame is technically redundant, because it's already > on mReflowState". But mReflowState might be null, which means there's value > in tracking the frame independently.) Yeah, I thought I was going to need mFrame more than I actually did. I'll consider removing it and pass it as an arg instead. > > +struct MOZ_STACK_CLASS nsGridContainerFrame::Grid > (1) This new struct needs a brief comment to explain what its purpose is Fixed. > (2) Are you sure you want to define so many of this struct's methods as > inline inside of the struct body? Yeah, nsGridContainerFrame.cpp is a bit large now so moving out the placement stuff to a separate file is probably a good idea. I'll keep these methods non-inlined to avoid churn. > If you like, lines like this one would benefit from a typedef at the top of > this .cpp file: > typedef nsGridContainerFrame::GridReflowState GridReflowState; OK, fixed. (In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #11) > Comment on attachment 8710772 [details] [diff] [review] > part 2 - [css-grid] Make GridItemInfo::mFrame available also in non-DEBUG > builds since we'll need it to support fragmentation. > > Similar to part 1, could you make this "nsIFrame* const"? (This is a > GridItemInfo member-var.) OK. (In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #13) > Comment on attachment 8710775 [details] [diff] [review] > part 5 - [css-grid] Create a couple of Grid container frame bits. Fixed those nits.

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #14) > Comment on attachment 8710777 [details] [diff] [review] > part 6 - [css-grid] Add support for creating Grid container continuations > > + NS_MergeReflowStatusInto(&aStatus, ocStatus); > > Do these lines really need to be all the way down here Yes, reflowing the children will assign aStatus. We could make it merge instead but I'm not sure it's worth the extra complexity. This is how it's usually done elsewhere, see nsColumnSetFrame::Reflow, nsFieldSetFrame::Reflow etc. Feel free to file a follow-up bug to address this accross the board if you feel it's worth fixing. > > + if (!(GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)) { > > + DisplayBorderBackgroundOutline(aBuilder, aLists); > > + } I just copy-pasted it from somewhere else ;-) The border-box block-size for Overflow Containers *should* be zero so it never draws any background or borders. (so for those two it's effectly just an optimization) I'm not entirely sure about outlines though... we probably should draw those around the overflow here. I need to investigate this more... Is it OK if I address this as a follow-up and do it for all frame types? It seems like it could be a more general problem. > Do you know how nsBlockFrame avoids painting > borders/backgrounds for its overflow containers? It looks like we have a bug for blocks - I filed bug 1249913.

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #28) > Comment on attachment 8716049 [details] [diff] [review] > part 9 - [css-grid] Create a SharedGridData object owned by the > > + mRows.mSizes is modified when breaks occur > > + * by modifying mPosition to remove the row gap at the break boundary > > I don't fully undrestand what this sentence is saying. OK, I rephrased it to make it clearer. > s/is used/are used/ Fixed. > Maybe rename to "InitializeFromFirstInFlow" or "InitializeForContinuation" > or something like that, to make it clearer that this is a > continuation-specific thing? Sure, I renamed it InitializeForContinuation. > > + for (auto pif = aGridContainerFrame->GetPrevInFlow(); > > + pif; pif = pif->GetPrevInFlow()) { > > + ++fragment; > > + firstInFlow = pif; > > + } > > This loop means that printing a grid will be an N^2 algorithm Yes I'm aware of the N^2 here, but it didn't seem like a big deal since we're not really doing any work in each step besides counting them. > Strawman example: here, maybe we could store the "current fragment ID" as a > uint32_t member-variable on the SharedGridData object That won't work since continuations can be reflowed in pretty much arbitrary order. I did consider adding a member on nsGridContainerFrame and set it lazily on reflow but I'm not sure it would work. It's the parent frame that creates/destroys our next-in-flows, but it might work if we can make the assumption that it never creates new next-in-flows in the middle of a continuation chain, and that it always also destroys the N+1 continuation if it destroyes the Nth. It might be worth trying since it's quite harmless if we miscount the number of breaks here. > > + for (uint32_t row = 0, breakCount = 0; row < numRows; ++row) { > > + if (rowSizes[row].mState & TrackSize::eBreakBefore) { > > + if (fragment == ++breakCount) { > > Hmm, looks like this will be n^2 too. (iterating with break-count going from > 1 up to |fragment|, effectively) True. > Maybe n^2 algorithms are unavoidable here? Fwiw, in the columnset case the number of continuations are bounded by the max number of columns (1000 IIRC). And the row count is max 19999. It seems unlikely that this will be a perf problem for any real web content though. (In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #30) > Comment on attachment 8716049 [details] [diff] [review] > part 9 - [css-grid] Create a SharedGridData object owned by the > I'm a bit concerned about n^2 algorithms noted here, but I suppose this is > r=me with these notes & comment 28 addressed how you see fit. I'm inclined to leave it as is for now. > > + if (mStartRow == numRows) { > > + mFragBStart = aConsumedBSize; > > Consider adding a comment here like: > // All of the grid's rows fit inside of previous grid-container fragments. Correct. I added that comment. > Why do we need to make our own private copy of the mCols & mRows arrays here > (distinct from the shared data)? Because we don't have shared data for the first-in-flow (and won't create it unless it's needed). I guess we could swap the array elements while reflowing continuations and then swap them back after we're done. I'll have to review the code to see if that's possible (i.e. that all changes done to the GridReflowState copy is what we want to share). I've added a XXX comment for now. I'll address this in a follow-up bug. > > + for (auto& itemInfo : mSharedGridData->mGridItems) { > > + if (itemInfo.mFrame == childFirstInFlow) { > > + mGridItems.AppendElement(GridItemInfo(child, itemInfo.mArea)); > > + break; > > This nested-loop seems like it's O(n^2), with n being the number of grid > items. > Can we avoid this? (Or at least cut it down to n^2 overall?) Maybe we > should be storing the shared grid items using a hash map, for constant-time > lookup, so that this nested loop will scale linearly instead of > quadratically, as the number of items & children grow? Maybe. I'd like to avoid any cost to non-fragmented reflow though. We only need the GridArea to use for item continuations, so maybe we could store a pointer into the shared data in a frame property when we create those continuations? (This makes the assumption that no-one else can arbitrarily create item continuations that the grid container isn't aware of, but perphaps that's ok.) Again, I added a XXX comment here. I think we can address this in a follow-up bug. > (1) consider renaming "children" to "absPosChildren", or "absPosKids", Fixed. > (2) I believe we could simplify this a bit with a range-based for loop, Fixed. > > + SharedGridData* mSharedGridData; > > Do we really need this pointer? I think it can be replaced with just a local > variable, since we only use it in a single function. Yes, it's used more in upcoming parts. > I'm a bit uneasy about adding a raw-pointer member-var that points to a > structure whose lifetime is non-obvious. (Particularly in pagination code, > which is notorious for having sec bugs from dangling pointers.) I'm not worried about this at all. Instances of GridReflowState has the lifetime of a single Reflow call so it's very transient (it's a MOZ_STACK_CLASS). There is no way it can outlive the SharedGridData object. > (2) Just add a comment here making the ownership clear > // [weak] owned by mFrame's first-in-flow. Sure, fixed. > (I think the /** double-asterisk might be a typo here, in this 1-line > comment? Not sure, maybe it has some significance I'm unaware of.) I thought "/**" has significance for the generated documention? Like the other javadoc-isms we use (@param @return etc) I thought our tools processed those with special meaning (it used to anyway, I don't know if that's still true). > > + bSize += gridReflowState.mRows.SumOfGridGaps(); > > + } else { > > + bSize = gridReflowState.mRows.GridLineEdge(numRows, > > + GridLineSide::eAfterGridGap); > > Could you add an explanatory comment? (Maybe they do produce the same thing, > and it's just that for the first-in-flow, we haven't computed the > line-positions yet? not sure.) Correct. All mPosition are zero until AlignJustifyContent runs. I added a note pointing this out. > > + nscoord prevTrackEnd(0); > > Nit: this should probably be "= 0" instead of "(0)" Fixed. > > + } else if (sharedGridData && !GetNextInFlow()) { > > + Properties().Delete(SharedGridData::Prop()); > > Do we really need the GetNextInFlow() check here? > > If we're complete, we know that our container is about to delete our > next-in-flow (if it exists), I think. So, I'd expect we should be OK to > delete our sharedGridData regardless of whether we have a next-in-flow, > because we know our next-in-flow is going away. > > The way this is structured right now, it looks like sharedGrid will stick > around after it's needed, until the next time a reflow happens. Right, I'm intentionally trying to avoid making assumptions about what the parent frame will do. I think it's fine deleting it on the next reflow *if* our next-in-flow is gone by then. (I think this case will happen very rarely in practice.)

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #31) > Comment on attachment 8716052 [details] [diff] [review] > part 10 - [css-grid] Add a few helper methods to do a break before a row, > Please add an assertion message to explain why this function is > block-axis-specific (& row-specific). E.g. > "Should only be fragmenting in block axis (between rows)"); It's a bit obvious, but sure. I guess it documents that these methods are meant to be used for implementing fragmentation. > > + nscoord prevRowEndPos(0); This is called direct-initialization, vs "= 0" which is value-initialization. There's a subtle difference which may make a difference if we at some point want to make nscoord a struct type (to overload its arithmetic operators to be saturating). > So: unless there's some reason to prefer & migrate towards this "(0)" > syntax, probably better to stay consistent and stick with "= 0". It probably won't matter when I think about it, so I'll use the "= 0" syntax for consistency. > > + auto& sz = mSizes[aRow - 1]; > > + prevRowEndPos = sz.mPosition + sz.mBase; > > Consider s/sz/prevSz/ inside this block, OK, fixed. (FYI, I've noticed that sz.mPosition + sz.mBase is quite common so I should add a BMost() convenience method for that, so the above can be written like: prevRowEndPos = mSizes[aRow - 1].BMost(); instead.) > > + void SetRowSize(uint32_t aRow, nscoord aSize) > > (1) Consider renaming to something like "AdjustRowSize", or "ResizeRow", or > something like that. OK, ResizeRow sounds fine to me. > (2) consider s/aSize/aNewSize/ -- that'll makes lines like "nscoord delta = > aSize - sz.mBase;" a bit easier to understand. Fixed. > As above, please add an assertion message here Fixed. Perhaps that'd be worth handling here with an early-return before the loop, > just like the early-return for gap==0 in this patch's other function, > BreakBeforeRow()? Meh, there's only one caller and it never calls this with the same size. > NS_WARN_IF_FALSE(delta != 0, "Useless call to SetRowSize); Sure.

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #32) > Comment on attachment 8720336 [details] [diff] [review] > part 11 - [css-grid] Add a GetNearestFragmentainer() method that collects > > + * Return a Fragmentainer object if we have a fragmentainer frame in our > > + * ancestor chain of containing block (CB) reflow states. All the CBs > > + * must have the same writing-mode and have overflow:visible. > > I think what you mean is: > "We'll only continue traversing the ancestor chain as long as the CBs have > the same writing-mode and have overflow:visible." OK, fixed. > logicalSkipSides is only used once; seems cleaner to just collapse it into > its one usage, like so: > bp.ApplySkipSides(GetLogicalSkipSides()); That code will be removed by a later part (as hinted in comment 20). > > + * True if the current fragment is not at the start of the fragmentainer. > > + */ > > + bool mIsTopOfPage; > > Maybe I'm misunderstanding, but it looks like the comment is wrong here -- I > think you want to drop the word "not"? Right, fixed.

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #33) > Comment on attachment 8716061 [details] [diff] [review] > part 12 - [css-grid] Collect and merge child frames we need for reflow. > > + for (nsFrameList::Enumerator e(aFrameList); !e.AtEnd(); e.Next()) { > > + ReparentFrame(e.get(), aOldParent, aNewParent); > > If you like, a range-based for loop would be a bit more concise: Fixed. > > +static void > > +MergeSortedFrameLists(nsFrameList& aDest, nsFrameList& aSrc, > > + nsIContent* aCommonAncestor) > > We already have nsIFrame::SortedMerge, which I think aims to do pretty much > this same operation (merge sorted frame lists, given a comparison function). > Do you know if it'd be possible to cleanly make use of that here, instead of > adding a separate sorted-merge implementation? Yes, this is a good observation. However, SortedMerge takes a static function as template arg which means we'd have to give up on the aCommonAncestor optimization. We could change it to take a Comparator object as an ordinary arg instead but then I'd be worried that's less efficient when you don't need it. Perhaps the compiler is smart enough to inline all of it so that there is no parameter passing at all and that the aComp.IsLessThanOrEqual call can be inlined. I'd have to look at the assembler to confirm that. E.g. something like this: nsIFrame::SortedMerge(nsIFrame* aLeft, nsIFrame* aRight, Comparator aComp) { ... if (aComp.IsLessThanOrEqual(aLeft, aRight)) ... } nsIFrame::MergeSort(nsIFrame* aSource, Comparator aComp) { ... SortedMerge(*left, current, aComp); ... } nsIFrame::SortFrameList(nsFrameList& aFrameList, Comparator aComp) { ... MergeSort(aFrameList.FirstChild(), aComp); ... } Then my code here could create a comparator with a mCommonAncestor member... Seems like fodder for a follow-up bug though, if it's worth it... I guess I could use a static variable to store aCommonAncestor since we know this code isn't multi-threaded, but that's a bit ugly... We'll not save many lines here though, since most of MergeSortedFrameLists is in fact the comparator function.

(this is the part that removes the 'logicalSkipSides' in GetNearestFragmentainer)

(In reply to Mats Palmgren (:mats) from comment #35) > I'm not entirely sure about outlines though... we probably should draw > those around the overflow here. I need to investigate this more... > > Is it OK if I address this as a follow-up and do it for all frame types? > It seems like it could be a more general problem. Sounds good -- yes, please do file a followup on that outlines issue. We probably just need an "else { DisplayOutline(...)" clause, to cover the NS_FRAME_IS_OVERFLOW_CONTAINER case. Thanks! > > Do you know how nsBlockFrame avoids painting > > borders/backgrounds for its overflow containers? > > It looks like we have a bug for blocks - I filed bug 1249913. Thanks!

Comment on attachment 8710777 [details] [diff] [review] part 6 - [css-grid] Add support for creating Grid container continuations and deal with overflow containers. [Marking part 6 r+, since comment 35 answers my questions/concerns in comment 14.]

I have a few minor improvements for part 12, here's the first two: 'foundOwnPushedChild' was only meant to be set for pushed items (i.e. frames that are the first-in-flow for an item). ('foundOwnPushedChild' is only used in the assertion you see at the end in this patch so this change makes that assertion stricter.) The second change moves the NS_STATE_GRID_DID_PUSH_ITEMS check earlier so that we skip looking for pushed items in the kOverflowList if it's not set. We can't have pushed items there unless this bit is set. (If it is set though, we *may* have pushed items there, or they may have been picked up by our next-in-flow already (who may have pushed them again etc). This is what the assertion above is meant to check - that we find at least one of these pushed frames when the bit is set.) (I'll extend this to also check each 'nif' in the next patch.) (No commit message -- I intend to fold it into part 12 before landing.)

... and this checks that each 'nif' with the bit set sees at least one pushed item in its kOverflowList or in its next-in-flows.

The invariant that two anonymous grid items can't be siblings doesn't hold anymore. E.g. A<x style="grid-row:2"/>B if only one row fits in a constrained height then <x> is pushed into a new fragment and the two anon items "A" and "B" will be siblings in the first fragment. So this patch moves the checks into SetInitialChildList so that it only checks the results after the initial frame construction. I think it still holds here. (I'll fill in SanityCheckAnonymousGridItems in the next patch with some other checks we can do on each reflow instead.)

This is kind of a complement to the assertions in part 12. It checks that NS_STATE_GRID_DID_PUSH_ITEMS is not set on |this| or any of its next-in-flows and that we have no stray pushed items. (This might be to heavy to run be default even in DEBUG builds, but let's leave it on while this code is still in development. We can put it behind some #ifdef later.)

FYI, the reason I'm beefing up the assertions is that I saw some intermittent assertions on one of the Android platforms (but nowhere else): ASSERTION: can't find GridItemInfo: 'mGridItems.Length() == len + 1' I think I understand how that can occur now, and I have a local testcase that reproduces it. The reason is that child frames can be removed/inserted dynamically into next-in-flows of the grid container. So when we reflow a first-in-flow without NS_STATE_GRID_DID_PUSH_ITEMS it will not pick up these "new items" and will not create a GridItemInfo for them, so when the next-in-flow tries to the info for such an inserted frame it will not find it. Hence the assertion. So I'll overload the Append/Insert/RemoveFrame methods and see if I can mitigate this somehow... So what I think happens on Android is that the network is very slow so after we have received just one or two packets or so we parse it and schedule a reflow. This happens to be in the middle of a grid inside a columnset, so we that created the condition "no bit, but a next-in-flow", then the parser resumed when more packets arrived but now all the new nodes are inserted dynamically instead (I'm seeing a call to IntertFrames on a grid next-in-flow), then we reflow due to that the assertion occurs. (It seems that we're *very* aggressive to schedule an initial reflow on Android for some reason. I wonder if that's intentional... it might lead to seeing something on the screen sooner but it also means we take slower path for the remaining content.)

(In reply to Mats Palmgren (:mats) from comment #48) > (It seems that we're *very* aggressive to schedule an initial reflow on > Android for some > reason. I wonder if that's intentional... it might lead to seeing something > on the screen > sooner but it also means we take slower path for the remaining content.) snorp: do you know if the reflow scheduling on Android is intentional?

(In reply to Jet Villegas (:jet) from comment #49) > > snorp: do you know if the reflow scheduling on Android is intentional? I am not aware of anything that would cause that specifically on Android, but it's possible there was some stuff added before my time. It definitely sounds sub-optimal. For a while I was working on being lazier here in bug 1150172, but didn't really end up seeing the benefits I wanted.

(In reply to Mats Palmgren (:mats) from comment #39) > (In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #33) > > Comment on attachment 8716061 [details] [diff] [review] > > part 12 - [css-grid] Collect and merge child frames we need for reflow. [...] > > We already have nsIFrame::SortedMerge, which I think aims to do pretty much > > this same operation (merge sorted frame lists, given a comparison function). > > Do you know if it'd be possible to cleanly make use of that here, instead of > > adding a separate sorted-merge implementation? > > Yes, this is a good observation. However, [...] [...] > We'll not save many lines here though, since most of MergeSortedFrameLists > is in fact the comparator function. Fair enough - sounds like it's not worth shoehorning this into nsIFrame::SortedMerge at this point.

I forgot to clear this array -- we need to do that because it's stored in the SharedGridData and we just append to it. (no commit message - I'll fold this into part 9)

So I wrote a bunch of tests that does DOM changes that are supposed to be idempotent: e.g. remove the first item, insert it again; and this revealed a few issues. This patch fixes two bugs in the part of part 16 that deals with child continuations. I've described it in detail in the commit message, let me know if you have questions.

(no changes, but this is now part 19)

(no changes but this is now part 20)

This is to maintain NS_STATE_GRID_DID_PUSH_ITEMS when new items are inserted. Basically we treat them as pushed items so that they get pulled up and reflowed properly. We also need to override DrainSelfOverflowList() so that we can do a merge of the two lists rather just append.

We always need to check NS_INLINE_IS_BREAK_BEFORE before testing on completion status, because it's simply not valid when we get NS_INLINE_IS_BREAK_BEFORE.

(9066 lines of tests!)

That should be all. Otsukare. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53cb3fedaf0

Actually, there is one addition to the sanity checks in part 20 that checks for the issues fixed in part 18. (no commit message - I'll fold this into part 20)

Comment on attachment 8716061 [details] [diff] [review] part 12 - [css-grid] Collect and merge child frames we need for reflow. Review of attachment 8716061 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 12, with nits below addressed: ::: layout/generic/nsGridContainerFrame.cpp @@ +192,5 @@ > + // NOTE: we get here when comparing ::before/::after for the same element. > + auto srcPseudo = src->GetContent()->NodeInfo()->NameAtom(); > + if (MOZ_UNLIKELY(srcPseudo == nsGkAtoms::mozgeneratedcontentbefore)) { > + auto destPseudo = dest->GetContent()->NodeInfo()->NameAtom(); > + if (!MOZ_UNLIKELY(destPseudo == nsGkAtoms::mozgeneratedcontentbefore) || I'd marginally prefer that the "!" be folded into the equality check here, i.e. if (MOZ_LIKELY(destPseudo != nsGkAtoms::mozgeneratedcontentbefore)) It feels more readable if the negation is actually part of the condition, instead of being separated from the condition by a macro. But, also not a big deal if you have a reason to prefer the way you've got it. @@ +194,5 @@ > + if (MOZ_UNLIKELY(srcPseudo == nsGkAtoms::mozgeneratedcontentbefore)) { > + auto destPseudo = dest->GetContent()->NodeInfo()->NameAtom(); > + if (!MOZ_UNLIKELY(destPseudo == nsGkAtoms::mozgeneratedcontentbefore) || > + !::IsPrevContinuationOf(dest, src)) { > + result = -1; Consider inverting the logic in the last subcondition -- i.e. instead of checking... !::IsPrevContinuationOf(dest, src) ...instead, check: ::IsPrevContinuationOf(src, dest) Seems to me that we should *only* affirmatively change the comparison-result from 0 to -1 (indicating that 'src' should sort first) *if* we can tell that src is a prev continuation of dest (and they're both ::before). (The way you've got it right now -- where we change the result to -1 if dest is *not* a prev-continuation of src -- makes less sense to me. The negation ("!") adds some cognitive burden to interpreting the condition -- and in the abstract scenario where neither dest nor src is a prev-continuation of the other, it makes more sense to leave |result| at 0 [as my rewriting would do] rather than to change it to -1 [as the patch would currently do]. As I see it, this if/else cascade is all about finding the scenarios where |src| should *clearly* sort first.) @@ +4116,5 @@ > > + // First we gather child frames we should include in our reflow, > + // i.e. overflowed children from our prev-in-flow, and pushed first-in-flow > + // children (that might now fit). It's important to note that these children > + // can be in arbitrary order visavi the current children in our child lists. Typo: s/visavi/vis-a-vis/ https://en.wikipedia.org/wiki/Vis-%C3%A0-vis @@ +4128,5 @@ > + // our child lists. (The sorting is trivial given that both lists are > + // already fully sorted individually - it's just a merge.) > + // > + // The invariants that we maintain are that all grid container child lists > + // are sorted in the normal document order at all times, but that children s/all...lists are sorted/each...list is sorted/ ("each list" is clearer about this being a per-list thing, with no larger implications/guarantees.) @@ +4178,5 @@ > + ourOverflow->RemoveFrame(f); > + items.AppendFrame(nullptr, f); > +#ifdef DEBUG > + foundOwnPushedChild = true; > +#endif You can drop the #ifdef DEBUG guards here -- in opt builds, the compiler should optimize this assignment away to nothing (or else DebugOnly isn't doing its job). @@ +4200,5 @@ > + for (nsIFrame* nifChild = nif->GetChildList(kPrincipalList).FirstChild(); > + nifChild; ) { > + nsIFrame* next = nifChild->GetNextSibling(); > + if (!nifChild->GetPrevInFlow()) { > + nif->StealFrame(nifChild); StealFrame is fallible, according to its return type & its documentation in nsContainerFrame.h: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.h?rev=7a5912b5ab4e#395 We need to handle its failure here (either by returning, or by asserting that it NS_SUCCEEDED with a brief justification, if you're really confident that it must always succeed here). @@ +4211,5 @@ > + for (nsIFrame* nifChild = nif->GetChildList(kOverflowList).FirstChild(); > + nifChild; ) { > + nsIFrame* next = nifChild->GetNextSibling(); > + if (!nifChild->GetPrevInFlow()) { > + nif->StealFrame(nifChild); Same here. (Need to handle or assert about StealFrame failure.)

Comment on attachment 8716066 [details] [diff] [review] part 13 - [css-grid] Refactor ReflowChildren() by separating out the code that reflows normal flow children (grid items and placeholders) into a new method ReflowInFlowChild(). Review of attachment 8716066 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 13, with nits below addressed as you see fit: ::: layout/generic/nsGridContainerFrame.cpp @@ +3958,5 @@ > + const LogicalPoint padStart(wm, pad.IStart(wm), pad.BStart(wm)); > + const bool isGridItem = !!aGridItemInfo; > + MOZ_ASSERT(isGridItem == (aChild->GetType() != nsGkAtoms::placeholderFrame)); > + LogicalRect cb(wm); > + WritingMode childWM = aChild->GetWritingMode(); The childWM decl seems to be moving a bit earlier here, for no clear reason. (In the old version of this code, childWM is declared right before its first usage, in the childCBSize decl. But here, it's declared 10 lines earlier, in between some seemingly-unrelated stuff.) Consider moving it back down to where it was previously declared (just before childCBSize), since it makes more sense there (IMO) and since this patch is aiming to be a cut-and-paste as much as possible. @@ +3983,5 @@ > + Maybe<nsHTMLReflowMetrics> childSize; // Maybe<> so we can reuse the space > + childSize.emplace(*childRS); > + const nsSize dummyContainerSize; > + ReflowChild(aChild, pc, *childSize, *childRS, childWM, LogicalPoint(childWM), > + dummyContainerSize, 0, aStatus); Previously (in the code that's getting removed here), we gave each child its own |childStatus| for its reflow. Now we're reusing our own reflow status and passing it to each child, to do with as it likes. Is that bad? Shouldn't we be using a clean status for each child, and then merge / react to them as-needed?

Comment on attachment 8716078 [details] [diff] [review] part 14 - [css-grid] Make ReflowInFlowChild() deal with a constrained available block-size. Review of attachment 8716078 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 14 I suppose, though I think the reflowCB & childCBSize code needs some reworking as noted below, and that might want another round of review. ::: layout/generic/nsGridContainerFrame.cpp @@ +3971,5 @@ > const bool isGridItem = !!aGridItemInfo; > MOZ_ASSERT(isGridItem == (aChild->GetType() != nsGkAtoms::placeholderFrame)); > LogicalRect cb(wm); > WritingMode childWM = aChild->GetWritingMode(); > + bool constrainedBSize = false; nit: give this variable an "is" or "has" prefix. (Otherwise, this variable's name makes it sound like it's a nscoord (a constrained BSize measurement), which makes its usages a bit harder to parse when reading the code.) @@ +3980,5 @@ > MOZ_ASSERT(area.IsDefinite()); > cb = aState.ContainingBlockFor(area); > + constrainedBSize = aFragmentainer && !wm.IsOrthogonalTo(childWM); > + if (constrainedBSize) { > + // Subtract the "consumed" part of the grid area. This comment really belongs a few lines down, inside of the "if (fragCBOffset < 0) {" clause, I think. (That's the only place where we actually *know* that some of this grid area has been consumed, and where we subtract the consumed amount off our our cb's size.) @@ +3983,5 @@ > + if (constrainedBSize) { > + // Subtract the "consumed" part of the grid area. > + nscoord fragCBOffset = cb.BStart(wm) - aState.mFragBStart; > + if (fragCBOffset < 0) { > + cb.BSize(wm) = std::max(fragCBOffset + cb.BSize(wm) , 0); Nit: Drop the stray space character before the comma here. @@ +3987,5 @@ > + cb.BSize(wm) = std::max(fragCBOffset + cb.BSize(wm) , 0); > + } > + cb.BStart(wm) = std::max(fragCBOffset, 0); > + toFragmentainerEnd = aFragmentainer->mToFragmentainerEnd - aState.mFragBStart; > + toFragmentainerEnd = std::max(toFragmentainerEnd - cb.BStart(wm), 0); Two nits: (1) The first toFragmentainerEnd assignment here looks like it's more than 80 chars. (but, read on) (2) I'm not clear on why we subtract cb.BStart inside of the max expression, instead of in the initial toFragmentainerEnd initialization. It makes the variable's meaning a bit harder to follow. If the var means distance from start of |cb| to the end of our fragmentainer (which I think is what it aims to mean), this should just be: toFragmentainerEnd = aFragmentainer->mToFragmentainerEnd - (aState.mFragBStart + cb.BStart(wm)); toFragmentainerEnd = std::max(toFragmentainerEnd, 0); ...or something like that. (That should be the same math as what your patch has, but it's got all of the subtraction up-front, and then the clamping to 0 as an afterthought -- rather than doing some subtraction-and-clamping simultaneously as an afterthought.) @@ +3999,5 @@ > + if (constrainedBSize) { > + reflowCB.BSize(wm) = toFragmentainerEnd; > + } > + LogicalSize childCBSize = reflowCB.Size(wm).ConvertTo(childWM, wm); > + LogicalSize percentBasis(cb.Size(wm).ConvertTo(childWM, wm)); This chunk of new code (from the "reflowCB" declaration, down to the NS_UNCONSTRAINEDSIZE assignment just after this quote) is all pretty confusing (and maybe wrong) for several reasons: (a) In the no-fragmenting scenario, reflowCB just ends up being a 0-sized rect at 0,0. (which makes it not really much of a "reflow CB" -- so its name and usage doesn't make much sense) (b) As a result, childCBSize's initialization from reflowCB seems kinda useless in the non-fragmenting scenario. (c) childCBSize doesn't ever seem to get its ISize set at all. (except maybe from taking reflowCB.BSize, in the edge case where the writing-modes are orthogonal and we're fragmenting) (d) percentBasis is declared & set in between some lines that focus on childCBSize. So it's kind of in the middle of some unrelated stuff. Could you restructure this to: - initialize percentBasis either before or after we're done setting up childCBSize, to make the scope of the childCBSize setup-work a little clearer. - Rework the childCBSize setup so that it's clearer that it has a useful BSize *and* ISize. - Remove reflowCB, or rename it and/or scope it to make it clearly fragmenting-related (and only use it when it's got useful value) Or if you think some of this doesn't need reworking, some explanatory code-comments could help here too. (Or if I'm completely missing where some of this stuff is initialized, let me know, too. I checked the final roll-up patch, in case later patches make this make more sense, but it looks like all of the above concerns still apply to the final code.) ::: layout/generic/nsIFrame.h @@ +716,5 @@ > + nscoord ContentBSize(mozilla::WritingMode aWritingMode) const { > + auto bp = GetLogicalUsedBorderAndPadding(aWritingMode); > + bp.ApplySkipSides(GetLogicalSkipSides()); > + auto sz = GetLogicalSize(aWritingMode); > + return std::max(0, sz.BSize(aWritingMode) - bp.BStartEnd(aWritingMode)); If you like, seems like the last 2 lines could be simplified (and made a bit more readable) by making use of the existing BSize() function (declared/defined directly above this). You can drop the "auto sz" line entirely, and drop "sz." from the other line to just have: return std::max(0, BSize(aWritingMode) - bp.BStartEnd(aWritingMode));

Comment on attachment 8721800 [details] [diff] [review] part 15 - [css-grid] Compute our pre-reflow logical skip sides and cache the result of ComputedLogicalBorderPadding() with that applied. Review of attachment 8721800 [details] [diff] [review]: ----------------------------------------------------------------- Marking this patch r- for now, since the main change here is the inlined GetLogicalSkipSides() code, and I suspect we don't want that change (pretty sure we can just call out to the normal GetLogicalSkipSides function, as noted in my second comment below). ::: layout/generic/nsGridContainerFrame.cpp @@ +1572,5 @@ > */ > nscoord mFragBStart; > /** The start row for this fragment. */ > uint32_t mStartRow; > + /** Our tentative ApplySkipSides bits - see below. */ "below" is a bit too vague here (and perhaps fragile in the face of future refactoring/code-reorderings, new code added here, etc.) Maybe s/see below/see GridReflowState constructor/ (which I think is the "below" that you're referring to). @@ +1606,5 @@ > + if (aReflowState) { > + mBorderPadding = aReflowState->ComputedLogicalBorderPadding(); > + // An inlined version of GetLogicalSkipSides that ignores GetNextInFlow(), > + // i.e. all fragments assume they're the last and should apply the block- > + // end border (except overflow containers which never have borders). The normal nsSplittableFrame::GetLogicalSkipSides implementation already does this (ignoring GetNextInFlow), if you pass in 'aReflowState': http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSplittableFrame.cpp?rev=b3c868596068&mark=255-270#239 Is there a reason we can't just call GetLogicalSkipSides(aReflowState) here? Seems like we should do that if possible, to avoid code-duplication & for maintainability. @@ +1608,5 @@ > + // An inlined version of GetLogicalSkipSides that ignores GetNextInFlow(), > + // i.e. all fragments assume they're the last and should apply the block- > + // end border (except overflow containers which never have borders). > + // We'll skip that border at the end of Reflow instead when we know > + // our reflow status. (If this comment is still around after you've addressed the above review comment): "We'll skip" isn't necessarily true here. Add a qualifier, something like: s/We'll skip/If we need fragments after this one, we'll skip/ @@ +4515,5 @@ > > + // Skip our block-end border if we're INCOMPLETE. > + if (!NS_FRAME_IS_COMPLETE(aStatus) && > + !gridReflowState.mSkipSides.BEnd() && > + StyleBorder()->mBoxDecorationBreak != s/StyleBorder()/aReflowState.mStyleBorder/ (It's a bit cheaper to use the cached reflow state style-struct pointers -- that's why they exist.) @@ +4517,5 @@ > + if (!NS_FRAME_IS_COMPLETE(aStatus) && > + !gridReflowState.mSkipSides.BEnd() && > + StyleBorder()->mBoxDecorationBreak != > + NS_STYLE_BOX_DECORATION_BREAK_CLONE) { > + bp.BEnd(wm) = nscoord(0); Just use "= 0" -- drop the nscoord() wrapper.

Comment on attachment 8723025 [details] [diff] [review] part 16 - [css-grid] Implement fragmentation. Review of attachment 8723025 [details] [diff] [review]: ----------------------------------------------------------------- Review for part 16, "wave 1" (up to the beginning of ReflowRowsInFragmentainer): ::: layout/generic/nsGridContainerFrame.cpp @@ +31,5 @@ > typedef nsGridContainerFrame::TrackSize TrackSize; > const uint32_t nsGridContainerFrame::kTranslatedMaxLine = > uint32_t(nsStyleGridLine::kMaxLine - nsStyleGridLine::kMinLine); > const uint32_t nsGridContainerFrame::kAutoLine = kTranslatedMaxLine + 3457U; > +typedef nsTHashtable< nsPtrHashKey<nsIFrame> > FrameHashtable; If you like, you can drop the spaces inside of < ... > -- that used to cause ambiguity from ">>" but it no longer does in any compiler we care about -- see line for "Foo<Bar<T>>" in https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code @@ +61,5 @@ > +ClampToCSSMaxBSize(nscoord aSize, const nsHTMLReflowState* aReflowState) > +{ > + auto maxSize = aReflowState->ComputedMaxBSize(); > + if (MOZ_UNLIKELY(maxSize != NS_UNCONSTRAINEDSIZE)) { > + maxSize = std::max(aReflowState->ComputedMinBSize(), maxSize); This clamping (checking e.g. max-height against min-height) is unnecessary -- nsHTMLReflowState should already enforce this internally, at the end of nsHTMLReflowState::ComputeMinMaxValues: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=4bd6ad569411#2917 So, you should get rid of this line (though maybe worth asserting that it holds, if you like). @@ +4243,5 @@ > + for (auto child : placeholders) { > + nsReflowStatus childStatus; > + ReflowInFlowChild(child, nullptr, aContainerSize, &aFragmentainer, > + aState, aContentArea, aDesiredSize, childStatus); > + MOZ_ASSERT(NS_FRAME_IS_COMPLETE(childStatus)); Please include an explanatory/descriptive assertion message here; e.g. "nsPlaceholderFrame should never need to be fragmented" @@ +4246,5 @@ > + aState, aContentArea, aDesiredSize, childStatus); > + MOZ_ASSERT(NS_FRAME_IS_COMPLETE(childStatus)); > + } > + > + // The avaliable size for children, we'll set this to the edge of the last 2 typos for this line: (1) spelling: "avaliable" --> "available" (2) grammar: "children, we'll" --> "children - we'll" (or use a period or a semicolon, just not a comma) @@ +4250,5 @@ > + // The avaliable size for children, we'll set this to the edge of the last > + // row in most cases below, but for now use the full size. > + nscoord childAvailableSize = aFragmentainer.mToFragmentainerEnd; > + uint32_t startRow = aState.mStartRow; > + const uint32_t numRows = aState.mRows.mSizes.Length(); Nit: looks like |startRow| can be declared as 'const', just like |numRows| is. (It doesn't seem to ever change.) @@ +4260,5 @@ > + uint32_t endRow = numRows; > + for (uint32_t row = startRow; row < numRows; ++row) { > + auto& sz = aState.mRows.mSizes[row]; > + const nscoord bEnd = sz.mPosition + sz.mBase; > + nscoord gap = childAvailableSize - bEnd; "gap" gives the implication that this has something to do with "grid-gap" (and it does not). Please rename this to something more descriptive like "remainingAvailableSize". @@ +4262,5 @@ > + auto& sz = aState.mRows.mSizes[row]; > + const nscoord bEnd = sz.mPosition + sz.mBase; > + nscoord gap = childAvailableSize - bEnd; > + if (gap < 0 || (bdbClone && gap < bpBEnd)) { > + endRow = row; Remove end-of-line whitespace. @@ +4280,5 @@ > + auto disp = info->mFrame->StyleDisplay(); > + if (disp->mBreakBefore) { > + // Propagate break-before on the first row to the container unless we're > + // already at top-of-page. > + if ((itemStartRow == 0 && !isTopOfPage) || avoidBreakInside) { Do we actually have to check isTopOfPage here? I think "avoidBreakInside" already implicitly checks that on aState.mReflowState (in the ShouldAvoidBreakInside() impl). (Put another way: is aFragmentainer.mIsTopOfPage guaranteed to match aState.mReflowState->mIsTopOfPage here? [I think the answer is "yes"]. If so, I don't think we need to bother with the local |isTopOfPage| variable in this function.) Or if we really do need to check aFragmentainer.mIsTopOfPage here, maybe add a comment explaining why our ShoulAvoidBreakInside() call's internal mIsTopOfPage check isn't sufficient. @@ +4290,5 @@ > + itemStartRow < endRow) { > + endRow = itemStartRow; > + isForcedBreak = true; > + // reset any BREAK_AFTER we found on an earlier item > + aStatus = NS_FRAME_COMPLETE; NS_FRAME_COMPLETE seems like a very odd status to be setting, when we actually know we're going to force a break. :) I think this assignment is really only trying to clear the BREAK_AFTER stuff that we tentatively set -- but it still seems very strange. The comment kind of implies that, but doesn't really explain why we're using COMPLETE, and it doesn't address the fact that COMPLETE contradicts the fact that we know we'll be breaking. Plus, if our caller happened to pass in a non-COMPLETE aStatus, then this would completely stomp on that value, which is bad. (Not sure if that can happen, but we don't seem to sanity-check the input at all right now, so it seems conceivable.) SO: I think we could handle this more cleanly by avoiding the whole "tentative" aStatus assignment. We can just use a helper nsReflowStatus local-var instead, which we only copy into aStatus when we're really sure. Something like: nsFrameStatus tentativeStatus = aStatus; // used if we find a "break-after" item for (const GridItemInfo* info : sortedItems) { uint32_t itemStartRow = info->mArea.mRows.mStart; if (itemStartRow == endRow) { // Make any "break-after" that we hit on this row take effect: aStatus = tentativeStatus; break; } [...] if (disp->mBreakBefore) { // (no need to set aStatus to a confusingly contradictory // NS_FRAME_COMPLETE value here anymore. We can just leave // the tentative value in tentativeStatus and forget about it.) [...] } if (disp->mBreakAfter) { [...] // This will be copied into aStatus when we complete this row, // unless we find an item with 'break-before' first. tentativeStatus = NS_INLINE_LINE_BREAK_AFTER(tentativeStatus); } (The above code is meant to be a skeleton of what you've already got, except with tentativeStatus inserted in a few choice places.) Alternately: we could do this with a "Maybe<nsReflowStatus> breakAfterStatus;" which we'd only emplace in the break-after section, and which we only copy into aStatus if it's been emplaced. Either of these options (tentativeStatus which we unconditionally set in our endRow clause, or Maybe<> breakAfterStatus which we conditionally set) seems cleaner than the eagerly-setting-aStatus strategy in the patch right now. @@ +4318,5 @@ > + (startRow != 0 || !aFragmentainer.mCanBreakAtStart)) { > + ++endRow; > + } > + > + // Honor avoid-break:inside if we can't fit all rows. I think you mean "break-inside: avoid"? (not "avoid-break") @@ +4385,5 @@ > + } else if (bSize <= bEndRow && startRow + 1 < endRow) { > + if (endRow == numRows) { > + // We have more than one row in this fragment, so we can break before > + // the last row instead. > + --endRow; I'm not clear how we get into this situation. Further up in this function, this patch purports to determine |endRow| so that we won't have to do this, I think. (per the comment "Set |endRow| to the first row that doesn't fit.") So if that higher-up code determines endRow correctly, how would we get into a situation where we need to decrement endRow in order for stuff to fit here? @@ +4412,5 @@ > + > + // Children always have the full size of the rows in this fragment. > + if (childAvailableSize < bEndRow) { > + childAvailableSize = bEndRow; > + } If you like, this can collapse to: childAvailableSize = std::max(childAvailableSize, bEndRow); to make this a one-liner and remove a little bit of variable-name-repetition. @@ +4416,5 @@ > + } > + > + // If we can't fit all rows then we're at least overflow-incomplete. > + if (endRow < numRows) { > + childAvailableSize = bEndRow; Do we need this "childAvailableSize = bEndRow;" assignment anymore? We just ensured that childAvailableSize was at least bEndRow in the code directly above this. It's not clear to me why we'd need a slightly-different version of the same assignment here. @@ +4427,5 @@ > + return ReflowRowsInFragmentainer(aState, aContentArea, aDesiredSize, aStatus, > + aFragmentainer, aContainerSize, sortedItems, > + startRow, endRow, bSize, childAvailableSize); > +} > + whitespace on blank line ::: layout/generic/nsGridContainerFrame.h @@ +137,5 @@ > } > > /** > * Reflow and place our children. > + // @return the consumed size of all continuations so far including this frame Two nits: (1) This "// inside of /**/" syntax seems broken. Unless this is some weird doxygen-ism, I think you want * at the start of this line, not //. (2) "all continuations so far" is perhaps too vague here, because this function is focused on children, which means it sounds like it might be talking about all continuations of our children. But I think you really mean "all of this grid container's continuations so far". So, consider replacing "all continuations" with "all of this grid container's continuations", or "all grid-container continuations", or something like that. (This same language appears in documentation for other functions further down in this patch, but I'm less concerned about this down there -- those other functions aren't focused on children, so it's clearer what "all continuations" means in those contexts.)

Comment on attachment 8723025 [details] [diff] [review] part 16 - [css-grid] Implement fragmentation. Review of attachment 8723025 [details] [diff] [review]: ----------------------------------------------------------------- Review comments for part 16, "wave 2" (of 2) This is mostly r=me, but I should probably hold off, because there are some key pieces of the logic flow that I'm not 100% clear on (questions in comment 70). Marking feedback+ for now; will upgrade to r+ with feedback addressed as you see fit & questions resolved. ::: layout/generic/nsGridContainerFrame.cpp @@ +4445,5 @@ > +{ > + FrameHashtable pushedItems; > + FrameHashtable incompleteItems; > + FrameHashtable overflowIncompleteItems; > + const bool bdbClone = MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak == Two nits: (1) Give this an "is" or "has" prefix, to make it more obviously a boolean (as opposed to e.g. a pointer to some cloned object) at its "if (bdbClone)" usage sites. (2) s/StyleBorder()/aReflowState.mStyleBorder/ -- should be a bit more efficient, as noted in comment 15. (These nits both apply to the "bdbClone" variable in this patch's other function, ReflowInFragmentainer, too.) @@ +4448,5 @@ > + FrameHashtable overflowIncompleteItems; > + const bool bdbClone = MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak == > + NS_STYLE_BOX_DECORATION_BREAK_CLONE); > + bool didGrowRow = false; > + bool isTopOfPage = aStartRow != 0 || !aFragmentainer.mCanBreakAtStart; This line is confusing to me. (1) I don't see how either of these conditions (nonzero row or not being able to break at the start) indicate that we're at the top of the apge. (2) aFragmentainer and aState.mState.mFlags each have their own member-var "mIsTopOfPage", and we're ignoring each of those member-vars here. (I assume for good reason. But, it's not clear how this new "isTopOfPage" variable differs from those other two that are already in play here.) Please add a comment to clarify the above. (Maybe an assertion, too, if there's anything useful we can assert about the relationship(s) between these isTopOfPage variables.) @@ +4456,5 @@ > + // Propagate the constrained size to our children. > + aFragmentainer.mToFragmentainerEnd = aAvailableSize; > + // Reflow the items in row order up to |aEndRow| and push items after that. > + uint32_t row = 0; > + for (int32_t i = 0, len = aSortedItems.Length(); i < len; ++i) { I was going to say: "s/int32_t/uint32_t/ for consistency with your other similar loops in this file." ...but then a bit later, I saw that you explicitly set i = -1, in some cases, much further down in this loop. So it does really want to be signed. SO: to prevent others from making that same mistake that I just made, please add a comment here saying e.g.: // "i" is intentionally signed, so we can set it to -1 to restart loop. Otherwise someone might inadvertently clean this up to uint32_t or size_t in the future. (which would probably be OK & produce the right behavior since unsigned overflow is defined in C++, but would make this pretty hacky) @@ +4466,5 @@ > + continue; > + } > + if (row > aStartRow) { > + // XXX make zero-sized row with grid-row-gap:0 still top-of-page? > + isTopOfPage = false; In addition to this check, can you assert that row >= aStartRow somewhere here? (I don't think we're expecting to see any items that are from before aStartRow, right? I doubt we want to be reflowing any such items as part of reflowing this fragment...) @@ +4471,5 @@ > + } > + > + // Can we grow this row? Only consider span=1 items per spec... > + bool rowCanGrow = !didGrowRow && info->mArea.mRows.Extent() == 1; > + nscoord maxRowSize = nscoord(0); "nscoord(0)" --> "0" @@ +4494,5 @@ > + > + // aFragmentainer.mIsTopOfPage is propagated to the child reflow state. > + // When it's false the child can request BREAK_BEFORE. We intentionally > + // set it to false when the row is growable per CSS Grid §13.1 and there > + // is a non-zero space between it and the fragmentainer end (that can be RE "per CSS Grid §13.1" here -- don't understand that reference. It sounds like you're saying "we set it to false... per CSS Grid §13.1", but that section of the spec doesn't mention breaks or pages at all, so I don't see what the connection is between setting mIsTopOfPage=false & that spec section. Maybe you mean to say "...is growable (as determined in CSS Grid §13.1) ..."? (If so, that rewording might make this clearer.) Though even so, I still don't exactly see what spec-text you'd be referencing RE which rows are growable vs. not growable in that section. Is there a more specific spec section / term you can reference here? In any case: please clarify this spec reference, one way or another. @@ +4505,5 @@ > + nsReflowStatus childStatus; > + ReflowInFlowChild(child, info, aContainerSize, &aFragmentainer, > + aState, aContentArea, aDesiredSize, childStatus); > + MOZ_ASSERT(!NS_FRAME_IS_FULLY_COMPLETE(childStatus) || > + !child->GetNextInFlow()); (Ideally, any nontrivial assertion should have a message. This one might want: "fully-complete reflow should destroy any NIFs") @@ +4508,5 @@ > + MOZ_ASSERT(!NS_FRAME_IS_FULLY_COMPLETE(childStatus) || > + !child->GetNextInFlow()); > + > + if (NS_INLINE_IS_BREAK_BEFORE(childStatus)) { > + MOZ_ASSERT(!aFragmentainer.mIsTopOfPage, "got NS_INLINE_IS_BREAK_BEFORE at top of page"); This line's too long -- wrap the message to a second line. @@ +4512,5 @@ > + MOZ_ASSERT(!aFragmentainer.mIsTopOfPage, "got NS_INLINE_IS_BREAK_BEFORE at top of page"); > + if (!didGrowRow) { > + if (rowCanGrow) { > + // Grow this this row and restart with the next row as |aEndRow|. > + MOZ_ASSERT(maxRowSize > aState.mRows.mSizes[row].mBase); Add message, e.g. "rowCanGrow lied" @@ +4537,5 @@ > + // We can break before this row - restart with it as the new end row. > + aEndRow = row; > + aBSize = aState.mRows.GridLineEdge(aEndRow, GridLineSide::eBeforeGridGap); > + isTopOfPage = savedIsTopOfPage; > + i = -1; // i == 0 after the next loop increment Please swap these two lines (the isTopOfPage assignment & the "i" assignment), either here or in the other "restart" clause, so that they're consistent. (I'd like the two "restart" codepaths to look as similar as possible, so that it's easier to see what actually differs between them.) @@ +4540,5 @@ > + isTopOfPage = savedIsTopOfPage; > + i = -1; // i == 0 after the next loop increment > + overflowIncompleteItems.Clear(); > + incompleteItems.Clear(); > + NS_FRAME_SET_INCOMPLETE(aStatus); If we need this aStatus-tweak here in this second restart clause, shouldn't there be an instance of this same line in the first restart clause, too? (perhaps guarded with an "if (aEndRow is actually changing)" check) In both restart clauses, we're potentially changing the fate of some rows that we previously thought would fit on this page -- so we might be going from everything hopefully-fitting to some stuff not fitting. (I assume that's why you need this aStatus tweak here.) (In the first clause, we're really only making those sorts of fate-changes if the "aEndRow = row + 1" assignment there actually changes anything. If aEndRow already happened to be row + 1, then we don't need to tweak aStatus. But otherwise I'd expect we do need to tweak it.) @@ +4602,5 @@ > + } else { > + MOZ_ASSERT(childNIF->GetParent() != this || > + !mFrames.ContainsFrame(childNIF), > + "child's NIF shouldn't be in the same principal list"); > + if (childNIF->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) { Please add a comment to sum up the idea for the code inside this chunk. E.g.: "If child's existing NIF is an overflow container, convert it to an actual NIF, since now |child| has non-overflow stuff to give it." @@ +4604,5 @@ > + !mFrames.ContainsFrame(childNIF), > + "child's NIF shouldn't be in the same principal list"); > + if (childNIF->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) { > + auto parent = childNIF->GetParent(); > + parent->StealFrame(childNIF); As noted above, we should technically be checking the return value of StealFrame (perhaps simply with a DebugOnly<nsresult> rv, which we assert NS_SUCCEEDED). @@ +4619,5 @@ > + childNIF = fc->CreateContinuingFrame(pc, child, this); > + childNIF->AddStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER); > + overflowIncompleteList.AppendFrame(nullptr, childNIF); > + } else { > + while (childNIF && !childNIF->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER)) { This line's too long; needs to wrap after the "&&". Also: add a comment just before the while loop, to sum up this chunk. E.g.: "If child has any non-overflow-container NIFs, convert them to overflow containers, since that's all child needs now." @@ +4621,5 @@ > + overflowIncompleteList.AppendFrame(nullptr, childNIF); > + } else { > + while (childNIF && !childNIF->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER)) { > + auto parent = childNIF->GetParent(); > + parent->StealFrame(childNIF); (need to check or assert about StealFrame return value here, too.) @@ +4654,5 @@ > + } > + } > + if (!overflowIncompleteList.IsEmpty()) { > + auto eoc = static_cast<nsFrameList*>(Properties().Get( > + ExcessOverflowContainersProperty())); Please use the safer "GetPropTableFrames" getter here: nsFrameList* eoc = GetPropTableFrames(ExcessOverflowContainersProperty());

Comment on attachment 8724471 [details] [diff] [review] part 17 - [css-grid] Add helper methods that add a sorted list of child frames to the Overflow and ExcessOverflowContainers child lists. Review of attachment 8724471 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 17 -- just 2 nits: ::: layout/generic/nsGridContainerFrame.cpp @@ +5177,5 @@ > +} > + > +void > +nsGridContainerFrame::MergeSortedExcessOverflowContainers(nsFrameList& aList) > +{ Hmm, these functions are basically identical, aside from the choice of frame list & the fact that we have some convenience functions (GetOverflowFrames/SetOverflowFrames) that we use in the first function. For maintainability, seems like we might want to remove the first version (MergeSortedOverflow), & just use this second impl, with the caller passing in the frame list ID for the list that it wants. (OverflowContainersProperty() vs ExcessOverflowContainersProperty()) But, maybe the code-duplication is an acceptable price to pay for the easier convenience in the caller. @@ +5185,5 @@ > + MOZ_ASSERT(aList.FirstChild()->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER), > + "this is the wrong list to put this child frame"); > + MOZ_ASSERT(aList.FirstChild()->GetParent() == this); > + auto eoc = static_cast<nsFrameList*>(Properties().Get( > + ExcessOverflowContainersProperty())); Please use the safer "GetPropTableFrames" getter here.

(In reply to Daniel Holbert [:dholbert] (mostly AFK March 3rd - 6th) from comment #72) > Comment on attachment 8724471 [details] [diff] [review] > part 17 - [css-grid] Add helper methods that add a sorted list of child > frames to the Overflow and ExcessOverflowContainers child lists. [...] > Hmm, these functions are basically identical, aside from the choice of frame > list & the fact that we have some convenience functions > (GetOverflowFrames/SetOverflowFrames) that we use in the first function. Sorry, disregard this comment about merging these functions. (I've added a tag "disregard-first-nit" to comment 72 as well, in the hopes that you don't waste time writing an answer for this part.) (I was mixing up the "Overflow" list & the "Overflow Containers" list, and I didn't initially notice the key "!" in MergeSortedOverflow's NS_FRAME_IS_OVERFLOW_CONTAINER assertion)

Comment on attachment 8724477 [details] [diff] [review] part 18 - [css-grid] Fix a couple of bugs in how we handle child existing continuations when pushing/pulling children. Review of attachment 8724477 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the detailed explanation of scenarios in the extended commit message here! r=me, nits below: ::: layout/generic/nsGridContainerFrame.cpp @@ +4599,5 @@ > if (!childNIF) { > childNIF = fc->CreateContinuingFrame(pc, child, this); > incompleteList.AppendFrame(nullptr, childNIF); > } else { > + auto parent = static_cast<nsGridContainerFrame*>(childNIF->GetParent()); Now that you've got a local variable for childNIF->GetParent(), you should probably simplify the assertion on the next line. It currently starts with: MOZ_ASSERT(childNIF->GetParent() != this || but it can now be: MOZ_ASSERT(parent != this || @@ +4613,5 @@ > + } else { > + if (parent == GetNextInFlow()) { > + nsFrameList toMove(childNIF, childNIF); > + parent->MergeSortedOverflow(toMove); > + } else { Could you add a comment to explain the "parent == GetNextInFlow()" special case here? (where we call MergeSortedOverflow instead of reparenting & appending to incompleteList) I had to stare at this for a bit to figure out what we were doing differently & why. I think it's just an optimization, right? I *think* the explanation is something like the following: // If childNIF already lives on the next grid fragment, then // we don't need to reparent it, since we know it's destined to // end up there anyway. Just move it to its parent's overflow list. @@ +4636,5 @@ > + childNIF->AddStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER); > + if (parent == this) { > + overflowIncompleteList.AppendFrame(nullptr, childNIF); > + } else { > + if (nif && parent != nif) { Please invert the polarity of this "if" check, and move the "else" clause's contents to come first. That way, it's consistent with the logic that you're adding higher up in this patch, where we're working with incompleteList. (Up there, you handle the "parent == GetNextInFlow()" scenario first, and the alternative afterwards.) Or, if you prefer, leave this part as it is, and instead invert the polarity of the logic higher up. Either way -- as long as the logic ends up with a consistent ordering. @@ +4643,5 @@ > + nif->MergeSortedExcessOverflowContainers(toMove); > + nif = nullptr; // just move the first one > + } else { > + nsFrameList toMove(childNIF, childNIF); > + parent->MergeSortedExcessOverflowContainers(toMove); Should we set nif = nullptr here, too? (Or rather: should we bump the existing "nif = nullptr;" line down ~5 lines, just after this "else" clause)? Put another way: as long as one of these overflow containers lives on our grid-NIF (even if it lived there already and we're just shifting it between lists on the same parent-frame), then I don't think we need to bother reparenting any overflow containers on subsequent grid-NIFs... So if we find one on our NIF, we can clear |nif| to prevent ourselves from ever bothering with the ReparentFrame stuff on subsequent continuations. I think. @@ +4649,2 @@ > } > + lastParent = parent; You're adding this DebugOnly<> "lastParent" variable and keeping it up to date, and I was assuming we'd use it in an assertion, but we don't seem to use it at all. So, seems like it should probably be removed, unless you want to keep it around for use when debugging this code with gdb. (If that's the case, consider adding "// for gdb" after its declaration, so it's clearer what it's for & why it's unused.) @@ +4819,5 @@ > + for (nsIFrame* f = overflowContainers->FirstChild(); f; ) { > + nsIFrame* next = f->GetNextSibling(); > + nsIFrame* pif = f->GetPrevInFlow(); > + if (pif && pif->GetParent() == this) { > + overflowContainers->RemoveFrame(f); I think this "overflowContainers->RemoveFrame" might really want to be "StealFrame(f)"... At least: - StealFrame is documented as applying to the overflow lists as well as the principal list. - If this removal leaves us with an empty overflowContainers list, StealFrame will clean that up for us (via TryRemoveFrame), whereas this chunk looks like it'll just leave us with that empty list in our property table.

Comment on attachment 8724480 [details] [diff] [review] part 19 - [css-grid] Sanity check the initial child lists we get from the frame constructor (DEBUG only). Review of attachment 8724480 [details] [diff] [review]: ----------------------------------------------------------------- r=me, minor nits: ::: layout/generic/nsGridContainerFrame.cpp @@ +5226,5 @@ > // nsCSSFrameConstructor::FrameConstructionItem::NeedsAnonFlexOrGridItem() > return aFrame->IsFrameOfType(nsIFrame::eLineParticipant); > } > > // Debugging method, to let us assert that our anonymous grid items are This line needs a minor tweak to stay correct. "Debugging method" --> "Debug-only override", or something like that. (In general, SetInitialChildList is not a "debugging method", though this particular override does happen to be debug-only & focused on assertions.) ::: layout/generic/nsGridContainerFrame.h @@ +72,5 @@ > +#ifdef DEBUG > + void SetInitialChildList(ChildListID aListID, > + nsFrameList& aChildList) override; > +#endif > + whitespace on blank line

Comment on attachment 8724481 [details] [diff] [review] part 20 - [css-grid] Sanity check our child lists before starting a Reflow (DEBUG only). Review of attachment 8724481 [details] [diff] [review]: ----------------------------------------------------------------- r=me, nits below: ::: layout/generic/nsGridContainerFrame.cpp @@ +5268,5 @@ > + ChildListIDs absLists = kAbsoluteList | kFixedList | > + kOverflowContainersList | kExcessOverflowContainersList; > + ChildListIDs itemLists = kPrincipalList | kOverflowList; > + for (const nsIFrame* f = this; f; f = f->GetNextInFlow()) { > + MOZ_ASSERT(!f->HasAnyStateBits(NS_STATE_GRID_DID_PUSH_ITEMS)); Please add an explanatory comment for this assertion; it's non-obvious why it should hold. (Looking at the caller, I think I see why it's safe to assert this, because we happen to handle this bit just before we call this function.) So -- the message might be something like: "At start of reflow, we should've pulled items back from all NIFs " "and cleared NS_STATE_GRID_DID_PUSH_ITEMS in the process" @@ +5278,5 @@ > + continue; > + } > + nsFrameList children = childLists.CurrentList(); > + for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) { > + const nsIFrame* child = e.get(); You can replace the above 3 lines with just: for (nsIFrame child : childLists.currentList) {

Comment on attachment 8724482 [details] [diff] [review] part 21 - [css-grid] Deal with dynamically inserted/appended/removed child frames. Review of attachment 8724482 [details] [diff] [review]: ----------------------------------------------------------------- r- on part 21 for now, mainly due to my first comment below. ::: layout/generic/nsGridContainerFrame.cpp @@ +5221,5 @@ > + nsContainerFrame::RemoveFrame(aListID, aOldFrame); > + > +#ifdef DEBUG > + // Need to do this in DEBUG builds to avoid asserting in the next Reflow(). > + if (invalidateDidPushItems) { Hmm, so here, we're making debug-only changes to a flag that influences our behavior in opt as well as debug builds, just to satisfy some assertions. I'm not comfortable with that -- it'll mean that our debug & opt builds will potentially do different things (where this flag is concerned), which is kind of scary from a correctness perspective. I think we need to either: (1) Remove the #ifdef DEBUG guard around this chunk, if it's not too expensive to do so. ...OR: (2) Don't mess with the DID_PUSH_ITEMS bit here after all, & instead just add some additional piece of debug-only state (e.g. a frame property-table entry with a key like "DidPushItemsMayBeInvalid"), which we can set here instead of clearing the flag, & check for that the assertions in question. ...or: (3) Something else that addresses this without making debug builds diverge from opt builds in terms of which codepath they'll take, as far as NS_STATE_GRID_DID_PUSH_ITEMS is concerned. @@ +5223,5 @@ > +#ifdef DEBUG > + // Need to do this in DEBUG builds to avoid asserting in the next Reflow(). > + if (invalidateDidPushItems) { > + auto nif = static_cast<nsGridContainerFrame*>(GetNextInFlow()); > + for (nsGridContainerFrame* f = this; f ; Drop stray space in "f ;" @@ +5231,5 @@ > + if (!overflow || !::ContainsAnyPushedItems(*overflow)) { > + if (!nif || > + (!nif->HasAnyStateBits(NS_STATE_GRID_DID_PUSH_ITEMS) && > + !::ContainsAnyPushedItems(nif->mFrames))) { > + f->RemoveStateBits(NS_STATE_GRID_DID_PUSH_ITEMS); For the first time through this loop, "f" is "this". So for that first loop iteration, the above-quoted line is calling this->RemoveStateBits(NS_STATE_GRID_DID_PUSH_ITEMS). I don't think that makes sense -- we've removed a frame from our *own* principal child list, which by definition could not be a frame that we pushed. So there's no justification for clearing the DID_PUSH flag on |this|. So, this first iteration of the loop seems a bit broken, or at least half-useless. Putting it another way: there's no scenario in which removing a child frame from a particular grid-fragment's principal list would justify clearing the DID_PUSH flag *on that same grid fragment*. (Though it may justify clearing that flag on previous grid-fragments.) I think really we might want to address this by changing this variable: bool invalidateDidPushItems = aListID == kPrincipalList && !aOldFrame->GetPrevInFlow(); ...to something like: // If aOldFrame is a first-in-flow, and no first-in-flows have been pushed // beyond us, then aOldFrame may have been the only thing responsible for // NS_STATE_GRID_DID_PUSH_ITEMS being set on our prev-in-flows. In that // case, this flag on our prev-in-flows may need clearing. bool invalidateDidPushItems = aListID == kPrincipalList && !aOldFrame->GetPrevInFlow() && !HasAnyStateBits(NS_STATE_GRID_DID_PUSH_ITEMS); And then your invalidate for-loop here would want to start with our prev-in-flow, instead of starting with |this|. (That makes more sense to me, because our prev-in-flow's DID_PUSH flag might actually have been referencing the removed frame.) (Apologies if I'm off the mark here; if I am, perhaps a more-correct version of my suggested comment above might help clarify what's going on in this code.) @@ +5264,5 @@ > + > + nsIPresShell* shell = PresContext()->PresShell(); > + for (auto pif = GetPrevInFlow(); pif; pif = pif->GetPrevInFlow()) { > + if (aListID == kPrincipalList) { > + pif->AddStateBits(NS_STATE_GRID_DID_PUSH_ITEMS); Please add a comment here, explaining why we're setting this bit despite nothing being "pushed" here. (as you said in comment 57, we treat newly-appended/inserted items as if they were pushed items, so that they can be pulled back to our prev-in-flows as-needed.) (You might also make sense to mention this usage in the documentation for NS_STATE_GRID_DID_PUSH_ITEMS in nsFrameStateBits.h, too. Right now, that documentation ("True iff some first-in-flow in-flow children were pushed") kinda disagrees with what you're doing here. Though depending on how you document it here, maybe the existing nsFrameStateBits.h description is still accurate enough.)

Comment on attachment 8724483 [details] [diff] [review] part 22 - [css-grid] Check NS_INLINE_IS_BREAK_BEFORE before checking other completion status. Review of attachment 8724483 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: layout/generic/nsGridContainerFrame.cpp @@ +4505,5 @@ > nsReflowStatus childStatus; > ReflowInFlowChild(child, info, aContainerSize, &aFragmentainer, > aState, aContentArea, aDesiredSize, childStatus); > + MOZ_ASSERT(NS_INLINE_IS_BREAK_BEFORE(childStatus) || > + !NS_FRAME_IS_FULLY_COMPLETE(childStatus) || (This is tweaking the same MOZ_ASSERT that I was pointing to in the middle of comment 71 for part 16, with my nit about "any nontrivial assertion". If you don't end up adding an assertion-message at that point in the patch-stack, please do so here or at some later point.)

Comment on attachment 8724484 [details] [diff] [review] part 23 - [css-grid] A grid container fragment that is an overflow container can't be INCOMPLETE, only OVERFLOW_INCOMPLETE and it should always have zero BSize. Review of attachment 8724484 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one nit: ::: layout/generic/nsGridContainerFrame.cpp @@ +5036,5 @@ > + aStatus |= NS_FRAME_REFLOW_NEXTINFLOW; > + } > + bSize = 0; > + LogicalSize desiredSize(wm, computedISize + bp.IStartEnd(wm), > + bSize + bp.BStartEnd(wm)); This variable shadows another |desiredSize| local-variable, declared just outside of this clause. Happily, I don't think we even need this new variable. Pretty sure we can just tweak the existing |desiredSize| variable, like so: bSize = 0; desiredSize.BSize(wm) = bSize + bp.BStartEnd(wm);

Comment on attachment 8724490 [details] [diff] [review] part 24 - [css-grid] Move the child frame merging code at the start of ReflowOverflowContainerChildren into a new method: DrainExcessOverflowContainersList... Review of attachment 8724490 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: layout/generic/nsContainerFrame.h @@ +398,5 @@ > */ > virtual bool DrainSelfOverflowList() override; > > + > + /** Fix whitespace on blank line, just before the "/**" here. (before the DrainExcessOverflowContainersList documentation) Actually, that blank line should probably just be dropped entirely, since there are 2 blank lines in a row for no clear reason.

Comment on attachment 8724491 [details] [diff] [review] part 25 - [css-grid] Enable fragmentation to occur by reporting our actual reflow status. Review of attachment 8724491 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8724494 [details] [diff] [review] part 20b Review of attachment 8724494 [details] [diff] [review]: ----------------------------------------------------------------- r=me on 20b, nits below: ::: layout/generic/nsGridContainerFrame.cpp @@ +5293,5 @@ > + OverflowContainersProperty())); > + auto eoc = static_cast<const nsFrameList*>(Properties().Get( > + ExcessOverflowContainersProperty())); > + auto pifEOC = static_cast<const nsFrameList*>(pif->Properties().Get( > + ExcessOverflowContainersProperty())); Similar to other parts above, let's use GetPropTableFrames for oc/eoc/pifEOC here, since it doesn't require a static_cast. (and include the "const nsFrameList*" type in the decl, instead of using "auto", since there won't be a static_cast that makes the type obvious anymore) @@ +5296,5 @@ > + auto pifEOC = static_cast<const nsFrameList*>(pif->Properties().Get( > + ExcessOverflowContainersProperty())); > + nsFrameList children = pif->GetChildList(kPrincipalList); > + for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) { > + const nsIFrame* childNIF = e.get()->GetNextInFlow(); Consider using a range-based for loop for a bit more brevity here -- it lets you drop one line, if you're ok not having a dedicated |children| local variable: for (const nsIFrame* child : pif->GetChildList(kPrincipalList)) { const nsIFrame* childNIF = child->GetNextInFlow();

(In reply to Daniel Holbert [:dholbert] (mostly AFK March 3rd - 6th) from comment #66) > part 12 - [css-grid] Collect and merge child frames we need for reflow. > I'd marginally prefer that the "!" be folded into the equality check here, Fixed. > Consider inverting the logic in the last subcondition -- i.e. instead of > checking... > !::IsPrevContinuationOf(dest, src) > ...instead, check: > ::IsPrevContinuationOf(src, dest) Fair enough, fixed. > Typo: s/visavi/vis-a-vis/ > > https://en.wikipedia.org/wiki/Vis-%C3%A0-vis Visavi is a word in Swedish (with the same origin and meaning). https://sv.wiktionary.org/wiki/visavi I think you should follow our good example and make it so in English too. :-p > s/all...lists are sorted/each...list is sorted/ Fixed. > > + nif->StealFrame(nifChild); > > StealFrame is fallible, according to its return type & its documentation in > nsContainerFrame.h: I intend to make it infallible when I can find some time. It already asserts that it succeeded: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=7a5912b5ab4e#1439 and that the frame is on the proper child list: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=7a5912b5ab4e#1396

(In reply to Daniel Holbert [:dholbert] (mostly AFK March 3rd - 6th) from comment #67) > part 13 - [css-grid] Refactor ReflowChildren() by separating out the code > The childWM decl seems to be moving a bit earlier here, for no clear reason. Right, I only need it up here in the next patch, but it doesn't seem worth the effort to refactor this line now, so I'll just leave it as is. > Previously (in the code that's getting removed here), we gave each child its > own |childStatus| for its reflow. Now we're reusing our own reflow status > and passing it to each child, to do with as it likes. Is that bad? > Shouldn't we be using a clean status for each child, and then merge / react > to them as-needed? I think you're referring to the calls in the non-fragmentainer loop: ReflowInFlowChild(*aState.mIter, info, containerSize, nullptr, aState, aContentArea, aDesiredSize, aStatus); Yeah, I'm kind of expecting aStatus to always be COMPLETE here. Perhaps it's better to just discard the child status in this case though since we have no support for anything else. I'll look in to it... (the ReflowInFragmentainer path do use separate |childStatus|)

> I'm kind of expecting aStatus to always be COMPLETE here. I added this to make that clear: MOZ_ASSERT(NS_FRAME_IS_COMPLETE(aStatus));

(In reply to Mats Palmgren (:mats) from comment #85) > > I'm kind of expecting aStatus to always be COMPLETE here. > I added this to make that clear: > MOZ_ASSERT(NS_FRAME_IS_COMPLETE(aStatus)); (Thanks! That's fine, though ideally I'd still prefer a dedicated local variable. I'm worried that unexpected fragmentation in a child reflow method (due to brokenness, new features, or something else) might propagate up to cause bizarre behavior via the grid's own nsReflowStatus being mysteriously being set to something INCOMPLETE, without the grid code actually intentionally doing this. Seems like that could totally happen right now, if some child-frame reflow method misbehaved. But the assertion at least makes that expectation clear, & it'll mean that we can catch any unexpected scenarios with fuzzing.) Please do include an assertion-message with that assertion, in any case -- thanks!

(In reply to Daniel Holbert [:dholbert] (mostly AFK March 3rd - 6th) from comment #68) > part 14 - [css-grid] Make ReflowInFlowChild() deal with a constrained Fixed the nits as suggested. > This chunk of new code (from the "reflowCB" declaration, down to the > NS_UNCONSTRAINEDSIZE assignment just after this quote) is all pretty > confusing (and maybe wrong) for several reasons: OK, I cleaned this up as suggested.

(In reply to Daniel Holbert [:dholbert] (mostly AFK March 3rd - 6th) from comment #69) > part 15 - [css-grid] Compute our pre-reflow logical skip sides and cache the > > + // An inlined version of GetLogicalSkipSides that ignores GetNextInFlow(), > > + // i.e. all fragments assume they're the last and should apply the block- > > + // end border (except overflow containers which never have borders). > > The normal nsSplittableFrame::GetLogicalSkipSides implementation already > does this (ignoring GetNextInFlow), if you pass in > 'aReflowState': > http://mxr.mozilla.org/mozilla-central/source/layout/generic/ > nsSplittableFrame.cpp?rev=b3c868596068&mark=255-270#239 Well, that particular chunk in GetLogicalSkipSides is just a hack, IMHO. I'd like to remove that at some point and make all Reflow code NOT use that function at all. I think it's simpler and more performant(*) to do it like I have done in this bug: assume that the frame will be COMPLETE, and then adjust the bit if the reflow status is INCOMPLETE. * GetLogicalSkipSides has a built-in call to GetConsumedBSize, which is O(n). And the Reflow paths know which type of frame they are so using a virtual method is wasteful. After we have fixed the Reflow paths, we can remove 'aReflowState' because the non-Reflow paths never has one. At that point, we should also consider changing the implementation to use a few frame bits to store the skip sides (there are only a few distinct states these bits can have so 2-3 bits should be enough). So I moved the code to a new nsSplittableFrame method in support of this long term goal. > > + StyleBorder()->mBoxDecorationBreak != > > s/StyleBorder()/aReflowState.mStyleBorder/ After moving this code it's not clear that adding a 'aReflowState' to that method just to be able to use mStyleBorder (once) would be worth it. > > + bp.BEnd(wm) = nscoord(0); > > Just use "= 0" -- drop the nscoord() wrapper. This is an assignment rather than initialization so I think I prefer to keep it. I think it's reasonable to stop supporting assignment from integer types to nscoord at some point. To prevent bugs like "bp.BEnd(wm) = aFlags" and whatnot.

Comment on attachment 8728608 [details] [diff] [review] part 15 - [css-grid] Compute our pre-reflow logical skip sides and cache the result of ComputedLogicalBorderPadding() with that applied. Review of attachment 8728608 [details] [diff] [review]: ----------------------------------------------------------------- (I only skimmed the stuff around the pre-reflow skipsides, so far, since that's the update for the thing I wasn't sure about.) One question, and a few nits: ::: layout/generic/nsGridContainerFrame.cpp @@ +4514,5 @@ > ReflowChildren(gridReflowState, contentArea, aDesiredSize, aStatus); > > + // Skip our block-end border if we're INCOMPLETE. > + if (!NS_FRAME_IS_COMPLETE(aStatus) && > + !gridReflowState.mSkipSides.BEnd() && Does this mean we might create a continuation that *only* includes our border & padding? And if so: supposing we have an element with "padding-bottom: 30in" -- does this mean we'll just death-spiral into generating continuations, since on every single page we'll discover that our continuation's (zero-sized) content-box bottom-borderpadding will make us INCOMPLETE? ::: layout/generic/nsSplittableFrame.h @@ +100,5 @@ > + /** > + * A faster version of GetLogicalSkipSides() that is intended to be used > + * inside Reflow before it's known if |this| frame will be COMPLETE or not. > + * It returns a result that assumes this fragment is the last and thus > + * should apply the block-end border/padding etc (except for "true" overflow Consider dropping "etc" here. (I'm not sure what "etc" refers to.) @@ +101,5 @@ > + * A faster version of GetLogicalSkipSides() that is intended to be used > + * inside Reflow before it's known if |this| frame will be COMPLETE or not. > + * It returns a result that assumes this fragment is the last and thus > + * should apply the block-end border/padding etc (except for "true" overflow > + * containers which always skip block sides). You're then expected to s/block sides/block-axis sides/ ("block sides" sounds like it could just mean all 4 sides, because a block has 4 sides. "block-axis" makes it clearer.) @@ +103,5 @@ > + * It returns a result that assumes this fragment is the last and thus > + * should apply the block-end border/padding etc (except for "true" overflow > + * containers which always skip block sides). You're then expected to > + * recalculate the block-end side (as needed) when you know |this| frame's > + * reflow status is INCOMPLETE. Consider: s/when you know/if you discover/ (This makes it clearer that it really could go either way, COMPLETE vs INCOMPLETE.) @@ +104,5 @@ > + * should apply the block-end border/padding etc (except for "true" overflow > + * containers which always skip block sides). You're then expected to > + * recalculate the block-end side (as needed) when you know |this| frame's > + * reflow status is INCOMPLETE. > + * This method is intended for frames that breaks in the block axis. Typo: s/breaks/break/ (should be "frames that break")

(In reply to Daniel Holbert [:dholbert] from comment #89) > Comment on attachment 8728608 [details] [diff] [review] > part 15 - [css-grid] Compute our pre-reflow logical skip sides and cache the > > + // Skip our block-end border if we're INCOMPLETE. > > + if (!NS_FRAME_IS_COMPLETE(aStatus) && > > + !gridReflowState.mSkipSides.BEnd() && > > Does this mean we might create a continuation that *only* includes our > border & padding? Yes, but it can only happen once - when there is a class C break at the end. > And if so: supposing we have an element with "padding-bottom: 30in" -- does > this mean we'll just death-spiral into generating continuations, since on > every single page we'll discover that our continuation's (zero-sized) > content-box bottom-borderpadding will make us INCOMPLETE? The code carefully avoids doing that. A zero-sized content box implies no class C break (at either end).

Comment on attachment 8728608 [details] [diff] [review] part 15 - [css-grid] Compute our pre-reflow logical skip sides and cache the result of ComputedLogicalBorderPadding() with that applied. Ah, right -- that happens via mCanBreakAtEnd, it looks like. (That seems to control whether we end up setting an INCOMPLETE status at the end.) r=me then - thanks!

(In reply to Daniel Holbert [:dholbert] from comment #70) > part 16 - [css-grid] Implement fragmentation. > > + auto maxSize = aReflowState->ComputedMaxBSize(); > > + if (MOZ_UNLIKELY(maxSize != NS_UNCONSTRAINEDSIZE)) { > > + maxSize = std::max(aReflowState->ComputedMinBSize(), maxSize); > > This clamping (checking e.g. max-height against min-height) is unnecessary > -- nsHTMLReflowState should already enforce this internally Ah, good point. I removed that and added an assert. > > + if (disp->mBreakBefore) { > > + // Propagate break-before on the first row to the container unless we're > > + // already at top-of-page. > > + if ((itemStartRow == 0 && !isTopOfPage) || avoidBreakInside) { > > Do we actually have to check isTopOfPage here? I think "avoidBreakInside" > already implicitly checks that on aState.mReflowState (in the > ShouldAvoidBreakInside() impl). We need to check it in the first term, yes. You're right that avoidBreakInside implies it but "itemStartRow == 0" isn't the same as "(itemStartRow == 0 && !isTopOfPage)" no matter what the second term says. (tests fail when it's removed) > (Put another way: is aFragmentainer.mIsTopOfPage guaranteed to match > aState.mReflowState->mIsTopOfPage here? Yes. > [I think the answer is "yes"]. If > so, I don't think we need to bother with the local |isTopOfPage| variable in > this function.) It's shorter which avoid line breaks, and has better performance since it can be put in a register for the duration of this loop. I made it const though, since it doesn't change in this method. > > + // reset any BREAK_AFTER we found on an earlier item > > + aStatus = NS_FRAME_COMPLETE; > > NS_FRAME_COMPLETE seems like a very odd status to be setting, when we > actually know we're going to force a break. :) Well, it's just resetting the status to what it was on entry to this method (which is always COMPLETE). > nsFrameStatus tentativeStatus = aStatus; // used if we find a > "break-after" item > for (const GridItemInfo* info : sortedItems) { > uint32_t itemStartRow = info->mArea.mRows.mStart; > if (itemStartRow == endRow) { > // Make any "break-after" that we hit on this row take effect: > aStatus = tentativeStatus; > break; > } > [...] > if (disp->mBreakBefore) { > // (no need to set aStatus to a confusingly contradictory > // NS_FRAME_COMPLETE value here anymore. We can just leave > // the tentative value in tentativeStatus and forget about it.) > [...] No, we still need to reset tentativeStatus here, otherwise you'd still return it at the "break" above. So I think your suggestions just introduce needless complexity. I added a MOZ_ASSERT(aStatus == NS_FRAME_COMPLETE) at the top though to make it clear we're not clobbering any value from the caller. > > + } else if (bSize <= bEndRow && startRow + 1 < endRow) { > > + if (endRow == numRows) { > > + // We have more than one row in this fragment, so we can break before > > + // the last row instead. > > + --endRow; > > I'm not clear how we get into this situation. Further up in this function, > this patch purports to determine |endRow| so that we won't have to do this, > I think. (per the comment "Set |endRow| to the first row that doesn't fit.") > > So if that higher-up code determines endRow correctly, how would we get into > a situation where we need to decrement endRow in order for stuff to fit here? This is an edge case when all rows fit (endRow == numRows) and there's no break-opportunity after it (it's an else-clause of "breakAfterLastRow") but we overflow due to the border not fitting. We don't want to push *just* the border in this case because there is no break-opportunity before it. Instead, we break before the next-to-last row, if we have it in this fragment (there's always a break-opportunity between each row). Note that the higher-up code only determines the |endRow| based on intrinsic row sizes alone. The code here adjusts that in a few edge cases after |bSize| has been set to either a definite size, or an auto-size with min/max applied. > > + // If we can't fit all rows then we're at least overflow-incomplete. > > + if (endRow < numRows) { > > + childAvailableSize = bEndRow; > > Do we need this "childAvailableSize = bEndRow;" assignment anymore? We just > ensured that childAvailableSize was at least bEndRow in the code directly > above this. It's not clear to me why we'd need a slightly-different version > of the same assignment here. Yes, sorry, that code was a bit confusing. I've moved the first assignment into an else-clause of the above instead. (In reply to Daniel Holbert [:dholbert] from comment #71) > > + bool isTopOfPage = aStartRow != 0 || !aFragmentainer.mCanBreakAtStart; > > This line is confusing to me. > (1) I don't see how either of these conditions (nonzero row or not being > able to break at the start) indicate that we're at the top of the apge. If "aStartRow != 0" we're in a continuation (which always starts at the top), otherwise, for the first-in-flow we're at the top of the page if there are no break opportunities before the first row. Well, at least as far as this method is concerned - the isTopOfPage is only for the children at this point - to let them know if they can cause a row break or not (by returning NS_INLINE_IS_BREAK_BEFORE). isTopOfPage is then set to false as soon as we're done with the first row in this fragment (aStartRow). aFragmentainer.mIsTopOfPage is used to propagate this value to the child reflow state's mIsTopOfPage, to control its AvoidBreakBefore behavior etc. We also set aFragmentainer.mIsTopOfPage false in the case were we can grow a row, because we want to retry reflowing the child in a larger row size in that case. There's a fairly long code comment explaining this. Let me know if anything is still unclear. > In addition to this check, can you assert that row >= aStartRow somewhere > here? No, we can have continuations for items with row < aStartRow. But sure, I added this: MOZ_ASSERT(child->GetPrevInFlow() ? row < aStartRow : row >= aStartRow, "unexpected child start row"); > RE "per CSS Grid §13.1" here -- don't understand that reference. It sounds > like you're saying "we set it to false... per CSS Grid §13.1", but that > section of the spec doesn't mention breaks or pages at all I meant that the term "growable" is defined per that section, nothing else. Doh! I now see that Fragmentation was bumped to §14. I changed the comment to exclude any chapter nunmber for now: "... growable (as determined in CSS Grid Fragmentation) ...". > Though even so, I > still don't exactly see what spec-text you'd be referencing RE which rows > are growable vs. not growable in that section. Is there a more specific spec > section / term you can reference here? Yeah, I meant the "increase the grid row as necessary for rows that either ... [criterias for a row being growable]". (The spec had a typo there: s/grid row/grid row size/.) > (In the first clause, we're really only making those sorts of fate-changes > if the "aEndRow = row + 1" assignment there actually changes anything. If > aEndRow already happened to be row + 1, then we don't need to tweak aStatus. > But otherwise I'd expect we do need to tweak it.) Hmm... I'll need to investigate this... OK, back after two days. :-) Yeah, this can happen in rare circumstances - good catch! It occurs when the grid would fit completely in the first column, but has a growable row contains an item that returns BREAK_BEFORE, and it's not the last row. Then we'll grow that row to fill the first column and thus "push" some rows, so we should change aStatus to INCOMPLETE, assuming the [max-]height isn't restricting us from doing so. So I fixed that, and wrote a bunch of new tests to cover this. > Please use the safer "GetPropTableFrames" getter here: > nsFrameList* eoc = > GetPropTableFrames(ExcessOverflowContainersProperty()); OK, I'll do that in part 17. I ignored the nits about the StealFrame return value, as explained earlier. I've addressed the other nits.

I also realized now that the calculations for |rowCanGrow| are unnecessary as long as |row < aStartRow|. It works anyway, because continuations should never report BREAK_BEFORE status, but it's an optimization worth doing. I'll do this in a separate patch.

Actually, I had to add quite a bit of logic to implement that so it doesn't seem worth it.

Well, this part should be worth doing anyway. Just wrap the calculations in "if (row >= aStartRow)". (no commit message - I'll fold this into part 16)

This might be easier to review.

(In reply to Daniel Holbert [:dholbert] from comment #74) > part 18 - [css-grid] Fix a couple of bugs in how we handle child existing > > + auto parent = static_cast<nsGridContainerFrame*>(childNIF->GetParent()); > > Now that you've got a local variable for childNIF->GetParent(), you should > probably simplify the assertion on the next line. Fixed. > Could you add a comment to explain the "parent == GetNextInFlow()" Yes, I added the suggested comment. > > + if (nif && parent != nif) { > > Please invert the polarity of this "if" check, and move the "else" clause's > contents to come first. Sure. > Put another way: as long as one of these overflow containers lives on our > grid-NIF (even if it lived there already and we're just shifting it between > lists on the same parent-frame), then I don't think we need to bother > reparenting any overflow containers on subsequent grid-NIFs... So if we find > one on our NIF, we can clear |nif| to prevent ourselves from ever bothering > with the ReparentFrame stuff on subsequent continuations. I think. That's a good point. I made it so. > You're adding this DebugOnly<> "lastParent" variable and keeping it up to > date, and I was assuming we'd use it in an assertion, but we don't seem to > use it at all. Removed. > > + overflowContainers->RemoveFrame(f); > > I think this "overflowContainers->RemoveFrame" might really want to be > "StealFrame(f)"... That would work but it's more expensive. We should try to avoid StealFrame when we're iterating a known nsFrameList. > - If this removal leaves us with an empty overflowContainers list, > StealFrame will clean that up for us (via TryRemoveFrame), whereas this > chunk looks like it'll just leave us with that empty list in our property > table. I added code to delete the property if the list became empty. Ideally, we should have an AutoFrameListPropRef to automate this...

(In reply to Daniel Holbert [:dholbert] from comment #77) > part 21 - [css-grid] Deal with dynamically inserted/appended/removed child > > +#ifdef DEBUG > > + // Need to do this in DEBUG builds to avoid asserting in the next Reflow(). > > + if (invalidateDidPushItems) { > > Hmm, so here, we're making debug-only changes to a flag that influences our > behavior in opt as well as debug builds, just to satisfy some assertions. OK, I removed the code that changed the frame bit, and instead added a DEBUG-only bool on the frame to suppress those assertions the next time we try to pull up pushed items. > > + if (!overflow || !::ContainsAnyPushedItems(*overflow)) { > > + if (!nif || > > + (!nif->HasAnyStateBits(NS_STATE_GRID_DID_PUSH_ITEMS) && > > + !::ContainsAnyPushedItems(nif->mFrames))) { > > + f->RemoveStateBits(NS_STATE_GRID_DID_PUSH_ITEMS); > > For the first time through this loop, "f" is "this". So for that first loop > iteration, the above-quoted line is calling > this->RemoveStateBits(NS_STATE_GRID_DID_PUSH_ITEMS). > > I don't think that makes sense -- we've removed a frame from our *own* > principal child list, which by definition could not be a frame that we > pushed. So there's no justification for clearing the DID_PUSH flag on > |this|. So, this first iteration of the loop seems a bit broken, or at least > half-useless. Just to clarify why that is in fact needed. The frame constructor calls RemoveFrame(kPrincipalList) even though the frames may actually be on either kPrincipalList or kOverflowList. We may have pushed items in our own kOverflowList (in case our NIF haven't reflowed yet before this RemoveFrame call). (BTW, this is not just a theoretical concern - some of the reftests triggers this.) Anyway, the code is removed now, but I've added a code comment to explain why we set the added DEBUG-bit for kPrincipalList. > (You might also make sense to mention this usage in the documentation for > NS_STATE_GRID_DID_PUSH_ITEMS in nsFrameStateBits.h, too. OK, I amended that comment.

Comment on attachment 8728690 [details] [diff] [review] part 16 - [css-grid] Implement fragmentation. Review of attachment 8728690 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mats Palmgren (:mats) from comment #92) > (In reply to Daniel Holbert [:dholbert] from comment #71) > > > + bool isTopOfPage = aStartRow != 0 || !aFragmentainer.mCanBreakAtStart; > > > > This line is confusing to me. > > (1) I don't see how either of these conditions (nonzero row or not being > > able to break at the start) indicate that we're at the top of the apge. > > If "aStartRow != 0" we're in a continuation (which always starts at > the top), otherwise, for the first-in-flow we're at the top of > the page if there are no break opportunities before the first row. Thanks -- that does clarify things. I think we need to make two changes here: (a) This variable probably needs renaming to something like "isCurRowTopOfPage", to indicate that it's specificially talking about the *current row* (and gets updated as we move across rows). This will better-distinguish it from the multiple other "mIsTopOfPage" variables that we have access to here. (b) It would help to have a little explanation alongside this variable-declaration, to clarify what it tracks and why we set it to the initial value that we do. Something like the following: // As we walk across rows, we track whether the current row is at the top // of its grid-fragment, to help decide whether we can break before it. When // this function starts, our row is at the top of the current fragment if: // - we're starting with a nonzero row (i.e. we're a continuation) // OR: // - we're starting with the first row, & we're not allowed to break before // it (which makes it effectively at the top of its grid-fragment). > aFragmentainer.mIsTopOfPage is used to propagate this value to > the child reflow state's mIsTopOfPage, to control its AvoidBreakBefore > behavior etc [...] > There's a fairly long code comment explaining this. Let me know > if anything is still unclear. Thanks -- I think I see the code-comment you're talking about, but that one is entirely focused on explaining how we set aFragmentainer.mIsTopOfPage, rather than on the meaning of this local variable here. So, per above, I think some documentation [and probably a rename] for the local variable would help clarify things. ::: layout/generic/nsGridContainerFrame.cpp @@ +69,5 @@ > +} > + > +// Same as above and set aStatus INCOMPLETE if aSize wasn't clamped. > +static nscoord > +ClampToCSSMaxBSize(nscoord aSize, const nsHTMLReflowState* aReflowState, Can you explain a bit more (here and/or at the callsites) what the point is of this function's aStatus behavior? (Why would not-clamping lead to something being INCOMPLETE, whereas clamping [even just by a little bit] would leave it COMPLETE?) It might help to just explain the circumstances under which this function is expected to be called, and what its aSize argument is supposed to represent (e.g. is it a size that's currently-known to fit without breaking, or something like that?) Then the behavior here might make more sense. (One observation: every caller passes |bEndRow| in for the |aSize| argument.) @@ +4455,5 @@ > + FrameHashtable overflowIncompleteItems; > + bool isBDBClone = aState.mReflowState->mStyleBorder->mBoxDecorationBreak == > + NS_STYLE_BOX_DECORATION_BREAK_CLONE; > + bool didGrowRow = false; > + bool isTopOfPage = aStartRow != 0 || !aFragmentainer.mCanBreakAtStart; (This is the variable that I think needs renaming/documenting, noted at the top of this bug-comment.) @@ +4523,5 @@ > + MOZ_ASSERT(!aFragmentainer.mIsTopOfPage, > + "got NS_INLINE_IS_BREAK_BEFORE at top of page"); > + if (!didGrowRow) { > + if (rowCanGrow) { > + // Grow this this row and restart with the next row as |aEndRow|. Typo, "this this" -> "this"

Comment on attachment 8728933 [details] [diff] [review] part 21 - [css-grid] Deal with dynamically inserted/appended/removed child frames. Review of attachment 8728933 [details] [diff] [review]: ----------------------------------------------------------------- r=me on 21 -- just one nit: ::: layout/generic/nsGridContainerFrame.cpp @@ +5255,5 @@ > + // It can also be on kOverflowList, in which case it might be a pushed > + // item, and if it's the only pushed item our DID_PUSH_ITEMS bit will lie. > + mDidPushItemsBitMayLie = (aListID == kPrincipalList && > + !aOldFrame->GetPrevInFlow()) || > + mDidPushItemsBitMayLie; Let's speed this up by swapping the order of the conditions here. The bool-check is fast, and when it passes, it'll let us short-circuit and skip the GetPrevInFlow() virtual-method call. So: mDidPushItemsBitMayLie = mDidPushItemsBitMayLie || ( ... );

(In reply to Daniel Holbert [:dholbert] from comment #101) > part 16 - [css-grid] Implement fragmentation. > I think we need to make two changes here OK, I renamed it isRowTopOfPage, and savedIsTopOfPage -> isStartRowTopOfPage. I also added the suggested comment. > > +ClampToCSSMaxBSize(nscoord aSize, const nsHTMLReflowState* aReflowState, > > Can you explain a bit more (here and/or at the callsites) what the point is > of this function's aStatus behavior? Sure, the given aSize is the new size for this fragment and thus implicitly the break point. If we clamp that size it means our size is less than the break point, i.e. we're effectively breaking in our overflow, so we should leave aStatus as is (it will likely be set to OVERFLOW_INCOMPLETE later). I added a code comment to that effect.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b7c33f5687 https://hg.mozilla.org/integration/mozilla-inbound/rev/373706299018 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7ff8ae1216 https://hg.mozilla.org/integration/mozilla-inbound/rev/035a1dd254e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa46d335b28 https://hg.mozilla.org/integration/mozilla-inbound/rev/738a70d91d8c https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbadb0b2f44 https://hg.mozilla.org/integration/mozilla-inbound/rev/91e99c2febec https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2efb5e8dfb https://hg.mozilla.org/integration/mozilla-inbound/rev/eba6b9e5a218 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0203659e30 https://hg.mozilla.org/integration/mozilla-inbound/rev/44697a495a24 https://hg.mozilla.org/integration/mozilla-inbound/rev/33b3bfd3d416 https://hg.mozilla.org/integration/mozilla-inbound/rev/77d251bd9afe https://hg.mozilla.org/integration/mozilla-inbound/rev/c5fd95723f2e https://hg.mozilla.org/integration/mozilla-inbound/rev/40456ea73860 https://hg.mozilla.org/integration/mozilla-inbound/rev/b52d71c48531 https://hg.mozilla.org/integration/mozilla-inbound/rev/47452506ec56 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7ef2dd4427 https://hg.mozilla.org/integration/mozilla-inbound/rev/557bb3ae736f https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b769cce0f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/04b6926cb2f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9834eee771 https://hg.mozilla.org/integration/mozilla-inbound/rev/fbed3943a9e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3e2675488c https://hg.mozilla.org/integration/mozilla-inbound/rev/552aabbcd116

https://hg.mozilla.org/mozilla-central/rev/a8b7c33f5687 https://hg.mozilla.org/mozilla-central/rev/373706299018 https://hg.mozilla.org/mozilla-central/rev/7e7ff8ae1216 https://hg.mozilla.org/mozilla-central/rev/035a1dd254e1 https://hg.mozilla.org/mozilla-central/rev/6fa46d335b28 https://hg.mozilla.org/mozilla-central/rev/738a70d91d8c https://hg.mozilla.org/mozilla-central/rev/1fbadb0b2f44 https://hg.mozilla.org/mozilla-central/rev/91e99c2febec https://hg.mozilla.org/mozilla-central/rev/0b2efb5e8dfb https://hg.mozilla.org/mozilla-central/rev/eba6b9e5a218 https://hg.mozilla.org/mozilla-central/rev/cd0203659e30 https://hg.mozilla.org/mozilla-central/rev/44697a495a24 https://hg.mozilla.org/mozilla-central/rev/33b3bfd3d416 https://hg.mozilla.org/mozilla-central/rev/77d251bd9afe https://hg.mozilla.org/mozilla-central/rev/c5fd95723f2e https://hg.mozilla.org/mozilla-central/rev/40456ea73860 https://hg.mozilla.org/mozilla-central/rev/b52d71c48531 https://hg.mozilla.org/mozilla-central/rev/47452506ec56 https://hg.mozilla.org/mozilla-central/rev/2a7ef2dd4427 https://hg.mozilla.org/mozilla-central/rev/557bb3ae736f https://hg.mozilla.org/mozilla-central/rev/e8b769cce0f1 https://hg.mozilla.org/mozilla-central/rev/04b6926cb2f3 https://hg.mozilla.org/mozilla-central/rev/5b9834eee771 https://hg.mozilla.org/mozilla-central/rev/fbed3943a9e5 https://hg.mozilla.org/mozilla-central/rev/5e3e2675488c https://hg.mozilla.org/mozilla-central/rev/552aabbcd116