1465296 - [css-grid-2] Implement subgrid item placement

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid-2] Implement subgrid item placement

This is the basic algo for recursing into subgrids to do
PlaceGridItems on them from the nearest non-subgrid grid
container.

(A couple more parts coming later to deal with clamping
and resolving line names...)

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid-2] Clamp lines in a subgrid to its extent

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid-2] Implement nested line name lookups for subgrids

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid-2] Implement nested area name lookups for subgrids

There might be some spec issues to sort out with area name matching,
but I'll sort that out later in a separate bug if so.
See https://github.com/w3c/csswg-drafts/issues/2564

Comment 5

[Mats Palmgren (inactive)]

That's all patches for this bug and it completes the grid placement
implementation for subgrids.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid-2] Implement subgrid item placement

Minor perf tweak - use std::move at the end of SubgridPlaceGridItems
to avoid copying the arrays.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8983377 [details] [diff] [review]
part 1 - [css-grid-2] Implement subgrid item placement

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +159,5 @@
> +  if (aMaxCoord.ConvertsToLength()) {
> +    *aMax = std::max(*aMin, aMaxCoord.ToLength());
> +  }
> +  if (aSizeCoord.ConvertsToLength()) {
> +    *aSize = Clamp(aSizeCoord.ToLength(), *aMin, *aMax);

If we fail one (or both) of the first two ConvertsToLength() calls (for aMinCoord/aMaxCoord), then we'll be using *aMin and/or *aMax here without ever giving them a value (in this function at least).

Fortunately it looks like the callers happen to always initialize them to 0 / NS_UNCONSTRAINEDSIZE, so this Clamp() call will fall back gracefully. So: since this function's operation depends on the callers having done this particular initialization (to those particular values), perhaps we should add an assertion to that affect to the beginning of this function, to validate & document that requirement? e.g.:
   MOZ_ASSERT(*aMin == 0 && *aMax == NS_UNCONSTRAINEDSIZE,
              "...");

[This is less important in the code as it currently stands, because this is a lambda function & it's right alongside the decls of the args that it'd be assertion-checking. But, spun out, it probably makes sense to sanity-check the args.]

@@ +603,5 @@
>      return IsSubgrid(eLogicalAxisInline) || IsSubgrid(eLogicalAxisBlock);
>    }
>  
> +  // Return the grid container frame associated with this subgrid item.
> +  nsGridContainerFrame* SubgridFrame() const

This documentation isn't quite specific enough -- it's not clear whether this is talking about the *outer* grid container or the *inner* grid container. (Both of those containers are arguably "associated" with this subgrid item.)

Maybe add "(inner)" here, to disambiguate? e.g.:
  // Return the (inner) grid container frame associated with this subgrid item.

Or "child", or something to that effect...

@@ +3234,5 @@
> +  Grid*               aParentGrid,
> +  const GridItemInfo& aGridItem)
> +{
> +  MOZ_ASSERT(aGridItem.mArea.IsDefinite() ||
> +             aGridItem.mFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW));

Let's include a brief explanatory assertion message here, so that it's slightly less opaque what's going on & how much we should worry, if/when this assertion is made to fail by some fuzzer testcase. :)

@@ +3261,5 @@
> +  }
> +
> +  // Abs.pos. subgrids may have kAutoLine in their area.  Map those to the edge
> +  // line in the parent's explicit grid (zero-based line numbers).  This is ok
> +  // because it's only used for translating lines and such, not for layout.

I don't fully understand what the last part of this comment is saying.  It's hinting that using these lines for layout would be not-OK [and emphasizing that we're not doing that].  But I'm not immediately clear about *what* precisely would be not-OK & why...

@@ +3267,5 @@
> +    subgrid->mArea.mCols.mStart = 0;
> +  }
> +  if (MOZ_UNLIKELY(subgrid->mArea.mCols.mEnd == kAutoLine)) {
> +    subgrid->mArea.mCols.mEnd = aParentGrid->mExplicitGridColEnd +
> +                                aParentGrid->mExplicitGridOffsetCol - 1;

It took me a little bit to reason about this arithmetic. One question to sanity-check my understanding, and one suggestion:

QUESTION: Is the "- 1" at the end here intended to convert to be 0-based? (which we have to do because "mExplicitGridColEnd" is 1-based?)

SUGGESTION: This might be easier to reason about if you swap the order of the two variables, to phrase it like so:
 subgrid->mArea.mCols.mEnd = aParentGrid->mExplicitGridOffsetCol +
                             aParentGrid->mExplicitGridColEnd - 1;

...because IIUC:
 - This would then have us stepping incrementally through the grid (starting at the start edge of the implicit grid, the first term here steps over the offset from that start-edge to the start of the explicit grid; and then the rest is to make the next jump across the distance from the start of the explicit grid to the line that we care about).
 - mExplicitGridOffsetCol is 0-based and is typically 0, so "mExplicitGridOffsetCol - 1" is a bit confusing to look at in isolation.
 - On the other hand, mExplicitGridColEnd is 1-based and hence it makes more sense conceptually to see us subtracting 1 from that as part of this expression.

(If you agree, then this applies a few lines further down, too, for the equivalent row expression.)

@@ +3490,5 @@
>                                         : &Grid::PlaceAutoCol;
>    uint32_t clampMaxMajorLine = isRowOrder ? clampMaxRowLine : clampMaxColLine;
>    aState.mIter.Reset();
>    for (; !aState.mIter.AtEnd(); aState.mIter.Next()) {
> +    auto& item = aState.mGridItems[aState.mIter.ItemIndex()];

Let's update the (preexisting) assertion 2 lines below this line to reuse this new "item" alias, rather than repeating  aState.mGridItems[aState.mIter.ItemIndex()]

(I'm taking about the assertion with message "iterator out of sync with aState.mGridItems")

@@ +6197,5 @@
> +      grid.PlaceGridItems(gridReflowInput, aReflowInput.ComputedMinSize(),
> +                          computedSize, aReflowInput.ComputedMaxSize());
> +    } else {
> +      auto* subgrid = GetProperty(Subgrid::Prop());
> +      gridReflowInput.mGridItems = subgrid->mGridItems;

Let's explicitly assert that |subgrid| is non-null here and add a brief justification/explanation in the assert message. (It took me a few minutes to figure out how we know it'll be initialized before this point.  I think (?) it's because, if we're a subgrid, we can expect that our parent-grid will have called SubgridPlaceGridItems() before calling our Reflow() method, and that earlier call will have given us an entry for Subgrid::Prop(). Right?)

(At first I was looking for places where the "this" grid instance would've called SubgridPlaceGridItems before this line, and I wasn't finding any such places. But after a bit I realized it's because the parent would be responsible for doing that... unless I've just confused myself further. :))

@@ +6198,5 @@
> +                          computedSize, aReflowInput.ComputedMaxSize());
> +    } else {
> +      auto* subgrid = GetProperty(Subgrid::Prop());
> +      gridReflowInput.mGridItems = subgrid->mGridItems;
> +      gridReflowInput.mAbsPosItems = subgrid->mAbsPosItems;

Further up in this file, you use std::move when copying these arrays (mGridItems and mAbsPosItems) in the other direction, from State to Subgrid, per comment 6.

Can/should we do that here too?  Or are we intentionally needing to copy the whole array here? (Is there anything we can do to avoid copying, e.g. maybe move'ing them to our temporary GridReflowInput struct and then move'ing them back to the Subgrid object at the end of our Reflow method?)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8983377 [details] [diff] [review]
part 1 - [css-grid-2] Implement subgrid item placement

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

(Tentative r+ w/ above comments addressed as you see fit, though feel free to have me take a look at the updated version too if you like.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8982507 [details] [diff] [review]
part 3 - [css-grid-2] Implement nested line name lookups for subgrids

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

r=me with a few reservations; as above, feel free to re-request review if the updates are nontrivial & feel like they'd merit another look (particularly for my lowest comment about anonymous boxes & GetParent()).

::: layout/generic/nsGridContainerFrame.cpp
@@ +789,5 @@
>     *   in a subgrid axis it's its explicit grid bounds (all 1-based)
> +   * @param aParentLineNameMap the parent grid's map parallel to this map, or
> +   *                           null if this map isn't for a subgrid
> +   * @param aRange the subgrid's range in the parent grid, or null
> +   * @param aIsSameDirection true if the subgrid/parent axes progresses in

typo: s/progresses/progress/

(because "axes" is plural, and "it progresses" vs "they progress")

Or, if you want to emhasize that the "axes" here are kind of a single axis, maybe rephrase as something like:
 "if our axis progresses in the same direction in the subgrid & parent"

@@ +935,5 @@
> +      MOZ_ASSERT(line >= 1, "expected a 1-based line number");
> +      aIndex = line - 1;
> +      map = parent;
> +    }
> +    MOZ_ASSERT_UNREACHABLE("");

Let's include *some* nonempty string to hint at why this is unreachable. :)  (It's obvious after analyzing the loop right now, but it might become less obvious [or invalid] if this code gets more complex in the future...)

Maybe:
    MOZ_ASSERT_UNREACHABLE("The only way out of loop above is via 'return'");

@@ +991,5 @@
> +  const LineNameMap* mParentLineNameMap;
> +  // The subgrid's range, or null if this map isn't for a subgrid.
> +  const LineRange* mRange;
> +  // True if the subgrid/parent axes progresses in the same direction.
> +  const bool mIsSameDirection;

As above, "axes progresses" is technically a typo. s/progresses/progress/, or rephrase.

@@ +2445,5 @@
> +    if (!mParentGrid) {
> +      return nullptr;
> +    }
> +    bool rows = aIsOrthogonal == (aAxis == eLogicalAxisInline);
> +    return rows ? mParentGrid->mRowNameMap : mParentGrid->mColNameMap;

How about we name the boolean "useRows" or "isRows", to make it more clearly bool-flavored?  (The current name, "rows", sounds quite a lot like a collection of rows / row-related things, which makes this a little harder to quickly skim/grok.)

@@ +3451,5 @@
>      numRepeatCols = cols.HasRepeatAuto() ?
>        std::max<uint32_t>(extent - cols.mLineNameLists.Length(), 1) : 0;
> +    parentLineNameMap = ParentLineMapForAxis(subgrid->mIsOrthogonal,
> +                                             eLogicalAxisInline);
> +    auto parentWM = aState.mFrame->GetParent()->GetWritingMode();

Is aState.mFrame->GetParent() really the right thing here?

In particular: if aState.mFrame is a subgrid which happens to be overflow:scroll (or have some other anonymous wrapper around us), won't this give us the WM for the intermediate wrapper frame, rather than WM for the the outer grid?  (because aState.mFrame->GetParent() would be the anonymous box, I think...)

(This question applies to the similar code below for the row/block axis, too.)

Comment 10

[Daniel Holbert [:dholbert]]

(I'm calling it a night for the moment -- I'll review part 4 tomorrow. Sorry for the wait on these up to this point.)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8982517 [details] [diff] [review]
part 4 - [css-grid-2] Implement nested area name lookups for subgrids

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

Partial review of part 4 (haven't fully grokked it yet, but I've got what I think are some substantive comments for the time being):

::: layout/generic/nsGridContainerFrame.cpp
@@ +860,5 @@
>    }
>  
> +  /**
> +   * Return a set of lines in aImplicitLines which matches the area name aName
> +   * on aSide.  For example, for aName "a" and aSide being an end side it

nit: add comma after "end side"

@@ +863,5 @@
> +   * Return a set of lines in aImplicitLines which matches the area name aName
> +   * on aSide.  For example, for aName "a" and aSide being an end side it
> +   * returns the line numbers which would match "a-end" in the relevant axis.
> +   * For subgrids it includes searching the relevant axis in all ancestor
> +   * grids too.  If an ancestor has opposite direction, we switch aSide to

s/too./too (within this subgrid's spanned area)./

Otherwise, with the wording here right now, it sounds like this is saying we search the *full grid* of all ancestor grids. But I don't think that's the case (right?)

@@ +872,5 @@
> +                      nsTArray<uint32_t>& aImplicitLines) const
> +  {
> +    // True if we're currently in a map that has the same direction as 'this'.
> +    bool sameDirectionAsThis = true;
> +    uint32_t min = mClampMinLine < 0 ? 1 : mClampMinLine;

It looks like negative mClampMinLine values get clamped up to 1, but 0 gets to stay as 0 (it doesn't clamp up to 1).

That seems like a noteworthy special case, assuming it's intentional.  Could you add a clarification to explain that?

@@ +875,5 @@
> +    bool sameDirectionAsThis = true;
> +    uint32_t min = mClampMinLine < 0 ? 1 : mClampMinLine;
> +    uint32_t max = mClampMaxLine;
> +    for (auto* map = this; true; ) {
> +      int32_t line = map->FindNamedArea(aName, aSide, min, max);

FindNamedArea returns a uint32_t, not an int32_t....  And it looks like we end up casting |line| back to a uint32_t a few lines below this, when we append it to aImplicitLines.

So: perhaps |line| should have type uint32_t then, since that's how it begins and ends its life?

@@ +891,5 @@
> +          // Remove duplicates and sort in ascending order.
> +          aImplicitLines.Sort();
> +          uint32_t prev = 0;
> +          for (size_t i = 0; i < aImplicitLines.Length(); ++i) {
> +            uint32_t line = aImplicitLines[i];

This |line| variable (of type uint32_t) seems to shadow the variable |line| (of type int32_t) that is declared in the parent-scope (in the outer "for" loop).

Probably best not to reuse that name & to avoid variable-shadowing, to help us keep track of which variable is which.

@@ +893,5 @@
> +          uint32_t prev = 0;
> +          for (size_t i = 0; i < aImplicitLines.Length(); ++i) {
> +            uint32_t line = aImplicitLines[i];
> +            if (line == prev) {
> +              aImplicitLines.UnorderedRemoveElementAt(i);

This algorithm for removing duplicates doesn't work (not yet) in a few cases, I think.

Specifically: what if there are duplicates in the middle and end of the array?  We'll find the middle ones first, and UnorderedRemoveElementAt will stomp on them by swapping in elements from the end of the array -- but then we'll never examine those (swapped) elements from the end of the array, and we'll never discover whether they were duplicates.

I think you can avoid this problem by iterating in reverse direction.  Due to the way that UnorderedRemoveElementAt works (always swapping from the end), you'll be fine if you ensure that the elements at the end (beyond "i") are already known-unique.  Or there might be another workaround, too... that's just what comes to mind first.

(If it's possible to add a testcase that exercises this pathological case, that'd probably be a good idea.)

@@ +909,5 @@
> +      }
> +      min = map->TranslateToParentMap(min);
> +      max = map->TranslateToParentMap(max);
> +      if (min > max) {
> +        mozilla::Swap(min, max);

I don't have a good mental model for when min > max might be true here (and why we need to handle it here but not up above).  Is this something that might only happen when we're swapping directions?  A comment would be helpful here (and/or maybe even an assertion to validate/document that this min>max mismatch only happens under certain conditions.)

@@ +919,5 @@
> +  /**
> +   * Return true if any implicit named areas match aName, in this map or
> +   * in any of our ancestor maps.
> +   */
> +  bool HasImplicitNamedArea(const nsString& aName) const

Side note (no action needed here, but possibly fodder for a followup):
Is there a reason this function (and its supporting functions Contains and HasNameAt from part 3) use "const nsString&" rather than "const nsAString&" for their arg type?

Since the arg is read-only, I'd think we should match the common practice of having the arg be the abstract string type nsAString&, to avoid accidentally depending on implementation details for nsString (and to allow different sorts of strings to be passed in)

Comment 12

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Comment on attachment 8982507 [details] [diff] [review]
> part 3 - [css-grid-2] Implement nested line name lookups for subgrids
> Is aState.mFrame->GetParent() really the right thing here?
> 
> In particular: if aState.mFrame is a subgrid which happens to be
> overflow:scroll (or have some other anonymous wrapper around us), won't this
> give us the WM for the intermediate wrapper frame, rather than WM for the
> the outer grid?  (because aState.mFrame->GetParent() would be the anonymous
> box, I think...)

Ah, good catch.  I already have a part for bug 1466358 that implements
a convenience method for that, so I'll move that here instead...

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid-2] Add a convenience method to get the parent GridContainerFrame for a subgrid

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid-2] Implement nested line name lookups for subgrids

(no need to re-review - just using ParentGridContainerForSubgrid()
instead of GetParent() to address the review comment)

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid-2] Implement nested area name lookups for subgrids

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Comment on attachment 8982517 [details] [diff] [review]
> part 4 - [css-grid-2] Implement nested area name lookups for subgrids
> s/too./too (within this subgrid's spanned area)./
> 
> Otherwise, with the wording here right now, it sounds like this is saying we
> search the *full grid* of all ancestor grids. But I don't think that's the
> case (right?)

Right, all line name lookups are within the subgrid's spanned area.

> > +    uint32_t min = mClampMinLine < 0 ? 1 : mClampMinLine;
> 
> It looks like negative mClampMinLine values get clamped up to 1, but 0 gets
> to stay as 0 (it doesn't clamp up to 1).
> 
> That seems like a noteworthy special case, assuming it's intentional.  Could
> you add a clarification to explain that?

Only non-subgrid LineNameMaps have mClampMinLine < 0 (i.e. kMinLine),
and using 1 instead in that case simplifies the logic in the rest of
the method.  This is ok b/c lines from 'grid-template-areas' are
always in the explicit grid (>= 1).  I changed it to:
    uint32_t min = !mParentLineNameMap ? 1 : mClampMinLine;
instead, which is equivalent and clearer, hopefully.

> > +      int32_t line = map->FindNamedArea(aName, aSide, min, max);
> 
> FindNamedArea returns a uint32_t, not an int32_t....  And it looks like we
> end up casting |line| back to a uint32_t a few lines below this, when we
> append it to aImplicitLines.
> 
> So: perhaps |line| should have type uint32_t then, since that's how it
> begins and ends its life?

Sure.

> > +            uint32_t line = aImplicitLines[i];
> 
> Probably best not to reuse that name & to avoid variable-shadowing, to help
> us keep track of which variable is which.

OK.

> This algorithm for removing duplicates doesn't work (not yet) in a few
> cases, I think.

Oops, you're right.  I simplified it to lookahead for dupes and then
RemoveElementsAt that range as needed.  Duplicates here are expected
to be rare, and the array small, so it doesn't really matter much
for performance anyway.

> > +      if (min > max) {
> > +        mozilla::Swap(min, max);
> 
> Is this something that
> might only happen when we're swapping directions?

Yes.  I added an assertion about that.

> > +  bool HasImplicitNamedArea(const nsString& aName) const
> 
> Side note (no action needed here, but possibly fodder for a followup):
> Is there a reason this function (and its supporting functions Contains and
> HasNameAt from part 3) use "const nsString&" rather than "const nsAString&"
> for their arg type?
> 
> Since the arg is read-only, I'd think we should match the common practice of
> having the arg be the abstract string type nsAString&, to avoid accidentally
> depending on implementation details for nsString (and to allow different
> sorts of strings to be passed in)

I don't think we should use an unnecessarily abstract type to handle
hypothetical cases that we don't currently need.  This isn't a public
API surface where we have no control over the consumers.
All these strings comes from style data: mGridTemplateAreas,
mLineNameLists etc, which are nsString; or nsAutoString that we
constructed on the stack in a few cases (to add "-start/end" to
an area name).  So I'd prefer nsString until we actually need to
handle other types of strings (I hope not).

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8984775 [details] [diff] [review]
part 3 - [css-grid-2] Add a convenience method to get the parent GridContainerFrame for a subgrid

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +5922,5 @@
> +{
> +  MOZ_ASSERT(IsSubgrid());
> +  nsIFrame* p = GetParent();
> +  while (p->GetContent() == GetContent()) {
> +    p = p->GetParent();

I don't think this will do the right thing, if we have a grid with a ::before/::after child that happens to be a subgrid.

In that scenario, I'm pretty sure that the ::before/::after child's GetContent() will return the content node for its parent node (the closest "real" content node).  So when we walk to the parent grid-frame in this loop, we'll keep iterating for longer than you'd like -- we'll iterate beyond the parent grid, because its GetContent() will match the subgrid's GetContent()... Right?

Comment 17

[Mats Palmgren (inactive)]

No, it's the pseudo-element.
Here's what a slightly edited frame tree looks like with a ::before
that is a subgrid:
GidContainer(div) [content=7f5e03b2e780] [sc=7f5e0a52be08]<
  GridContainer(_moz_generated_content_before) [content=7f5e03b2ef00] [sc=7f5e0a52bf08:before]<
    Block(_moz_generated_content_before) [content=7f5e03b2ef00] [sc=7f5e0a52c908:-moz-anonymous-grid-item]<
      line 7f5e03b33db8: ... <
        Text(0)"::before" [content=7f5e04087a00] [sc=7f5e0a52c808:-moz-text]

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8984775 [details] [diff] [review]
part 3 - [css-grid-2] Add a convenience method to get the parent GridContainerFrame for a subgrid

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

Hmm, OK - I'm getting anonymous box behavior and pseudo-element behavior mixed up.  r=me then

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8984777 [details] [diff] [review]
part 5 - [css-grid-2] Implement nested area name lookups for subgrids

r=me

Comment 20

[Daniel Holbert [:dholbert]]

(Is it possible to include tests to exercise this code at this point? This is all r+, but I'm a little uneasy landing this without tests.)

Comment 21

[Mats Palmgren (inactive)]

I don't think it's meaningful to land subgrid tests without the sizing
patches in bug 1466358.  Hopefully we can get that landed soon enough.
I think I'll hold these patches for now, since I don't know how robust
grid layout is without the other stuff.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:mats, could you have a look please?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e44c9fcc9a4
part 1 - [css-grid-2] Implement subgrid item placement.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/386b8a935d2b
part 2 - [css-grid-2] Clamp lines in a subgrid to its extent.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b249a6ae76a
part 3 - [css-grid-2] Add a convenience method to get the parent GridContainerFrame for a subgrid.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/67fb1683b89c
part 4 - [css-grid-2] Implement nested line name lookups for subgrids.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f82446806fa
part 5 - [css-grid-2] Implement nested area name lookups for subgrids.  r=dholbert

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6e44c9fcc9a4
https://hg.mozilla.org/mozilla-central/rev/386b8a935d2b
https://hg.mozilla.org/mozilla-central/rev/6b249a6ae76a
https://hg.mozilla.org/mozilla-central/rev/67fb1683b89c
https://hg.mozilla.org/mozilla-central/rev/1f82446806fa