1009272 - [css-grid] [DEBUG] add some sanity checks for the grid container child frame lists

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

I just shamelessly stole your stuff from the flexbox code and
tweaked it a bit :-) .  We might want to share a common function
later on but for now, while the code is still evolving, it might
be better to have a separate copy to get something in place
quickly to catch any regressions.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8421368 [details] [diff] [review]
fix

>+#if DEBUG

This should be #ifdef, not #if, I think. (At least, I think that's the standard way we check for this everywhere else, and we should be consistent, even if this saves 3 characters and technically works.) :) Unless we're actively migrating to use #if DEBUG in general? (I'm not aware of it, but maybe I missed the memo.)

>+static bool
>+FrameWantsToBeInAnonymousGridItem(nsIFrame* aFrame)
>+{
>+  // Note: This needs to match the logic in
>+  // nsCSSFrameConstructor::FrameConstructionItem::NeedsAnonFlexItem()

This comment is out of date -- the function it's referring to is now called NeedsAnonFlexOrGridItem. Could you fix that here and in nsFlexContainerFrame while you're at it?

>+#if 0 // XXX haven't decided yet whether to reorder children or not
>+        MOZ_ASSERT(!prevChildWasAnonGridItem ||
>+                   HasAnyStateBits(NS_STATE_GRID_CHILDREN_REORDERED),
>+                   "two anon grid items in a row (shouldn't happen, unless our "
>+                   "children have been reordered with the 'order' property)");
>+#else
>+        MOZ_ASSERT(!prevChildWasAnonGridItem, "two anon grid items in a row");
>+#endif

I seem to recall some general prohibition against intentionally-#if-0 code; I think wrapping this in a /**/ comment is slightly preferred, maybe extending your XXX comment slightly to say "Maybe use this instead of the following assertion (haven't decided yet [...]):"

I'll trust your judgement on that, though, since you've been working under our coding style longer than I have. :)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8421368 [details] [diff] [review]
> fix
> 
> >+#if DEBUG
> 
> This should be #ifdef, not #if, I think. (At least, I think that's the
> standard way we check for this everywhere else, and we should be consistent,
> even if this saves 3 characters and technically works.) :)

Note: for MXR support:
 Over 1000 hits (i.e. zillions) for #ifdef:
  http://mxr.mozilla.org/mozilla-central/search?string=%23ifdef+DEBUG%24&regexp=1
 85 hits for #if (many of which are in 3rd-party code):
  http://mxr.mozilla.org/mozilla-central/search?string=%23if+DEBUG%24&regexp=1

Comment 4

[Mats Palmgren (inactive)]

With nits fixed (also updated the comment in nsFlexContainerFrame).
https://hg.mozilla.org/integration/mozilla-inbound/rev/40c010000007

Comment 5

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b77f70b817 for b2g build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=39979041&tree=Mozilla-Inbound

Comment 6

[Mats Palmgren (inactive)]

The build problem was hidden by unified builds :(
The fixes, adding a missing #include and s/FrameChildListIDs/ChildListIDs/
are trivial so I didn't bother with another review:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f1981b6a49

It's green on Try at least:
https://tbpl.mozilla.org/?tree=Try&rev=e4eaf8f57d90

(I've moved nsGridContainerFrame.cpp from UNIFIED_SOURCES to SOURCES in
my local tree now, to avoid this in the future)

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57f1981b6a49