(reftests will come later, in bug 1151212) https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3e18b6960f7

(Sorry for delays here; I've been taking advantage of the fact that you're on vacation to catch up on other reviews/tasks. I'm intending to have your various patches reviewed by the time you're back. Moving them to my front-burner now.)

Comment on attachment 8625320 [details] [diff] [review] Implement the "Maximize Tracks" algorithm Review of attachment 8625320 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following addressed as you see fit: ::: layout/generic/nsGridContainerFrame.cpp @@ +473,5 @@ > + * http://dev.w3.org/csswg/css-grid/#algo-grow-tracks > + */ > + void DistributeFreeSpace(nscoord aAvailableSize) > + { > + const uint32_t len = mSizes.Length(); Maybe s/len/numTracks/? The current "length"-flavored name is slightly ambiguous, in a function that's about adjusting physical "lengths". ("numTracks" feels a bit clearer than "len" when used in comparison/subtraction with "numFrozen", further down in this function.) @@ +487,5 @@ > + uint32_t numFrozen = 0; > + for (const TrackSize& sz : mSizes) { > + space -= sz.mBase; > + if (sz.mBase == sz.mLimit) { > + ++numFrozen; Please include a brief descriptive comment before each of these main loops here, to make this function a bit easier to skim & quickly grok. e.g. before this loop: // Compute free space (& count already-frozen tracks) ...and before the next loop: // Distribute |space| to the un-frozen tracks. @@ +497,5 @@ > + TrackSize& sz = mSizes[i]; > + if (sz.mBase == sz.mLimit) { > + continue; > + } > + nscoord base = sz.mBase + spacePerTrack; Maybe s/base/newBase/ here. ::: layout/reftests/css-grid/grid-whitespace-handling-2-ref.xhtml @@ +13,5 @@ > <title>CSS Reftest Reference</title> > <link rel="author" title="Daniel Holbert" href="mailto:dholbert@mozilla.com"/> > <link rel="stylesheet" type="text/css" href="support/ahem.css" /> > <style> > + div.a, div.b, div.grid { height: 100px; } (Did you mean to make this reference-case tweak as part of this patch? I don't immediately see why this would be needed. If it is indeed needed, it seems fine - just calling it out in case it was accidental.)

(In reply to Daniel Holbert [:dholbert] (out 8/5-8/10) from comment #3) > Please include a brief descriptive comment before each of these main loops > here, to make this function a bit easier to skim & quickly grok. I did so, but I also decided to reverse the logic: from numFrozen to numGrowable, to avoid doing "len - numFrozen" inside the loop. (asking for re-review for this change) > (Did you mean to make this reference-case tweak as part of this patch? Yes, that test renders differently (correctly now). I fixed the other nits as suggested.

Comment on attachment 8647467 [details] [diff] [review] Implement the "Maximize Tracks" algorithm Looks good. 2 optional suggestions, to address (or not) as you see fit: >+++ b/layout/generic/nsGridContainerFrame.cpp >+ // Compute free space and count growable tracks. >+ nscoord space = aAvailableSize; >+ uint32_t numGrowable = numTracks; >+ for (const TrackSize& sz : mSizes) { >+ space -= sz.mBase; >+ if (sz.mBase == sz.mLimit) { >+ --numGrowable; >+ } Perhaps worth asserting here (before the "==" check) that sz.mBase <= sz.mLimit? [I assume that must be the case, or else this "==" check would have to be ">=" to handle any bases that have somehow gone over the limit by a small amount.] I trust that there's code elsewhere that enforces this -- but perhaps good to assert about it here, because we're separated from that code, and the logic here depends on this assertion holding. >+ nscoord newBase = sz.mBase + spacePerTrack; >+ if (newBase >= sz.mLimit) { >+ space -= sz.mLimit - sz.mBase; >+ sz.mBase = sz.mLimit; >+ --numGrowable; >+ } else { >+ sz.mBase = newBase; >+ space -= spacePerTrack; >+ } Nit: maybe swap the order of the 2 lines inside the "else" clause, to make that code more consistent with the code in the "if" clause. (The idea being: first we chip away from |space|, and then we lock in the final sz.mBase -- in *both* clauses.)

(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #5) > Looks good. 2 optional suggestions, to address (or not) as you see fit: OK, fixed as suggested.

Backed out for reftest bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=13722584&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/11e2a307334c