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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

http://dev.w3.org/csswg/css-grid/#algo-grow-tracks

Comment 1

[Mats Palmgren (inactive)]

Attachment: Implement the "Maximize Tracks" algorithm

(reftests will come later, in bug 1151212)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3e18b6960f7

Comment 2

[Daniel Holbert [:dholbert]]

(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 3

[Daniel Holbert [:dholbert]]

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:[e-mail]"/>
>      <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.)

Comment 4

[Mats Palmgren (inactive)]

Attachment: Implement the "Maximize Tracks" algorithm

(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 5

[Daniel Holbert [: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.)

Comment 6

[Mats Palmgren (inactive)]

(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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19b5e154c6f9

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed39340ecfc9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed39340ecfc9