I pushed this to Try just to double check it still passes: https://tbpl.mozilla.org/?tree=Try&rev=8caea49ffa5c

Comment on attachment 8411263 [details] [diff] [review] fix This looks like a patch for bug 1000431 -- did you mean to post it over there?

Comment on attachment 8411263 [details] [diff] [review] fix Oops, sorry, wrong tab. :-)

No worries. :) Having said that, here are my nits on https://hg.mozilla.org/try/rev/0b5ba57a4f41 from your try push (which does seem to be for this bug): > - // the flexbox spec. I'm not bothering with that at this point, since it's > + // the CSS spec. I'm not bothering with that at this point, since it's "the CSS spec" implies CSS2.1 to me, and I don't think we want to be giving that implication here. Maybe instead, say "the CSS grid & flexbox specs". (You may need to rewrap this line to fit that text, but it's not a huge deal, since the comment ends after 1 more line.) > // Now |iter| points to the first child that needs to be wrapped in an > - // anonymous flex item. Now we see how many children after it also want > + // anonymous flex/grid item. Now we see how many children after it also want > // to be wrapped in an anonymous flex item. I think that edited line might end up being > 80 chars. Also, the final line needs some "flex"-->"flex/grid" treatment. > "(this will make a difference when we encounter " > - "'flex-align: baseline')"); > + "'flex/grid-align: baseline')"); Oops! "flex-align" is an old property-name. It's now been renamed to "align-items", which works both for flexbox and grid. So, s/flex-align/align-items/ here (and no need to add any mention of grid).) > - // an anonymous flex item. That's where it's already going - good! > + // an anonymous flex or grid item. That's where it's already going - good! I think this is over 80 chars. > - // Indicates whether (when in a flexbox container) this item needs to be > + // Indicates whether (when in a flexbox or grid container) this item needs s/flexbox/flex/ (this was left over from an old version of the spec which used the term "flexbox container", IIRC)

Comment on attachment 8411368 [details] [diff] [review] part 1, Add -moz-anonymous-grid-item in the style system. ># HG changeset patch ># Parent a545cbf02733b8463f4f191f3bbd530ca6fa3d40 ># User Mats Palmgren <matspal@gmail.com> >Bug 1000376 - part 1, Add -moz-anonymous-grid-item in the style system. r=dholbert > >diff --git a/layout/style/nsCSSAnonBoxList.h b/layout/style/nsCSSAnonBoxList.h >--- a/layout/style/nsCSSAnonBoxList.h >+++ b/layout/style/nsCSSAnonBoxList.h >@@ -61,16 +61,20 @@ CSS_ANON_BOX(scrolledPageSequence, ":-mo > CSS_ANON_BOX(columnContent, ":-moz-column-content") > CSS_ANON_BOX(viewport, ":-moz-viewport") > CSS_ANON_BOX(viewportScroll, ":-moz-viewport-scroll") > > // Inside a flex container, a contiguous run of non-replaced inline children > // gets wrapped in an anonymous block, which is then treated as a flex item. > CSS_ANON_BOX(anonymousFlexItem, ":-moz-anonymous-flex-item") > >+// Inside a grid container, a contiguous run of non-replaced inline children >+// gets wrapped in an anonymous block, which is then treated as a grid item. >+CSS_ANON_BOX(anonymousGridItem, ":-moz-anonymous-grid-item") The "contiguous run of non-replaced inline children" language here is out of date. (It predates the ApplyStyleFixups stuff that promotes inline-level child elements to be block-level.) It looks like the flexbox & grid ED specs both currently say: # ...each contiguous run of text that is directly # contained inside a [flex|grid] container is # wrapped in an anonymous [flex|grid] item. http://dev.w3.org/csswg/css-flexbox/#flex-items http://dev.w3.org/csswg/css-grid/#grid-items Could you replace "non-replaced inline children" with "text" here, to match the current state of these specs? >+++ b/layout/style/ua.css >-*|*::-moz-anonymous-flex-item { >+*|*::-moz-anonymous-flex-item, >+*|*::-moz-anonymous-grid-item { > /* Anonymous blocks that wrap contiguous runs of inline non-replaced >- * content inside of a flex container. */ >+ * content inside of a flex or grid container. */ > display: block; Same here. (s/inline non-replaced content/text/) Thanks! r=me with that

Turns out I missed a couple of places that used IsAnonymousFlexItem in my first Try push, so it's evidently error prone to keep that. I've replaced it IsAnonymousFlexOrGridItem and removed the remaining uses (only in asserts). All anon-flex-item checks in non-asserts are now using "FlexOrGridItem". I fixed the nits in comment 4, except the long lines you pointed out were 80 and 79 respectively so I kept those. I've verified in the upcoming tests that frame dumps are exactly the same as the corresponding flex test with s/flex/grid/ so it seems to be working.

These are just copies of a few flexbox tests with s/flex/grid/. I figured it would be nice to have them to detect assertions/crashes for now. https://tbpl.mozilla.org/?tree=Try&rev=48545f8d1521

Fixed the comments per comment 6.

Comment on attachment 8411377 [details] [diff] [review] part 2, Implement frame construction part for anonymous grid items (reusing the anon flex item code). >+++ b/layout/base/nsCSSFrameConstructor.cpp >+#if DEBUG >+static void >+AssertFlexOrGridItemParent(const nsIFrame* aChild, const nsIFrame* aParent) >+{ >+ MOZ_ASSERT(IsAnonymousFlexOrGridItem(aChild), >+ "expected an anonymous flex or grid item child frame"); [...] >+} >+#else >+#define AssertFlexOrGridItemParent(x, y) do { /* nothing */ } while(0) >+#endif This function/macro's name should probably mention "Anon", since it only applies to *anonymous* flex/grid items (based on its first assertion, quoted above). (I'm mentioning this because we *do* have special-case code elsewhere that applies to all flex items -- not just anonymous ones -- as a MXR search for "IsFlexItem" will show. So, it's worth distinguishing that this function isn't in that category; it's *only* for anonymous items.) So: maybe rename to "AssertAnonItemHasRightParent"? Or "AssertAnonItemMatchesParentType"? r=me with something like that.

Comment on attachment 8411379 [details] [diff] [review] part 3, s/FlexItemStyleFixup/FlexOrGridItemStyleFixup/ and corresponding comment changes. >diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp >- TreeMatchContext::AutoFlexItemStyleFixupSkipper >- flexFixupSkipper(mTreeMatchContext, >+ TreeMatchContext::AutoFlexOrGridItemStyleFixupSkipper >+ flexOrGridFixupSkipper(mTreeMatchContext, > element->IsRootOfNativeAnonymousSubtree()); Reindent that last line, so that element-> stays lined up with mTreeMatchContext. >diff --git a/layout/style/nsStyleSet.h b/layout/style/nsStyleSet.h >- // Indicates that we should skip the flex-item-specific chunk of >+ // Indicates that we should skip the flex- or grid-item-specific chunk of > // ApplyStyleFixups(). IMO, the current "flex **OR** grid item" language (in comments like the one above & in variable/type names) is a bit confusing. It sounds to me like we're going to skip *either* some flex-specific code, *or* we're going to skip some different grid-specific code. But really this is all about skipping one single chunk of code, which is common to both types of items. In particular, the comment quoted above seems to suggest that there are two separate chunks, and we're skipping one or the other. (The variable/type names with "or" give me the same impression). I think this would make more sense with "and" instead of "or" in this comment & in the various the type/variable names. (The idea being, "flex and grid item fixup" is a single process, which we do for flex items *and* grid items). But if you prefer "Or", I'm fine with that too. So, r=me either way.

Comment on attachment 8411386 [details] [diff] [review] part 4, Add some grid item tests, but only "load" them for now. (In reply to Mats Palmgren (:mats) from comment #9) > Created attachment 8411386 [details] [diff] [review] > part 4, Add some grid item tests, but only "load" them for now. > > These are just copies of a few flexbox tests with s/flex/grid/. > I figured it would be nice to have them to detect assertions/crashes > for now. Sounds good. But: since the files in this patch are all direct copies of existing test files with a minimal tweak (or with no tweaks in the case of ahem.css & bluesquare), they should be generated using 'hg cp' instead of being added as new-files-created-from-scratch. Could you change the patch to do that? I don't think it should be too much work -- you can just backup your added test files, then 'hg qpop' the patch, and then initialize a new patch with 'hg qnew', and run 'hg cp flextestfile gridtestfile' for each test file, and then copy your backed-up versions to stomp on your freshly-hg-cp'd files, and then qrefresh the patch. This will make for saner hg history, and an easier-to-audit patch that just shows the actual differences between the old & new files. >diff --git a/layout/reftests/css-grid/reftest.list b/layout/reftests/css-grid/reftest.list >new file mode 100644 >--- /dev/null >+++ b/layout/reftests/css-grid/reftest.list >@@ -0,0 +1,6 @@ >+default-preferences pref(layout.css.grid.enabled,true) >+ >+# just "load" for now since grid layout is not implemented yet >+load grid-whitespace-handling-1a.xhtml >+load grid-whitespace-handling-1b.xhtml >+load grid-whitespace-handling-2.xhtml "load" is definitely better than nothing, but it leaves the reference cases completely unused [so we're adding them for no reason?], and it als won't poke us if we start passing the tests (in case we forget to update this manifest at that point). And it also feels a bit crashtest-flavored, for a reftest manifest. So, I think I'd prefer that we list these as: fails == grid-whitespace-handling-1a.xhtml grid-whitespace-handling-1-ref.xhtml etc. That addresses the concerns I mentioned above (it'll make use of the reference cases, and it'll notifying us if/when we start passing). It also should still turn the tree orange if we assert or crash, too. (IIRC those sorts of issues are not included in "fails" annotations.) (Note: I'm not sure the reference cases are even correct, but we can fix them & the tests as-necessary as we implement grid layout I suppose.)

s/AssertFlexOrGridItemParent/AssertAnonymousFlexOrGridItemParent/

(In reply to Daniel Holbert [:dholbert] from comment #12) > Reindent that last line, so that element-> stays lined up with > mTreeMatchContext. Fixed. > >+ // Indicates that we should skip the flex- or grid-item-specific chunk of > In particular, the comment quoted above seems to suggest that there are two > separate chunks, and we're skipping one or the other. This was the only comment that used "or" - I've changed it to "flex/grid" as the other comments. I think it's clear that these comments refers to a shared piece of code for both types of items. > But if you prefer "Or", I'm fine with that too. For function names and variables I do prefer "Or", because in that case the subject is the item, which is a flex *or* grid item. And I think it adds value that they all use "FlexOrGridItem" to make it easy to find them. (and in some cases it would be wrong to use "And", eg: IsAnonymousFlexOrGridItem(const nsIFrame* aFrame) and I'd like to avoid having a mix of "And" and "Or")

Copied with "hg cp" and s/load/fails ==/ as suggested. (the last test pass because the test/ref are both blank ATM)

Comment on attachment 8413191 [details] [diff] [review] part 4, Add some grid item tests. Thanks! r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/da974bf74a0b https://hg.mozilla.org/integration/mozilla-inbound/rev/1a7278aae639 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ec70824b69 https://hg.mozilla.org/integration/mozilla-inbound/rev/e86e6fc40b8b

https://hg.mozilla.org/mozilla-central/rev/da974bf74a0b https://hg.mozilla.org/mozilla-central/rev/1a7278aae639 https://hg.mozilla.org/mozilla-central/rev/d4ec70824b69 https://hg.mozilla.org/mozilla-central/rev/e86e6fc40b8b