978478 - Support repeat() in CSS Grid templates

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Simon Sapin (:SimonSapin)]

Bug 976787 adds style system support for the grid-template-columns and grid-template-rows properties, but does not yet include support for the repeat() function that the spec allows there.

Comment 1

[Simon Sapin (:SimonSapin)]

The easiest implementation seems to expand repeat() into a repeated <track-list> when parsing. However, this could be an attack vector where a small stylesheet like `*{grid-template-columns: repeat(999999999, 20px)}` could cause high memory usage.

It may be worth limiting the amount of repetitions, similar to how the CSS Counter Style spec allows limiting the length of counter representations

http://dev.w3.org/csswg/css-counter-styles/#counter-styles
> Some values of system (symbolic, additive) and some descriptors (pad) can
> generate representations with size linear to an author-supplied number.
> This can potentially be abused to generate excessively large representations
> and consume undue amounts of the user’s memory or even hang their browser.
> User agents must support representations at least 60 Unicode codepoints long,
> but they may choose to instead use the fallback style for representations
> that would be longer than 60 codepoints.

Comment 2

[Mats Palmgren (inactive)]

FYI, we limit 'column-count' to 1000:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#7739

Comment 3

[Daniel Holbert [:dholbert]]

That seems like a reasonable restriction, though I'm not sure 1000 is quite large enough. I can imagine there might end up being demos that use grid in crazy ways with one pixel per grid-cell, which would run up against that limit, since monitors are generally ~1000-2000 pixels wide in each dimension.  

Regardless of the wisdom of writing content like that :) it seems unfortunate that we'd *barely* miss out on parsing the markup correctly, with a limit of 1000.

But I think a limit in that ballpark would be reasonable. Maybe 5,000 or 10,000?

Comment 4

[Simon Sapin (:SimonSapin)]

I’ll go with parse-time expansion, clamping the number of repetitions to 10,000 or so.

Comment 5

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1: repeat() in subgrid

Patch 2 will add repeat() in <track-list>, which is more tricky because of <line-names> lists splicing.

Comment 6

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1 v2: repeat() in subgrid

Clamp to 10,000 repetitions, per comment 3.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: gzipped pathological testcase (WARNING: hangs browser and allocates lots of memory, w/ grid preffed on)

FWIW, I tried to exercise this to see how much I could mess up my memory usage. Here's a gzipped copy of the testcase that ultimately hanged my browser and made it slowly grow memory usage indefinitely. (I broke in with gdb after a few minutes, to poke around and see what was going on, when it was at 20% of CPU usage.)

The testcase is basically just a div with style="grid-template: subgrid / subgrid (foo) repeat(999999999, (aaaaaaaa bbbbbbbbbbbbb cccccccccccccc dddddddddddddd eeeeeeeeeeeee ffffffffffffffffff jjjjjjjjjjjjjjjjjj kkkkkkkkkkkkkkkkk lllllllllllll [etc, continuing until the file hits ~1.6M]"

We seem to parse this (and expand the repeat()) just fine, but then when we convert it into computed style, we get reeeeeeeeeeeeeeeallllly slooooow performance, apparently because nsTArray::AppendElement (and in particular EnsureCapacity()) only uses doubling behavior until it surpasses a page-size -- and from then on, it just reallocs with n+1 pages.  So we end up allocating 1 page, then 2 pages, then 3 pages, etc., all the way up to the number of pages we ultimately need:
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray-inl.h?rev=3bee396bb681&mark=130-141#130

Maybe this is good; it gets us better memory-usage for reasonable testcases, and it makes pathological testcases hang & slowly eat up memory, rather than eating up memory more quickly...

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> FWIW, I tried to exercise this to see how much I could mess up my memory
> usage. Here's a gzipped copy of the testcase that ultimately hanged my
> browser and made it slowly grow memory usage indefinitely. (I broke in with
> gdb after a few minutes, to poke around and see what was going on, when it
> was at 20% of CPU usage.)

(Sorry, I meant 20% of _memory_ usage. It was 100% CPU usage, doing the repeated array-realloc'ing described above.)

(The high memory usage also makes me think this might be defeating our string sharing code right now. Currently investigating with gdb to find out more.)

Comment 9

[Daniel Holbert [:dholbert]]

(This is on a machine with 16GB ram, so that 20% memory usage works out to 3.2 GB.)

Comment 10

[Simon Sapin (:SimonSapin)]

I have some ideas on how to reducing memory usage and copying, but I don’t know how much such pathological cases are worth optimizing for.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: smaller pathological testcase (hangs my debug build for 10s, ends up using ~650 MB)

Here's a smaller version of the pathological testcase, which hangs my debug build for ~10 seconds and makes us allocate about 650 MB.

From watching this testcase load in GDB and in sysprof, I'm not noticing any string-copying, so I think we're OK as far as string-sharing goes. We seem to be spending most of our time reallocating nsTArrays (which blows up, as noted towards the end of comment 7) and addreffing / releasing string buffers (which we have to do for all of the strings every time we copy an old nsTArray into a freshly-allocated one). (A good chunk of the addref/release time is spent in logging code, too, which presumably is debug-only.)

So, this is probably OK, perf-wise. (& maybe we can improve things before preffing it on.)

Comment 12

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1 v3: repeat() in subgrid

Rebase on top of the rename in patch 983175

Comment 13

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2: repeat() in <track-list>

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8398094 [details] [diff] [review]
Patch 1 v3: repeat() in subgrid

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
> // Length of the "var-" prefix of custom property names.
> #define VAR_PREFIX_LENGTH 4
> 
>+#define GRID_TEMPLATE_MAX_REPETITIONS 10000
>+
> // End-of-array marker for mask arguments to ParseBitmaskValues
> #define MASK_END_VALUE  (-1)

Add a brief comment for this new #define, like we have for the others there.

(In particular, the code seems to assume that it's > 0, per my mention below about an assertion. That'd be worth documenting here.)

>+// Assuming the 'repeat(' function token has already been consumed,
>+// parse the rest of repeat(<positive-integer>, <line-names>+)
>+bool
>+CSSParserImpl::ParseSubgridRepeat(nsCSSValueList** aItem)
>+{

|aItem| is a bit confusing -- after some code-reading, it looks like it's a pointer to the current tail of the nsCSSValueList, and is a double-pointer so that we can update that tail and let the caller know about it. (?)  Please document it, in any case, and consider renaming it to "aTailPtr" or something clearer like that.

Also, two things about the function-name:
 (1) I'd suggest s/Repeat/RepeatBody/ (to make it clearer that this is called _after_ we've parsed "repeat(")

 (2) Not sure "Subgrid" should be in the name. This isn't so much for "subgrid" as it is for <line-name-list>. (As it happens, <line-name-list> only ever occurs after 'subgrid', but they're still separate parts of the grammar.)

So: Maybe rename to "ParseGridLineNameListRepeat[Body]"?

(Note the similarity in naming to "ParseGridTrackListRepeat", added in your next patch here.)

>+  int32_t repetitions = std::min(mToken.mInteger,
>+                                 GRID_TEMPLATE_MAX_REPETITIONS);

Assert that repetitions is > 0, to prove that the "while (--repetitions)" loop below is sane.

(Your sanity-checks on mToken already mostly-ensure this, but if someone sets a bogus value of GRID_TEMPLATE_MAX_REPETITIONS (e.g. to 0 or -1, maybe as an attempt to disable the maximum or enforce a strict cap), it'll cause some serious trouble for that loop.)

>+  // Parse at least one <line-names>
>+  nsCSSValueList* item = *aItem;
>+  do {
>+    item->mNext = new nsCSSValueList;
>+    item = item->mNext;
>+    if (ParseGridLineNames(item->mValue) != CSSParseResult::Ok) {
>+      SkipUntil(')');
>+      return false;
>+    }
>+  } while (!ExpectSymbol(')', true));
[...]
>+  *aItem = item;
>+  return true;

Similar to note above for "aItem/aTailPtr", I think "item" here would be clearer if it were named "tail".

e.g. I think this is clearer as:
  nsCSSValueList* tail = *aTailPtr;
  do {
    tail->mNext = new nsCSSValueList;
    tail = tail->mNext;
[...etc....]
  *aTailPtr = tail;
  return true;


> bool
> CSSParserImpl::ParseOptionalLineNameListAfterSubgrid(nsCSSValue& aValue)
> {
>   for (;;) {
>+    // First try to parse repeat(<positive-integer>, <line-names>+)
>+    if (!GetToken(true)) {
>+      return true;
>+    }
>+    if (mToken.mType == eCSSToken_Function &&
>+        mToken.mIdent.LowerCaseEqualsLiteral("repeat")) {
>+      if (!ParseSubgridRepeat(&item)) {
>+        return false;
>+      }
>+      continue;
>+    }
>+    UngetToken();
>+
>+    // This was not a repeat() function. Try to parse <line-names>.
>     nsCSSValue lineNames;

Replace "continue;}UngetToken()" with "else { UngetToken();" (and reindent the stuff after that)

("continue" is a bit confusing for control flow; nice not to rely on it unless we really have to.)


>@@ -7448,17 +7515,16 @@ CSSParserImpl::ParseGridTemplateAfterString(const nsCSSValue& aFirstLineNames)
> 
>   for (;;) {
>     if (!ParseGridTemplateAreasLine(mToken.mIdent, areas, areaIndices)) {
>       return false;
>     }
> 
>     rowsItem->mNext = new nsCSSValueList;
>     rowsItem = rowsItem->mNext;
>-    // TODO: add repeat()
>     CSSParseResult result = ParseGridTrackSize(rowsItem->mValue);

Nit: Technically this belongs in the next patch, but not a big deal if you don't want to bother moving it.

>+++ b/layout/style/test/property_database.js
>@@ -4909,16 +4911,25 @@ if (SpecialPowers.getBoolPref("layout.css.grid.enabled")) {
>+			"subgrid repeat(0, ())",
>+			"subgrid repeat(-3, ())",

Include an otherwise-valid entry with "1.0" or "2.0" here, too.

>+    {
>+        // https://bugzilla.mozilla.org/show_bug.cgi?id=978478#c1
>+        // The number of repetitions is clamped to
>+        // #define GRID_TEMPLATE_MAX_REPETITIONS 10000
>+        specified: "subgrid / subgrid (foo) repeat(999999999, (a))",
>+        gridTemplateColumns: "subgrid",
>+        gridTemplateRows: "subgrid (foo)" + Array(10000 + 1).join(" (a)"),
>+    },

(Note: I didn't get this at first, but now I see it's joining 10001 empty strings, with " (a)" between each empty-string. Hacky, but cool. :) I'd expect there'd be a more elegant way to do this, but from googling, it looks like the best alternative is " (a)".repeat(10000), and that may only work in Firefox.[1] So I guess what you've got is better for now. :)
 [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/repeat
)

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> >+bool
> >+CSSParserImpl::ParseSubgridRepeat(nsCSSValueList** aItem)
> >+{
[...]
> So: Maybe rename to "ParseGridLineNameListRepeat[Body]"?
> 
> (Note the similarity in naming to "ParseGridTrackListRepeat", added in your
> next patch here.)

(I don't love this suggested name; if you can come up w/ a better name that's consistent across the two types of repeat(...) covered in the two patches here, that's great too.)

Maybe ParseRepeatedGridLineNameList / ParseRepeatedGridTrackList?  (IMHO "Repeated" suggests more to me that we've already parsed the "repeat(" and now we're parsing the repeated stuff, and hence don't need a "Body" modifier)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8398096 [details] [diff] [review]
Patch 2: repeat() in <track-list>

Partial review of Part 2:
=========================

>From: Simon Sapin <[e-mail]>
>Bug 978478 part 2: Add repeat() in <track-list> (CSS Grid)

s/Add/Add support for parsing/

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
> bool
> CSSParserImpl::ParseGridTrackListWithFirstLineNames(nsCSSValue& aValue,
>                                                     const nsCSSValue& aFirstLineNames)
> {
[...]
>   for (;;) {

Add a comment before this loop, explaining the point of the loop (since it's a bit opaque).

e.g. something like:

 // This function is trying to parse <track-list>, which is
 //   [ <line-names>? [ <track-size> | <repeat()> ] ]+ <line-names>?
 // and we're already past the first "<line-names>?".
 //
 // Each iteration of the following loop attempts to parse either a
 // repeat() or a <track-size> expression, and then an (optional)
 // <line-names> expression.
 //
 // The only successful exit point from this loop is the ::NotFound
 // case after ParseGridTrackSize(); i.e. we'll greedily parse
 // repeat()/<track-size> until we can't find one.

>+      CSSParseResult result = ParseGridTrackSize(trackSize);
[...]
>+      if (result == CSSParseResult::NotFound) {
>+        break;
>+      }
>-    CSSParseResult result = ParseGridTrackSize(trackSize);
>-    if (result == CSSParseResult::NotFound) {
>-      // What we've parsed so far is a valid <track-list>. Stop here.
>       break;

Looks like this code is just being reindented, but you're also dropping the comment from this ::NotFound case. I'm guessing that comment-removal might've been unintentional? I think the comment's useful & worth keeping, as long as it's still accurate (or could be tweaked to remain accurate).

>+  // Require at least on <track-size>.

s/on/one/

>+bool
>+CSSParserImpl::ParseGridTrackListRepeat(nsCSSValueList** aItem)

As noted above:
 - consider renaming this function along with the one in the other patch
 - document what aItem is supposed to be & what it ends up as
 - and probably rename aItem to aTailPtr


>+  int32_t repetitions = std::min(mToken.mInteger,
>+                                 GRID_TEMPLATE_MAX_REPETITIONS);

As with patch 1, assert that 'repetitions' is > 0.

>+  // Parse [ <line-names>? <track-size> ]+ <line-names>?
>+  // but keep the first and last <line-names> separate
>+  // because theyâll need to be joined.

Fix quote character.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8398096 [details] [diff] [review]
Patch 2: repeat() in <track-list>

Second partial review of Part 2:
================================

>+  // Parse [ <line-names>? <track-size> ]+ <line-names>?
[...]
>+  nsCSSValue firstLineNames;
>+  nsCSSValue trackSize;
>+  nsCSSValue lastLineNames;
>+  // Optional
>+  if (ParseGridLineNames(firstLineNames) == CSSParseResult::Error) {

I think the code below this (the ParseGridLineNames, ParseGridTrackSize, & looped ParseGridLineNames + ParseGridTrackSize) could be condensed a bit, to at least fold the first ParseGridTrackSize() call into the loop.

In particular, it's easier if you structure this to parse a different (but equivalent) formulation:
  <line-names>? [<track-size> <line-names>?]+
(Note that the grouping square-braces are shifted, but I believe it's still equivalent.)

I'm imagining something like this (paraphrased a bit to remove early-returns / error-handling):

  // Parse first <line-names>
  ParseGridLineNames(firstLineNames);
  nsAutoPtr<nsCSSValueList> firstTrackSizeItemAuto(new nsCSSValueList);
  nsCSSValueList* item = firstTrackSizeItemAuto;

  // Parse <track-size> <line-names>? until we hit close-paren
  do {
    ParseGridTrackSize(trackSize);
    // [error handling goes here]
    item->mValue = trackSize;
    item->mNext = new nsCSSValueList;


    lastLineNames.reset();
    ParseGridLineNames(lastLineNames);
    // [error handling goes here]
    item->mValue = trackSize;
    item->mNext = new nsCSSValueList;

  } while (!ExpectSymbol(')'));

I'd prefer this because:
 - it has the loop-condition baked into the loop instead of needing a 3-line check+"break" clause halfway through
 - it saves us one ParseGridTrackSize() call and the eror
which makes it easier to follow IMO.

Comment 18

[Daniel Holbert [:dholbert]]

(er, that second "item->mValue = trackSize;" should've been " = lastLineNames", of course)

Comment 19

[Daniel Holbert [:dholbert]]

(Ah, I guess my suggested code isn't quite equivalent to your current patch's code, in that my version appends a copy of |lastLineNames| to our list earlier than your patch does, and before we've merged lastLineNames and firstLineNames.

I think we can still make that work reasonably cleanly, though; we can simply append |firstLineNames| to the the final |item| in our list (which is the list's copy of lastLineNames), and then treat that item as the joiner item when we're doing repeats, or something like that.)

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8398096 [details] [diff] [review]
Patch 2: repeat() in <track-list>

Third and final partial review of Part 2:
=========================================

>+  // Join our first <line-name> whith the one before repeat().
>+  // (a) repeat(1, (b) 20px) expands to (a b) 20px
>+  nsCSSValueList* previousItemBeforeRepeat = *aItem;
>+  if (firstLineNames.GetUnit() != eCSSUnit_Null) {

s/whith/with/
s/<line-name>/<line-names>/

Also: this chunk (20+ lines) seem like it could stand to be factored into a helper-function ("MergeLineNames"?), the interests of making the overall function (ParseGridTrackListRepeat) more grokable / readable. (It's a bit long right now.)

>+      nsCSSValueList* source = firstLineNames.GetListValue();
[...]
>+      // Copy the firt name thatâs in a nsCSSValueList_heap
>+      target->mNext = new nsCSSValueList;
>+      target = target->mNext;
>+      target->mValue = source->mValue;
>+      // Move the rest of the linked list.
>+      target->mNext = source->mNext;
>+      source->mNext = nullptr;

Comment nits:
- s/firt/first, and fix fancy-quote in "that's".
- Also, the mention of "nsCSSValueList_heap" here might be a little too specific... (nsCSSValueList_heap is an implementation detail of nsCSSValue).


NOTE: The list-merging pattern here feels a bit hacky, too... it's got an unnecessary-feeling nsCSSValue copy operation ("target->mValue = source->mValue", for the first entry in the source list), and it sort of makes assumptions about nsCSSValue internals (at least for the knowledge that we have to copy that first element). We *really* want to just steal the *whole* list from firstLineNames, but we can't because we know nsCSSValue will destroy the list when it gets reset, so we have to sever it at the first link (and copy the entry before that link).

I kind of want to suggest making |firstLineNames| a nsAutoPtr<nsCSSValueList> instead of a nsCSSValue in the first place, but that would probably require rewriting ParseGridLineNames() to take a nsCSSValueList** instead of a nsCSSValue... which seems like it could get messy.

So I think the existing pattern here is OK for now, but I wish we could do something cleaner for this list-merge... (maybe I'll try to do that eventually in a followup.)


>+  // No more early return after this.
>+  nsCSSValueList* firstTrackSizeItem = firstTrackSizeItemAuto.forget();
[...]
>+  previousItemBeforeRepeat->mNext = firstTrackSizeItem;

That comment's assumption ("no more early returns") isn't future-proof. :)

We don't have to rely on that assumption, though. Instead, just do this assignment directly:
  previousItemBeforeRepeat->mNext = firstTrackSizeItemAuto.forget();
which we *can* trust to be safe, but *not because* there are no more early returns. Rather, it's because we're transfering ownership directly to an object that we're trusting to manage the lifetime from here on out.

(Then below that, you can declare nsCSSValueList* firstTrackSizeItem = previousItemBeforeRepeat->mNext if you like, since you need to re-use that pointer later, during the repeat-expansion.)

Comment 21

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1 v4: repeat() in subgrid

(In reply to Daniel Holbert [:dholbert] from comment #14)
> >+CSSParserImpl::ParseSubgridRepeat(nsCSSValueList** aItem)
>
>  (1) I'd suggest s/Repeat/RepeatBody/ (to make it clearer that this is
> called _after_ we've parsed "repeat(")
> 
>  (2) Not sure "Subgrid" should be in the name. This isn't so much for
> "subgrid" as it is for <line-name-list>. (As it happens, <line-name-list>
> only ever occurs after 'subgrid', but they're still separate parts of the
> grammar.)
> 
> So: Maybe rename to "ParseGridLineNameListRepeat[Body]"?

I went with ParseGridLineNameListRepeat. As to including "Body", I don’t think that much precision is really helpful. "Parse" is not quite exact either, since we’re also doing the expansion/repetition. But I think this is good enough.

Comment 22

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v2: repeat() in <track-list>

(In reply to Daniel Holbert [:dholbert] from comment #19)
> (Ah, I guess my suggested code isn't quite equivalent to your current
> patch's code, in that my version appends a copy of |lastLineNames| to our
> list earlier than your patch does, and before we've merged lastLineNames and
> firstLineNames.
> 
> I think we can still make that work reasonably cleanly, though; we can
> simply append |firstLineNames| to the the final |item| in our list (which is
> the list's copy of lastLineNames), and then treat that item as the joiner
> item when we're doing repeats, or something like that.)

I spent a bunch of time trying to do this before giving up. Doing this right (regarding to ownership of the various bits of linked lists) required enough code that the overall complexity didn’t seem to improve. Also, I think the way I approached this was doing more copying (or going through lists to find their end) than before.

Right now I can’t find a way to organize this that’s significantly better (and correct). But feel free to try.


(In reply to Daniel Holbert [:dholbert] from comment #20)
> NOTE: The list-merging pattern here feels a bit hacky, too... it's got an
> unnecessary-feeling nsCSSValue copy operation ("target->mValue =
> source->mValue", for the first entry in the source list), and it sort of
> makes assumptions about nsCSSValue internals (at least for the knowledge
> that we have to copy that first element). We *really* want to just steal the
> *whole* list from firstLineNames, but we can't because we know nsCSSValue
> will destroy the list when it gets reset, so we have to sever it at the
> first link (and copy the entry before that link).
> 
> I kind of want to suggest making |firstLineNames| a
> nsAutoPtr<nsCSSValueList> instead of a nsCSSValue in the first place, but
> that would probably require rewriting ParseGridLineNames() to take a
> nsCSSValueList** instead of a nsCSSValue... which seems like it could get
> messy.
> 
> So I think the existing pattern here is OK for now, but I wish we could do
> something cleaner for this list-merge... (maybe I'll try to do that
> eventually in a followup.)

The root cause that makes this (and a lot of other code) more complex than it should be is that our representation of a empty linked list is very different from that of a non empty one. It needs to be special-cased everywhere.

I think nsCSSValueList** wouldn’t work, for this reason. As soon as you have a nsCSSValueList, you assume at least one element.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8398491 [details] [diff] [review]
Patch 1 v4: repeat() in subgrid

>From: Simon Sapin <[e-mail]>
>Bug 978478 part 1: Add repeat() in <line-name-list> (CSS Grid)

As with patch 2, s/Add repeat()/Add support for repeat()/

(In reply to Daniel Holbert [:dholbert] from comment #14)
> >+++ b/layout/style/test/property_database.js
> >@@ -4909,16 +4911,25 @@ if (SpecialPowers.getBoolPref("layout.css.grid.enabled")) {
> >+			"subgrid repeat(0, ())",
> >+			"subgrid repeat(-3, ())",
> 
> Include an otherwise-valid entry with "1.0" or "2.0" here, too.

Looks like this ^ still needs addressing. (I don't see any N.0 values in the property_database.js additions in latest patch)

r=me on part 1 with that. Thanks!

Comment 24

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1 v5: repeat() in subgrid. r=dholbert

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Simon Sapin (:SimonSapin) from comment #22)
> I spent a bunch of time trying to do this before giving up. Doing this right
> (regarding to ownership of the various bits of linked lists) required enough
> code that the overall complexity didn’t seem to improve. Also, I think the
> way I approached this was doing more copying (or going through lists to find
> their end) than before.

Darn -- sorry about that. I'm willing to believe that the ownership might get a bit more complex (and as a result) foil the code-simplification.

In any case, this suggestion was in an effort to make this function shorter / more grokable, and it looks like the (separate) ConcatLineNames refactoring helped a lot with that.

Comment 26

[Simon Sapin (:SimonSapin)]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Darn -- sorry about that. I'm willing to believe that the ownership might
> get a bit more complex (and as a result) foil the code-simplification.
> 
> In any case, this suggestion was in an effort to make this function shorter
> / more grokable, and it looks like the (separate) ConcatLineNames
> refactoring helped a lot with that.

It may just have been me starting in the wrong direction, and getting confused as a result. I believe it’s possible to simplify a bit more, but it’s not obvious how.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8398586 [details] [diff] [review]
Patch 2 v2: repeat() in <track-list>

>+// Assuming the 'repeat(' function token has already been consumed,
>+// parse the rest of
>+// repeat( <positive-integer> ,
>+//         [ <line-names>? <track-size> ]+ <line-names>? )
>+bool
>+CSSParserImpl::ParseGridTrackListRepeat(nsCSSValueList** aTailPtr)

As with Part 1, document what |aTailPtr| is for. (You can probably largely copypaste the documentation you added in part 1, for the other Parse*Repeat function)

>+++ b/layout/style/test/property_database.js
>@@ -4872,16 +4872,24 @@ if (SpecialPowers.getBoolPref("layout.css.grid.enabled")) {
>@@ -4908,16 +4916,25 @@ if (SpecialPowers.getBoolPref("layout.css.grid.enabled")) {
>+			"repeat(0, 20px)",
>+			"repeat(-3, 20px)",

As noted in IRC, please add a "1.0" or "2.0" repetition testcase here, too.

With that, r=me. Thanks!

Comment 28

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v3: repeat() in <track-list>. r=dholbert

https://tbpl.mozilla.org/?tree=Try&rev=eaf747090653

Comment 29

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c83c52c332be
https://hg.mozilla.org/integration/mozilla-inbound/rev/9051a06f0268

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3476de5a680
https://hg.mozilla.org/mozilla-central/rev/f5c5742ad004

Comment 31

[Firefox Commit Bot]

The following changesets are now in Firefox Nightly:

> f3476de5a680 Bug 978478 part 1: Add support for repeat() in <line-name-list> (CSS Grid) r=dholbert
> f5c5742ad004 Bug 978478 part 2: Add support for repeat() in <track-list> (CSS Grid) r=dholbert

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

Comment 32

[Merih Akar]

Is grid layout included in latest nightly? I cannot find any flag in about:config related to grid.

Comment 33

[Simon Sapin (:SimonSapin)]

(In reply to merihakar from comment #32)
> Is grid layout included in latest nightly? I cannot find any flag in
> about:config related to grid.

Nightly currently has this and a few other patches that are preliminary steps for CSS Grid, but there is no actual layout implemented at this point. The pref is not visible in about:config because it doesn’t do anything useful yet.