1151212 - [css-grid] Implement the Track Sizing Algorithm

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

http://dev.w3.org/csswg/css-grid/#algo-track-sizing

Bug 1139539 has already implemented "1. Initialize Track Sizes".

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - Introduce a few local structs (GridReflowState, Tracks, TrackSizingFunctions) to make it easier to pass around data

Nothing new here, just refactoring stuff.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - Introduce a local GridItemInfo struct for holding a grid item's GridArea and other data...

* add a new GridItemInfo struct
* put those in nsTArrays on the container - one for normal flow, one for abspos
* the GridItemIndex() method on the iterator gives you the index to use
and refactor the code accordingly...

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Implement the "Resolve Intrinsic Track Sizes" algorithm

There are likely some optimizations still to do here, but I figured
it might be easier to review this for correctness in its current state.

The code in CalculateTrackSizes is a bit repetitive - I've fixed that
in a later patch (in bug 1174574) by moving it into a 'Tracks' method.

The code for "step 2.1 - 2.3" is mostly the same - I'll probably move
those blocks into a shared method later as well.

This patch also removes the "#if 0" around the MinSize,
Min/MaxContentContribution functions since we start using them now.

I spawned off bug 1176619 and bug 1176621 separately.

This patch implements "12.5. Resolve Intrinsic Track Sizes":
http://dev.w3.org/csswg/css-grid/#algo-content

(a separate patch with reftests will come later)

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

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8622128 [details] [diff] [review]
part 1 - Introduce a few local structs (GridReflowState, Tracks, TrackSizingFunctions) to make it easier to pass around data

Nit on part 1 (for something that's used in part 3):

>diff --git a/layout/generic/nsGridContainerFrame.cpp b/layout/generic/nsGridContainerFrame.cpp
>+struct nsGridContainerFrame::GridReflowState
>+{
[...]
>+  const nsHTMLReflowState* mReflowState;
>+  nsRenderingContext& mRenderingContext;
>@@ -1304,18 +1392,17 @@ nsGridContainerFrame::ReflowChildren(Gri
[...]
>+  nsRenderingContext rc(PresContext()->PresShell()->
>+                          CreateReferenceRenderingContext());
>+  GridReflowState gridReflowState(this, rc, aReflowState);

I don't think we actually need "mRenderingContext" here, or the call to CreateReferenceRenderingContext() which provides its value.

As I discovered in bug 1165667:
 (1) CreateReferenceRenderingContext can be expensive.
 (2) nsHTMLReflowState brings along its own reference rendering context, so we should be able to just use that.

Comment 6

[Daniel Holbert [:dholbert]]

Er, maybe we do need GridReflowState::mRenderingContext, if we expect to have GridReflowState instances that have a null nsHTMLReflowState pointer. (and I suspect we do expect that, given that you've got a GridReflowState constructor for this)

But for the other constructor which does take a nsHTMLReflowState, we should copy its mRenderingContext pointer, instead of requiring a brand-new one.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8622128 [details] [diff] [review]
part 1 - Introduce a few local structs (GridReflowState, Tracks, TrackSizingFunctions) to make it easier to pass around data

>+struct nsGridContainerFrame::TrackSizingFunctions
[...]
>+struct nsGridContainerFrame::Tracks
[...]
>+struct nsGridContainerFrame::GridReflowState

Please annotate all three new structs as MOZ_STACK_CLASS.  (They all have weak references/pointers to things whose lifetimes they don't control, so if any of them happened to be heap-allocated & outlived their stack frame, bad things could happen.)


>+  GridReflowState(nsGridContainerFrame*    aFrame,
>+                  nsRenderingContext&      aRenderingContext,
>+                  const nsHTMLReflowState& aReflowState)
>+    : GridReflowState(aFrame, aRenderingContext, &aReflowState,
>+                      aReflowState.GetWritingMode())
>+  {}
>+  GridReflowState(nsGridContainerFrame* aFrame,
>+                  nsRenderingContext&   aRenderingContext)
>+    : GridReflowState(aFrame, aRenderingContext, nullptr,
>+                      aFrame->GetWritingMode())
>+  {}
[...]
>+private:
>+  GridReflowState(nsGridContainerFrame*    aFrame,
>+                  nsRenderingContext&      aRenderingContext,
>+                  const nsHTMLReflowState* aReflowState,
>+                  const WritingMode&       aWM)
>+    : mIter(aFrame, kPrincipalList)
>+    , mGridStyle(aFrame->StylePosition())

If we have a non-null nsHTMLReflowState, we should just copy its mStylePosition here, instead of invoking the frame method. (Similar to how you already handle mWM -- you initialize it from either the reflowState or the frame, in the public constructors)

To do this, we should probably match the pattern you're already using for mWM here (and that you'll probably be using for the nsRenderingContext as well, per my previous comments) -- i.e. we should add a new 'nsStylePosition*' parameter to the private constructor, and make the public constructors pass in the value from aReflowState or from aFrame.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 1 - Introduce a few local structs (GridReflowState, Tracks, TrackSizingFunctions) to make it easier to pass around data

(In reply to Daniel Holbert [:dholbert] from comment #5)
> As I discovered in bug 1165667:
>  (1) CreateReferenceRenderingContext can be expensive.

Yes, I'm aware, thanks.

>  (2) nsHTMLReflowState brings along its own reference rendering context, so
> we should be able to just use that.

You're right.  I somehow jumped to the conclusion that that member is
a "const nsRenderingContext* const" b/c the arg to Reflow() is "const".
My mistake.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Please annotate all three new structs as MOZ_STACK_CLASS.

Fixed.

> If we have a non-null nsHTMLReflowState, we should just copy its
> mStylePosition here, instead of invoking the frame method. 

Good point. Fixed.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8626535 [details] [diff] [review]
part 1 - Introduce a few local structs (GridReflowState, Tracks, TrackSizingFunctions) to make it easier to pass around data

Review of attachment 8626535 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here - reviews on huge table-verticalization patches & whistler-events/travel have been occupying all of my attention recently.  I'm hoping to get through all your reviews ASAP now, though. :)

r=me on part 1 with comments below addressed as you see fit.

::: layout/generic/nsGridContainerFrame.cpp
@@ +975,3 @@
>  {
> +  const nsStylePosition* const gridStyle = aState.mGridStyle;
> +  GridItemCSSOrderIterator& iter = aState.mIter;

In PlaceGridItems & ReflowChildren, I'd prefer we directly use 'aState.mIter' rather than abstracting it away with a local alias.  The local alias makes it harder to tell that our loops here are actually modifying part of |aState| (and that's pretty important, since we reuse this piece of aState over and over, and need to be aware of what's changing it).

If we were saving a ton of characters, it might be worth it, but I don't think we are; "aState.mIter" is pretty short, and we only use it once per loop (to get 'nsIFrame* child').

Not a big deal, though; if you strongly prefer the aliasing, I'm OK with this as-is, too.

@@ +1200,2 @@
>  {
> +  const size_t explicitGridOffset = aFunctions.mExplicitGridOffset;

Nit: there's a type mismatch here. (aFunctions.mExplicitGridOffset has type uint32_t, and we're copying its value into a size_t.)

Perhaps we should make mExplicitGridOffset a size_t as well? (Or, we could convert explicitGridOffset and i/j further down to uint32_t -- though i/j are compared against nsTArray::Length() which is size_t.)

I guess this is unlikely to cause a problem, since size_t is >= 32 bits on all supported platforms. It's just a bit confusing/arbitrary that we use 2 different types here, and it'd be nice to be consistent. Not sure any action is needed for now though.

@@ +1227,5 @@
>  }
>  
>  void
> +nsGridContainerFrame::CalculateTrackSizes(GridReflowState&   aState,
> +                                          const LogicalSize& aContentSize)

"aContentSize" is ambiguous (unclear from the name whether it's the size of our contents, vs. our content-box size).

I'd suggest renaming to aContentBoxSize, or even aComputedSize (since it's exactly what our reflowstate would return from the ComputedSize() method).

@@ +1292,5 @@
>    }
>  }
>  
>  LogicalRect
> +nsGridContainerFrame::ContainingBlockFor(GridReflowState& aState,

Looks like aState can be 'const' here. (This is a read-only method; doesn't modify the state.)

@@ +1305,4 @@
>  }
>  
>  LogicalRect
> +nsGridContainerFrame::ContainingBlockForAbsPos(GridReflowState&    aState,

Same her -- I think aState can be const.

@@ +1327,5 @@
>  void
> +nsGridContainerFrame::ReflowChildren(GridReflowState&         aState,
> +                                     const LogicalRect&       aContentArea,
> +                                     nsHTMLReflowMetrics&     aDesiredSize,
> +                                     const nsHTMLReflowState& aReflowState,

Note: the 'nsHTMLReflowState& aReflowState' parameter to this method (ReflowChildren) is now somewhat redundant, since the new GridReflowState parameter brings along a pointer to the same object.

So, it might be best to drop the aReflowState param, & replace its usages here with *aState.mReflowState. (We can also assert that aState.mReflowState is non-null at the beginning of ReflowChildren.)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8622129 [details] [diff] [review]
part 2 - Introduce a local GridItemInfo struct for holding a grid item's GridArea and other data...

r=me with the following addressed:

>+++ b/layout/generic/nsGridContainerFrame.cpp
>     if (mEnumerator) {
>+      if (mSkipPlaceholders ||
>+          mEnumerator->get()->GetType() != nsGkAtoms::placeholderFrame) {
>+        ++mGridItemIndex;
>+      }
>       mEnumerator->Next();
>     } else {
>       MOZ_ASSERT(mArrayIndex < mArray->Length(), "iterating past end");
>+      if (mSkipPlaceholders ||
>+          (*mArray)[mArrayIndex]->GetType() != nsGkAtoms::placeholderFrame) {
>+        ++mGridItemIndex;
>+      }
>       ++mArrayIndex;
>     }

Two things:
 (1) This method (Next) should assert !AtEnd() before it does anything, now that it's trying to inspect the frame at our current position. If we're at the end, we'd crash or do something dangerous if we've run off the end, so better to catch that with an up-front assertion.

 (2) We should increment mGridItemIndex in only one place, and collapse the two new "if" clauses together. We can use operator* to do that (abstracting away the mArray/mEnumerator accesses here) -- something like:
     if (mSkipPlaceholders || (**this)->GetType() != nsGkAtoms::placeholderFrame) {
       ++mGridItemIndex;
     }
This would have to go *before* the "if (mEnumerator)" check, of course -- before we've actually advanced.


>+    GridArea& area =
>+      mGridItems.AppendElement(GridItemInfo(PlaceDefinite(child, gridStyle)))->mArea;

This statement is a bit long & complex... Could we simplify it by capturing the GridItemInfo& in a local var ('info'), and then set "GridArea& area = info.mArea"? This helps with my next comment, too...

>+#ifdef DEBUG
>+    mGridItems[iter.GridItemIndex()].mFrame = child;
>+#endif

With my previous suggestion, this can just become:
 info.mFrame = child;
...which is IMO much clearer about what's going on.

(Separately: it might be worth asserting here, in this #ifdef DEBUG chunk, that iter.GridItemIndex() does actually point to the last element, i.e. that it's equal to mGridItems.Length() - 1.)

>@@ -1069,93 +1089,93 @@ nsGridContainerFrame::PlaceGridItems(Gri
>   for (; !iter.AtEnd(); iter.Next()) {
[...]
>+    GridArea& area = mGridItems[iter.GridItemIndex()].mArea;
>+    MOZ_ASSERT(*iter == mGridItems[iter.GridItemIndex()].mFrame);

Include an assertion message; e.g. "iterator out of sync with mGridItems"

>     nsFrameList children(GetChildList(GetAbsoluteListID()));
[...]
>+    size_t i = 0;
>+    for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next(), ++i) {

If you like, this loop could become:
  for (nsIFrame* child : children) {
    ...
    i++;
  }

This seems more readable to me, but the enumerator version is fine too.

(With the range-based formulation, there's a slight risk that "i" could become out-of-sync, if we added a "continue" statement that skipped past the i++ line.  But we probably have no reason to use 'continue' here, so I'm not too concerned about that.)

>-      GridArea area(PlaceAbsPos(child, gridStyle));
>+      GridArea& area =
>+        mAbsPosItems.AppendElement(GridItemInfo(PlaceAbsPos(child, gridStyle)))->mArea;
>+#ifdef DEBUG
>+      mAbsPosItems[i].mFrame = child;
>+#endif

As above, this assignment is a bit complex -- consider using a local GridItemInfo& variable here (and then setting info.mFrame = child).

>@@ -1332,19 +1352,20 @@ nsGridContainerFrame::ReflowChildren(Gri
[...]
>+      MOZ_ASSERT(mGridItems[iter.GridItemIndex()].mFrame == child);

As above: the asserted condition here is non-trivial to interpret. Please include an assertion-message, to give a hint. (e.g. "iterator out of sync with mGridItems")

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8625319 [details] [diff] [review]
part 3 - [css-grid] Implement the "Resolve Intrinsic Track Sizes" algorithm

Review of attachment 8625319 [details] [diff] [review]:
-----------------------------------------------------------------

Partial review:

::: layout/generic/nsGridContainerFrame.cpp
@@ +286,5 @@
> +    for (uint32_t i = start; i < end; ++i) {
> +      const TrackSize& sz = mSizes[i];
> +      space -= sz.mBase;
> +      if (space <= 0) {
> +        return space;

This early-return (if space goes below 0) doesn't quite match the documentation. (The documentation says we return "aSpace minus the sum of base sizes for the tracks in aRange", but here we haven't yet subtracted everything yet.)

Also, I'm not sure the actual 'space' value matters here; might be simpler to just return 0 in this case (indicating "we ran out of space"). That would make it easy to tweak the documentation to reflect reality here, too -- you could just add "or 0 if this subtraction goes below 0" to the end of the documentation.

@@ +357,5 @@
> +  nscoord GrowTracksToLimit(nscoord                   aSpace,
> +                            nsTArray<TrackSize>&      aPlan,
> +                            const nsTArray<uint32_t>& aGrowableTracks) const
> +  {
> +    MOZ_ASSERT(aSpace > 0 && aGrowableTracks.Length() > 0);

Are we really sure that aGrowableTracks is nonempty here?

That array is populated by CollectGrowableLimit(), and it looks like CollectGrowableLimit() can easily leave it empty, if none of our tracks match the bitfield-selector.  I think we could still end up here in that case, as long as we have nonzero space after CollectGrowableLimit().  (We do check |space| before calling DistributeSpaceToTracks, but we don't check whether |tracks| is empty).

So: could you either handle empty |aGrowableTracks| (perhaps by checking whether it's empty in the same spots where we check "space > 0" up a few levels), or clarify this assertion to explain why we know it's non-empty? Or both :D

@@ +358,5 @@
> +                            nsTArray<TrackSize>&      aPlan,
> +                            const nsTArray<uint32_t>& aGrowableTracks) const
> +  {
> +    MOZ_ASSERT(aSpace > 0 && aGrowableTracks.Length() > 0);
> +    nscoord space = aSpace;

This makes me uneasy -- this is asking for a subtle typo somewhere after this point, where we accidentally use 'aSpace' instead of 'space' and end up with subtly broken results.

Let's just get rid of |space| variable entirely, and directly chip away at |aSpace|. (Or alternately, if you want to keep |space|, we should rename one or the other of these vars, to make it harder to mix them up with a subtle typo.)

@@ +359,5 @@
> +                            const nsTArray<uint32_t>& aGrowableTracks) const
> +  {
> +    MOZ_ASSERT(aSpace > 0 && aGrowableTracks.Length() > 0);
> +    nscoord space = aSpace;
> +    uint32_t numGrowable = aGrowableTracks.Length();

This line (combined with how numGrowable is used below) assumes that *no* entries in aGrowableTracks are frozen. Is that intentional?

Elaborating: further down, we chip away at numGrowable whenever we freeze a track, and we return when numGrowable reaches 0. And we skip past frozen tracks when iterating. This all implies that we think numGrowable is actually the count of unfrozen tracks. But if it's not [i.e. if the array is passed in with some already-frozen tracks], then we can easily end up looping forever and never hitting a return statement.

(I don't think anything actually enforces that aGrowableTracks must only contain unfrozen things.  Perhaps we should check for frozenness before appending to it, in CollectGrowableBase?)

(This function's documentation also suggests that tracks could start out frozen -- it says it grows "tracks in aGrowableTracks that aren't frozen". If it assumes that all tracks in the array must be unfrozen, the documentation should make that clearer.)

@@ +365,5 @@
> +      nscoord spacePerTrack = std::max<nscoord>(space / numGrowable, 1);
> +      for (uint32_t track : aGrowableTracks) {
> +        TrackSize& sz = aPlan[track];
> +        if (sz.IsFrozen()) {
> +          continue;

(This is where having some already-frozen tracks will bite us.  In particular, if *all* of aGrowableTracks' entries are frozen at this point, we'll loop forever, since all of the return statements are after this 'continue'.)

@@ +368,5 @@
> +        if (sz.IsFrozen()) {
> +          continue;
> +        }
> +        nscoord base = sz.mBase + spacePerTrack;
> +        if (base > sz.mLimit) {

Maybe s/base/newBase/ ? (I think it represents the tentative new base size.)

@@ +376,5 @@
> +          if (--numGrowable == 0) {
> +            return space;
> +          }
> +        } else {
> +          sz.mBase = std::max(sz.mBase, base);

I don't see why we need this max() (and it doesn't seem to do anything). Note that 'base' is strictly greater than sz.mBase at this point, because it was set to sz.mBase + spacePerTrack, and spacePerTrack is at least 1.

So this line just simplifies to:
   sz.mBase = base;

@@ +390,5 @@
> +    return 0;
> +  }
> +
> +  /**
> +   * Increase the planned size unlimited for tracks in aGrowableTracks that

"Increase...size unlimited" doesn't make sense -- I think you mean "beyond their limit"?

@@ +400,5 @@
> +                                   nsTArray<TrackSize>&      aPlan,
> +                                   const nsTArray<uint32_t>& aGrowableTracks,
> +                                   TrackSize::StateBits      aSelector) const
> +  {
> +    MOZ_ASSERT(aSpace > 0 && aGrowableTracks.Length() > 0);

(As above: how do we know that aGrowableTracks is nonempty here?)

@@ +405,5 @@
> +    uint32_t numGrowable = aGrowableTracks.Length();
> +    if (aSelector) {
> +      for (uint32_t i = 0, len = numGrowable; i < len; ++i) {
> +        const uint32_t track = aGrowableTracks[i];
> +        if (! (aPlan[track].mState & aSelector)) {

Drop space after "!" here

@@ +406,5 @@
> +    if (aSelector) {
> +      for (uint32_t i = 0, len = numGrowable; i < len; ++i) {
> +        const uint32_t track = aGrowableTracks[i];
> +        if (! (aPlan[track].mState & aSelector)) {
> +          aPlan[track].mLimit = -1; // excluded below

Instead of this mLimit special-case, it seems like we should perhaps clear & re-use the "freeze" state-bit.  At least, the spec uses the term "unfreeze" -- and reusing that bit would allow for better code-sharing with the other method (GrowTracksToLimit).

If at all possible, I'd really like to share the entire "while (true)" loop between this method & GrowTracksToLimit, since the code is so similar.  I think we could make that sharing pretty elegant, if we just treat "selected-for-unlimited-growth" tracks here by clearing their eFrozen bit & setting their mLimit to +infinity. (or some other sentinel value which we check for in the shared code).

After doing that processing, I think we could just directly call GrowTracksToLimit, basically, as long as it can handle having some aGrowableTracks already frozen (which it can't currently, I think).

@@ +414,5 @@
> +    }
> +    // 12.5, Distribute space beyond growth limits:
> +    // "if there are no such tracks, then all affected tracks"
> +    if (numGrowable == 0) {
> +      numGrowable = aGrowableTracks.Length();

I don't think this is sufficient -- right now, these tracks will still all have mLimit set to -1, which still makes them "excluded" in the loop below this, despite our now-nonzero numGrowable value.  So we'll loop forever, because all of the tracks are "excluded".

@@ +416,5 @@
> +    // "if there are no such tracks, then all affected tracks"
> +    if (numGrowable == 0) {
> +      numGrowable = aGrowableTracks.Length();
> +    }
> +    nscoord space = aSpace;

(Above note about space/aSpace applies here too; I'd rather we either use more distinct names, or just drop the 'space' variable & directly chip away at |aSpace|. Though, per my next comment, we may end up just passing aSpace directly into a helper method at this point.)

@@ +422,5 @@
> +      nscoord spacePerTrack = std::max<nscoord>(space / numGrowable, 1);
> +      for (uint32_t track : aGrowableTracks) {
> +        TrackSize& sz = aPlan[track];
> +        if (sz.mLimit == -1) {
> +          continue; // an excluded track

Several notes:
 (1) (This is where I think we'll loop forever, if we leave all the mLimits equal to -1, as noted above.

 (2) This whole "while(true) {" loop is a slightly-simplified version of the same code in GrowTracksToLimit.  Due to the similarities, it'd be great if we could just share code (maybe even calling GrowTracksToLimit here directly), if possible.

I think we can share code pretty well if we clear the "freeze" state-bit & set mLimit to NS_UNCONSTRAINEDSIZE on each "unlimited-growth" track. (This matches what the spec suggests -- it says to "unfreeze".)  Then, we could directly call GrowTracksToLimit and it'll just do the right thing, as long as it can handle having some aGrowableTracks already frozen (which it can't currently, as noted in my comments for it above -- but it probably should be adjusted to handle that).

@@ +1746,5 @@
> +      // Step 1. Size tracks to fit non-spanning items.
> +      ResolveIntrinsicSizeStep1(aState, aFunctions, aPercentageBasis,
> +                                aConstraint, lineRange, child);
> +    } else {
> +      TrackSize::StateBits state = TrackSize::StateBits(0);

Can't this just be
  TrackSize::StateBits state(0);

@@ +1775,5 @@
> +          maxContent = MaxContentContribution(child, aState.mReflowState,
> +                                              rc, wm, mDimension);
> +        }
> +        step2Items.AppendElement(Step2ItemData({span, state, lineRange, minSize,
> +                minContent, maxContent, child}));

Indentation on 2nd line of AppendElement call seems a bit arbitrary here.

Might be cleaner w/ newline+2-space-indent after AppendElement(, and then everything correctly aligned after that, like so:
        step2Items.AppendElement(
          Step2ItemData({span, state, lineRange, minSize,
                         minContent, maxContent, child}));

::: layout/generic/nsGridContainerFrame.h
@@ +58,5 @@
> +#ifdef DEBUG
> +    void Dump() const;
> +#endif
> +    enum StateBits : uint16_t {
> +      eIntrinsicMinSizing =      0x1,

I was initially confused about how this bit (eIntrinsicMinSizing) is used and how that corresponds to the spec.  After stumbling around, I think (?) we use this bit as a "catch-all" to imply one of the 3 bits below it, right?

Could you document this meaning somewhere? Perhaps next to or above the bit here?

OR better (I think): could we just make this into a bitmask for min-content|max-content|auto, instead of giving it its own bit? (From looking at where we set this eIntrinsicMinSizing bit, in TrackSize::Initialize, I don't see how this dedicated bit adds any information.)  Note that we're already using bitmasks here for other things, e.g. "eMinOrMaxContentMinSizing", so if we can use them for this as well, that seems like a Good Thing.

@@ +64,5 @@
> +      eMinContentMinSizing =     0x4,
> +      eMaxContentMinSizing =     0x8,
> +      eMinOrMaxContentMinSizing = eMinContentMinSizing | eMaxContentMinSizing,
> +      eFlexMinSizing =          0x10,
> +      eIntrinsicMaxSizing =     0x20,

(The above all applies to this bit, eIntrinsicMaxSizing, as well.)

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> If at all possible, I'd really like to share the entire "while (true)" loop
[...]
>  (2) This whole "while(true) {" loop is a slightly-simplified version

[gah, sorry -- most of these ^ two comments are basically saying the same thing. I changed my mind about where to insert this comment, and copypasted with edits, but forgot to delete the older version from Splinter]

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 1 - Introduce a few local structs (GridReflowState, Tracks, TrackSizingFunctions) to make it easier to pass around data

(In reply to Daniel Holbert [:dholbert] (out 8/5-8/10) from comment #9)
> In PlaceGridItems & ReflowChildren, I'd prefer we directly use
> 'aState.mIter' rather than abstracting it away with a local alias.

'aState.mIter' is less readable, but fair enough.

> Nit: there's a type mismatch here. (aFunctions.mExplicitGridOffset has type
> uint32_t, and we're copying its value into a size_t.)

Changed it to uint32_t.  (I think I was using size_t here only
to avoid a warning at some point.)

> > +nsGridContainerFrame::CalculateTrackSizes(GridReflowState&   aState,
> > +                                          const LogicalSize& aContentSize)
> 
> "aContentSize" is ambiguous (unclear from the name whether it's the size of
> our contents, vs. our content-box size).

Renamed it aContentBox.  The "Size" part seems redundant in context.

> > +nsGridContainerFrame::ContainingBlockFor(GridReflowState& aState,
> 
> Looks like aState can be 'const' here. (This is a read-only method; doesn't
> modify the state.)

Made it so.

> > +nsGridContainerFrame::ContainingBlockForAbsPos(GridReflowState&    aState,

And here.

> Note: the 'nsHTMLReflowState& aReflowState' parameter to this method
> (ReflowChildren) is now somewhat redundant, since the new GridReflowState
> parameter brings along a pointer to the same object.

Removed it as suggested.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 2 - Introduce a local GridItemInfo struct for holding a grid item's GridArea and other data...

(In reply to Daniel Holbert [:dholbert] (out 8/5-8/10) from comment #10)
>  (1) This method (Next) should assert !AtEnd() before it does anything,

Fixed.

>  (2) We should increment mGridItemIndex in only one place, and collapse the
> two new "if" clauses together. We can use operator* to do that (abstracting
> away the mArray/mEnumerator accesses here) -- something like:
>      if (mSkipPlaceholders || (**this)->GetType() !=
> nsGkAtoms::placeholderFrame) {
>        ++mGridItemIndex;
>      }

The reason I didn't do that is that it would make the code slightly slower.
But fair enough, mSkipPlaceholders is true in most loops such that (**this)
won't be evaluated in most cases.

> >+      mGridItems.AppendElement(...
> This statement is a bit long & complex... 

Fixed as suggested.

> (Separately: it might be worth asserting here, in this #ifdef DEBUG chunk,
> that iter.GridItemIndex() does actually point to the last element, i.e. that
> it's equal to mGridItems.Length() - 1.)

Sure.

> Include an assertion message; e.g. "iterator out of sync with mGridItems"

Fixed.

> If you like, this loop could become:
>   for (nsIFrame* child : children) {
>     ...
>     i++;
>   }

I think incrementing it in the 'for' is more idiomatic.

> >+        mAbsPosItems.AppendElement(...
> As above, this assignment is a bit complex

OK, fixed.

> >+      MOZ_ASSERT(mGridItems[iter.GridItemIndex()].mFrame == child);
> 
> As above: the asserted condition here is non-trivial to interpret.

Fixed.

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Implement the "Resolve Intrinsic Track Sizes" algorithm

(In reply to Daniel Holbert [:dholbert] (out 8/5-8/10) from comment #11)
> This early-return (if space goes below 0) doesn't quite match the
> documentation.

I made it return 0 as suggested and updated the documentation.

> > +    MOZ_ASSERT(aSpace > 0 && aGrowableTracks.Length() > 0);
> 
> Are we really sure that aGrowableTracks is nonempty here?

Yes, all CollectGrowable* calls are preceeded by a
          if (!(item.mState & FLAG)) {
            continue;
          }
and FLAG is only set if at least one spanned track has FLAG,
so we must find that track (unless we ran out of space before that).

> > +    nscoord space = aSpace;
> Let's just get rid of |space| variable entirely, and directly chip away at
> |aSpace|.

I don't like modifying in-params, so I renamed aSpace to aAvailableSpace
instead.

> > +    uint32_t numGrowable = aGrowableTracks.Length();
> 
> This line (combined with how numGrowable is used below) assumes that *no*
> entries in aGrowableTracks are frozen. Is that intentional?

Yes, that is correct.

> (I don't think anything actually enforces that aGrowableTracks must only
> contain unfrozen things.  Perhaps we should check for frozenness before
> appending to it, in CollectGrowableBase?)

Sure, I added a MOZ_ASSERT there.

> If it assumes that all tracks in the array must be unfrozen, the
> documentation should make that clearer.)

I updated the documentation.

> > +        nscoord base = sz.mBase + spacePerTrack;
> > +        if (base > sz.mLimit) {
> 
> Maybe s/base/newBase/ ? (I think it represents the tentative new base size.)

Sure, fixed.

> > +          sz.mBase = std::max(sz.mBase, base);
> 
> I don't see why we need this max() (and it doesn't seem to do anything).
> Note that 'base' is strictly greater than sz.mBase at this point, because it
> was set to sz.mBase + spacePerTrack, and spacePerTrack is at least 1.

Ah, good point.  (this std::max was a left-over from an earlier version)

> "Increase...size unlimited" doesn't make sense -- I think you mean "beyond
> their limit"?

Fixed.

> (As above: how do we know that aGrowableTracks is nonempty here?)

Same reason.

> > +        if (! (aPlan[track].mState & aSelector)) {
> 
> Drop space after "!" here

Fixed.

> > +          aPlan[track].mLimit = -1; // excluded below
> 
> Instead of this mLimit special-case, it seems like we should perhaps clear &
> re-use the "freeze" state-bit.

I changed this to use two new bits, more on why below...

> > +    // "if there are no such tracks, then all affected tracks"
> > +    if (numGrowable == 0) {
> > +      numGrowable = aGrowableTracks.Length();
> 
> I don't think this is sufficient -- right now, these tracks will still all
> have mLimit set to -1, which still makes them "excluded" in the loop below
> this, despite our now-nonzero numGrowable value.  So we'll loop forever,
> because all of the tracks are "excluded".

Good catch!  This also reveals a gaping hole in the test coverage.
I have fixed this by introducing two new bits to mark the tracks
that are excluded.

I also realized that I misunderstood the spec in this case.
Specifically, the spec text:
 * when handling min-content or auto base sizes: 
   any affected track that happens to also have an intrinsic max track
   sizing function; if there are no such tracks, then all affected tracks.
 * when handling max-content base sizes: any affected track that happens
     to also have a max-content max track sizing function; if there are
     no such tracks, then all affected tracks. 

So, these are two independent sets of tracks that each needs a bit.
Here's how I read that spec text:
Let the tracks with either a 'min-content' or 'auto' min-sizing
function be set A.  For each track in A check if it has an intrinsic
max-sizing function, mark it as excluded if not.  If all tracks in
set A were excluded then unmark all the tracks in set A.
(Ditto for the second bullet above, as a separate set)

> I think we can share code pretty well if we clear the "freeze" state-bit &
> set mLimit to NS_UNCONSTRAINEDSIZE on each "unlimited-growth" track.

I've noted your suggestions on refactoring the code, but I'd
like to punt on those to a follow-up bug, if you don't mind.
I'm focusing on correctness, with an eye to performance as well,
at this point.  Once the sizing code is complete (soon), I'll
consider refactoring some of this if it can be done without
affecting performance too much.

> > +      TrackSize::StateBits state = TrackSize::StateBits(0);
> 
> Can't this just be
>   TrackSize::StateBits state(0);

My clang says:
error: cannot initialize a variable of type 'TrackSize::StateBits' with
an rvalue of type 'int'

> Indentation on 2nd line of AppendElement call seems a bit arbitrary here.

Fixed.

> ::: layout/generic/nsGridContainerFrame.h
> > +      eIntrinsicMinSizing =      0x1,
> OR better (I think): could we just make this into a bitmask for
> min-content|max-content|auto, instead of giving it its own bit? 

OK, fixed as suggested.  (same for eIntrinsicMaxSizing)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8647155 [details] [diff] [review]
part 3 - [css-grid] Implement the "Resolve Intrinsic Track Sizes" algorithm

Review of attachment 8647155 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +238,5 @@
> +   * Return true if aRange spans at least one track with an intrinsic
> +   * sizing function and does not span any flex tracks.
> +   * @param aRange the span of tracks to check
> +   * @param aState will be set to the union of the state bits for the tracks
> +   *               when this method returns true, the value is undefined when

s/true, the value/true. The value/

@@ +1479,3 @@
>      case eStyleUnit_Enumerated:
> +      mState = IsMinContent(aMinCoord) ? eMinContentMinSizing
> +                                       : eMaxContentMinSizing;

In this clause and the one above it (in nsGridContainerFrame::TrackSize::Initialize), you use "=" to assign mState, but I think everywhere else you use "|=".

Let's make these consistent with the rest of the code and use "|=" (unless there's a reason for the difference that I'm missing).  That way, we can set other bits earlier than this, as-needed, and we won't accidentally stomp on them in a subset of the switch cases.

@@ +1483,2 @@
>      case eStyleUnit_FlexFraction:
> +      mState |= eFlexMinSizing;

Is there a reason we use "|=" when setting mState in this clause, vs. "=" in the 2 clauses above it? (Note that mState is 0 when we enter this switch statement.)

Probably best to be consistent on this, unless there's a reason for the difference that I'm missing. (I lean towards replacing the assignments with "|=", so you can set other bits earlier than this as-needed later on and not accidentally stomp on them.)

@@ +1660,5 @@
> +  bool foundIntrinsic = false;
> +  for (uint32_t i = start; i < end; ++i) {
> +    TrackSize::StateBits state = mSizes[i].mState;
> +    if (state & TrackSize::eFlexMaxSizing) {
> +      return false;

Don't we need to check for eFlexMinSizing here, too? (Right now you just check for eFlexMaxSizing, but not MinSizing).

The spec (& this function's name & documentation) don't seem to be max-sizing-specific.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> @@ +1483,2 @@
> >      case eStyleUnit_FlexFraction:
> > +      mState |= eFlexMinSizing;
> 
> Is there a reason we use "|=" when setting mState in this clause, vs. "="

(Sorry, disregard this point; I rephrased it as the point above it, and forgot to delete the earlier formulation.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8647155 [details] [diff] [review]
part 3 - [css-grid] Implement the "Resolve Intrinsic Track Sizes" algorithm

Review of attachment 8647155 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following and comment 3 addressed as you see fit (& sorry again for the delay & for repeating myself in comment 3)

::: layout/generic/nsGridContainerFrame.cpp
@@ +384,5 @@
> +  {
> +    bool foundOneSelected = false;
> +    bool foundOneGrowable = false;
> +    uint32_t numGrowable = aNumGrowable;
> +    for (auto track : aGrowableTracks) {

s/auto/uint32_t/ here -- better to make the type clear here. (since there are various "track"-related types in this file, so it's not immediately clear from this line of code what the type is.)

(Most of your other "for (... track : fooTracks)" loops use "uint32_t", not "auto" -- the loops in this function are the exception)

@@ +389,5 @@
> +      TrackSize& sz = aPlan[track];
> +      const auto state = sz.mState;
> +      if (state & aMinSizingSelector) {
> +        foundOneSelected = true;
> +        if (state & aMaxSizingSelector) {

The variable "state" seems a bit unnecessary here -- it's only used twice, and it only saves you a 4 characters on each usage ("sz.mState" is barely longer).  And it adds a little complexity by introducing a new variable / new line of code.

So: consider just directly using sz.mState & dropping 'state'.

@@ +400,5 @@
> +      }
> +    }
> +    // 12.5 "if there are no such tracks, then all affected tracks"
> +    if (foundOneSelected && !foundOneGrowable) {
> +      for (auto track : aGrowableTracks) {

As above, s/auto/uint32_t/ here.

@@ +410,5 @@
> +  }
> +
> +  /**
> +   * Increase the planned size for tracks in aGrowableTracks that match
> +   * aSelector beyond their limit.  This implements the "Distribute space

Nit: the "tracks...that match aSelector" documentation here isn't quite correct -- you've got special-case handling for the StateBits(0) selector.  

One would naturally expect that an empty selector would select the empty set (since no bits would match), but this function actually treats that as a special-case and uses it to select *all* tracks. (and you have one caller that depends on this behavior)

So -- maybe worth calling out that special value / special case in a brief clarifying footnote in the documentation here, something like:
    * Note: to select all tracks, use a blank aSelector, i.e. StateBits(0).

@@ +441,5 @@
> +      }
> +    }
> +    nscoord space = aAvailableSpace;
> +    while (true) {
> +      nscoord spacePerTrack = std::max<nscoord>(space / numGrowable, 1);

Is numGrowable really the right thing to divide by at this point, in the scenario where we made two calls to MarkExcludedTracks()?

In that scenario, numGrowable will be value returned by the *second* call, and it's the number of tracks that don't have the eSkipGrowUnlimited2 bit set. But then in this loop where we use numGrowable to determine spacePerTrack, we actually consider *both* "skip" bits -- i.e. we skip tracks that have *either* eSkipGrowUnlimited bit set. So I don't think numGrowable necessarily corresponds to the actual number of tracks that are really growable here in this loop.

(Unless the second group of numGrowable tracks must be a strict superset of the first numGrowable tracks, i.e. if eSkipGrowUnlimited2 can only ever be set if eSkipGrowUnlimited1 is also set... is that the case? It's not obvious to me that it is, but maybe that's why this is the way it is.)

@@ +1830,5 @@
> +
> +      bool updatedBase = false; // Did we update any mBase in step 2.1 - 2.3?
> +      if (stateBitsPerSpan[span] & TrackSize::eIntrinsicMinSizing) {
> +        // Step 2.1 MinSize to intrinsic min-sizing.
> +        for (i = spanGroupStartIndex; i < spanGroupEndIndex; ++i) {

Observation: this same chunk of 23 lines of code (starting with the stateBitsPerSpan[span] check) gets repeated, back-to-back, 3 times here (for step 2.1, and then 2.2, and then 2.3).  The only difference between the repetitions is the exact state flags that we use, and the Step2ItemData field that we distribute. (e.g. mMinSize vs. mMinContentContribution)

Seems like there might be a refactoring opportunity for this chunk, or the bulk of it, to reduce repetition. (Or does that make things too clumsy/abstract?)

(It's actually *nearly* repeated 5 times -- not just 3 times -- but the last 2 repetitions are for Limit instead of Base, so their differences are more substantial than just state flags. So to the extent that we can refactor usefully, there might be diminishing returns on supporting the last 2 repetitions.)

@@ +1947,5 @@
> +          for (i = spanGroupStartIndex; i < spanGroupEndIndex; ++i) {
> +            Step2ItemData& item = step2Items[i];
> +            if (item.mSpan != span) {
> +              break;
> +            }

Do we actually need this "item.mSpan != span" check here?  I don't think we do... We're only iterating among items from spanGroupStartIndex up to spanGroupEndIndex, which should all have the same mSpan I think, so I don't think we need to worry about checking mSpan here.

(Note that the 4 similar chunks above this one (for steps 2.1 through 2.5) do not have this mSpan check.)

@@ +1967,5 @@
> +        }
> +      }
> +    }
> +  }
> +  

Nit: drop whitespace on blank line

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> r=me with the following and comment 3 addressed as you see fit (& sorry
> again for the delay & for repeating myself in comment 3)

(*facepalm*.  By "comment 3" I meant "Comment 16". [I got distracted by "part 3" at the top of Comment 16 & misread it as the comment number.])

Comment 20

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> Don't we need to check for eFlexMinSizing here, too? (Right now you just
> check for eFlexMaxSizing, but not MinSizing).

A <flex> min-sizing function is special:
https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-minmax
"As a maximum, a <flex> value sets the track's flex factor. As a minimum, it is treated as zero (or min-content, if the grid container is sized under a min-content constraint)."

So, for example, a minmax(1fr, auto) track is not actually flexing.
The relevant spec text for the code you quoted says:
https://drafts.csswg.org/css-grid/#algo-content (bullet 2):
"Next, consider the items with a span of 2 that do not span a track with a flexible sizing function:"

I consider the "with a flexible sizing function" here to be after converting
the min-sizing to zero / min-content.  The conversion seems rather pointless
otherwise.  It's quite clear that 12.7 (flexing) deals only with tracks that
have <flex> max-sizing function (since that sets the track's flex factor per
my first spec link), so if we would skip <flex> min-sizing tracks too in
12.5 the min-content case wouldn't contribute in the algo that deals with
intrinsic track sizing, which seems wrong.

Actually, now that I think about it, it looks like I forgot to deal with
this for span>1.  I got it right in ResolveIntrinsicSizeStep1 for span=1
tracks, but we need something similar for span>1.  (BTW, the spec used to
say a <flex> min-sizing should always be treated as zero.)

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 3b - fix <flex> min-sizing under min-content constraint

This wasn't too complicated so I think we can handle it as an addendum to
part 3 here.  I'll fold it before landing.

When under a min-content constraint:
* make HasIntrinsicButNoFlexSizingInRange count <flex> min-sizing tracks
  as intrinsic 
* add eFlexMinSizing to the track selector in steps 2.1 and 2.2 (where
  we handle 'min-content' min-sizing)
* relax an assertion to allow eFlexMinSizing

(I've added ~1500 lines worth of tests for <flex> min-sizing but that comes
later in the series since the layout depends on a few other bugs too.)

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8652523 [details] [diff] [review]
part 3b - fix <flex> min-sizing under min-content constraint

Review of attachment 8652523 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following:

::: layout/generic/nsGridContainerFrame.cpp
@@ +236,5 @@
>    /**
>     * Return true if aRange spans at least one track with an intrinsic
>     * sizing function and does not span any flex tracks.
>     * @param aRange the span of tracks to check
> +   * @param aConstraint if MIN_ISIZE, treat a <flex> min-sizing as 'min-content'

Probably worth clarifying the phrase "does not span any flex tracks" (2nd line of this documentation) to say something like "does not span any tracks with a <flex> max-sizing function".

(The existing "flex tracks" language here is becoming less-obviously max-content-specific, IMO, now that we're explicitly bringing up <flex> min-sizing right after it.)

@@ +1848,5 @@
>        }
>  
>        bool updatedBase = false; // Did we update any mBase in step 2.1 - 2.3?
> +      TrackSize::StateBits selector(flexMin | TrackSize::eIntrinsicMinSizing);
> +      if (stateBitsPerSpan[span] & selector) {

Note: this section of repeated code is now even more ripe for refactoring into a helper-function, per my 'Observation:' in comment 17,  now that we're abstracting out the 'selector' into a variable. (That was one of the two things that differed.)

After this patch, the first two instances of this repeated code are 100% identical except for their choice of 'space' local-variable -- and IIRC the 3rd might be just as identical, if it used a 'selector' local variable too.

Please file a followup for this refactoring, unless you see some reason we shouldn't do it.

Comment 23

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #16)
> s/true, the value/true. The value/

OK, I rewrote that comment in part 3b.

> In this clause and the one above it (in
> nsGridContainerFrame::TrackSize::Initialize), you use "=" to assign mState,
> but I think everywhere else you use "|=".

I made it consistently '=' for the initial assignments.
I think '|=' gives a false impression that some bits may already
be set there, when they shouldn't be (we assert that).


(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #18)
> > +    for (auto track : aGrowableTracks) {
> 
> s/auto/uint32_t/ here -- better to make the type clear here.

Fair enough, fixed.

> @@ +389,5 @@
> > +      TrackSize& sz = aPlan[track];
> > +      const auto state = sz.mState;
> > +      if (state & aMinSizingSelector) {
> > +        foundOneSelected = true;
> > +        if (state & aMaxSizingSelector) {
> 
> The variable "state" seems a bit unnecessary here

It allows the compiler to use a register rather than refetching
sz.mState for the second use.  Stuff like do matter for performance.

> Nit: the "tracks...that match aSelector" documentation here isn't quite
> correct -- you've got special-case handling for the StateBits(0) selector.

OK, I've documented that.

> Is numGrowable really the right thing to divide by at this point, in the
> scenario where we made two calls to MarkExcludedTracks()?

There are two calls:
  numGrowable = MarkExcludedTracks(aPlan, numGrowable, ... bit1)
  numGrowable = MarkExcludedTracks(aPlan, numGrowable, ... bit2)
the 'numGrowable' arg to the first call is aGrowableTracks.Length(), it
returns that number minus those marked w. bit1, the second call takes
that number and subtracts the number of tracks marked w. bit2.
The same track is never marked with both bits - it's easy to see that
since the first call only selects eMaxContentMinSizing and that bit
is explicitly excluded for the second call:
      aSelector & ~TrackSize::eMaxContentMinSizing;

> Observation: this same chunk of 23 lines of code

Yeah, as I said earlier, I'll circle back to this function once all
the sizing code (and tests) are in place and then refactor / optimize
it while measuring performance.  Filed bug 1201932.

> > +          for (i = spanGroupStartIndex; i < spanGroupEndIndex; ++i) {
> > +            Step2ItemData& item = step2Items[i];
> > +            if (item.mSpan != span) {
> > +              break;
> > +            }
> 
> Do we actually need this "item.mSpan != span" check here?

Nope, I meant to remove that.  Thanks!

> Nit: drop whitespace on blank line

Fixed.

(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #22)
> Probably worth clarifying the phrase "does not span any flex tracks"

Fixed.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96fd29a6c902
https://hg.mozilla.org/integration/mozilla-inbound/rev/1194452bc071
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada43c8af8ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5260ff7a146
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2be5367116

Comment 25

[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/19e1ac94c6c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3be224872bad
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e91fd985dcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf33493cd70
https://hg.mozilla.org/integration/mozilla-inbound/rev/dafa4f1221cb

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55426564ef57
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2257fc4cacc
https://hg.mozilla.org/integration/mozilla-inbound/rev/390aba3006bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbfbfa04bbc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/26547d95274d

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55426564ef57
https://hg.mozilla.org/mozilla-central/rev/f2257fc4cacc
https://hg.mozilla.org/mozilla-central/rev/390aba3006bc
https://hg.mozilla.org/mozilla-central/rev/dbfbfa04bbc3
https://hg.mozilla.org/mozilla-central/rev/26547d95274d