Closed Bug 1176619 Opened 7 years ago Closed 6 years ago

[css-grid] Implement the "Maximize Tracks" algorithm

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

Attachments

(1 file, 1 obsolete file)

Blocks: 1176621
(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.)
Attachment #8625320 - Flags: review?(dholbert) → review+
(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.
Attachment #8625320 - Attachment is obsolete: true
Attachment #8647467 - Flags: review?(dholbert)
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.)
Attachment #8647467 - Flags: review?(dholbert) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/ed39340ecfc9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.