Closed
Bug 1009272
Opened 10 years ago
Closed 10 years ago
[css-grid] [DEBUG] add some sanity checks for the grid container child frame lists
Categories
(Core :: Layout, defect, P4)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Attachment #8421368 -
Flags: review?(dholbert)
Comment 2•10 years ago
|
||
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. :)
Attachment #8421368 -
Flags: review?(dholbert) → review+
Comment 3•10 years ago
|
||
(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®exp=1 85 hits for #if (many of which are in 3rd-party code): http://mxr.mozilla.org/mozilla-central/search?string=%23if+DEBUG%24®exp=1
Assignee | ||
Comment 4•10 years ago
|
||
With nits fixed (also updated the comment in nsFlexContainerFrame). https://hg.mozilla.org/integration/mozilla-inbound/rev/40c010000007
Flags: in-testsuite-
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
Flags: needinfo?(matspal)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Flags: needinfo?(matspal)
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/57f1981b6a49
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•