I don't think the nsGridContainerFrameSuper pattern adds any value in general and it makes finding call sites using 'grep' harder so I'm changing it to use nsContainerFrame directly.

(the contents of this method will soon be mostly replaced so don't put to much effort into reviewing it)

Comment on attachment 8421023 [details] [diff] [review] part 2, Add a basic nsGridContainerFrame::Reflow() method >+nsresult >+nsGridContainerFrame::Reflow(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus) >+{ This should return void, now that bug 1008917 has landed, right? >+ nscoord contentHeight = GetEffectiveComputedHeight(aReflowState); >+ if (contentHeight == NS_AUTOHEIGHT) { >+ contentHeight = 0; >+ } Nit: you technically need to clamp contentHeight to be at least as large as the min-height, in the NS_AUTOHEIGHT case. (I'm fine with that being addressed in the subsequent patch that replaces this code, though.) >+ aStatus = NS_FRAME_COMPLETE; >+ aDesiredSize.mCarriedOutBottomMargin.Zero(); I don't think mCarriedOutBottomMargin is (or will become) relevant here, and it's initialized to zero by default, so probably just drop this line. >+ NS_FRAME_SET_TRUNCATION(aStatus, aReflowState, aDesiredSize); >+ return NS_OK; >+} Just to finish out the boilerplate -- I think we also need a call to... FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize, aReflowState, aStatus); ...towards the end of the function. If that's coming in your next version of this function, great (though IMHO it'd make more sense to add it here, since it's not really grid-specific and is boilerplate-ish.) r=me with the above addressed as you see fit.

(In reply to Daniel Holbert [:dholbert] from comment #4) > This should return void, now that bug 1008917 has landed, right? Yes, I wrote this before that bug - thanks for the reminder though! > I don't think mCarriedOutBottomMargin is (or will become) relevant here, and > it's initialized to zero by default, so probably just drop this line. True, I dropped it. > Just to finish out the boilerplate -- I think we also need a call to... > FinishReflowWithAbsoluteFrames(aPresContext, aDesiredSize, > aReflowState, aStatus); > ...towards the end of the function. > > If that's coming in your next version of this function, great (though IMHO > it'd make more sense to add it here, since it's not really grid-specific and > is boilerplate-ish.) I've looked into to reflowing abs.pos. children a bit, and it's probably not going to be boilerplate (since each child needs a separate containing block). Thanks for the advice though, but I think I'll skip this for now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad82809e623 https://hg.mozilla.org/integration/mozilla-inbound/rev/7048bc41993f https://hg.mozilla.org/integration/mozilla-inbound/rev/46cf8e706620

(In reply to Mats Palmgren (:mats) from comment #6) > https://hg.mozilla.org/integration/mozilla-inbound/rev/46cf8e706620 This cset was backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/39d9450b3d60 because dholbert says it depended on bug 1008917, which was backed out.

[marking leave-open for the time being, so this doesn't get auto-closed when the first two parts are merged to central]

The failure that caused bug 1008917 to be backed out happened on retriggers prior to 1008917 landed, so once it relands, this can reland, too.

https://hg.mozilla.org/mozilla-central/rev/6ad82809e623 https://hg.mozilla.org/mozilla-central/rev/7048bc41993f https://hg.mozilla.org/mozilla-central/rev/ac7e1b2d6408