User Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110615 Firefox/7.0a1 Build ID: 20110629110552 Steps to reproduce: These files can be reduced to a single file by adding condition on mContent->Tag that will be easier to fix the bug 557476

One space between 'if' and the following '(', everywhere, please.

(In reply to comment #1) > Created attachment 543816 [details] [diff] [review] [review] > Patch merge munder and mover into munderover nsMathMLmsubsupFrame::PlaceSubSupScript is incorrectly used in nsMathMLmunderoverFrame::Place with this patch only. Jonathan has another patch merging msup and msub into msubsup.

I don't think msubsup with empty scripts is currently equivalent to msub/msup. Merging msub, msup and msubsup won't be very helpful to fix bug 557476 anyway. To prevent adding potential regressions, we can maybe just modify the attachment 543910 [details] [diff] [review] in that way: instead of passing a nsIAtom* Tag to nsMathMLmsubsupFrame::PlaceSubSupScript according to the mContent->Tag() value, we can directly choose the relevant nsMathML*Frame::Place*Script function to call.

Jonathan, do you have an updated version of the munderover patch ready?

Comment on attachment 545349 [details] [diff] [review] Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame Can you mention in the comment before NS_NewMathMLmunderoverFrame that this also applies to munder and mover please? Perhaps just copy the <munder> and <mover> opening comments from the deleted files. Can you copy the opening "REC says" comment from TransmitAutomaticData for the mover or munder frame, please? >+ if (mContent->Tag() == nsGkAtoms::munder_ || >+ mContent->Tag() == nsGkAtoms::munderover_) { >+ underscriptFrame = baseFrame->GetNextSibling(); >+ } >+ if (mContent->Tag() == nsGkAtoms::mover_) { >+ overscriptFrame = baseFrame->GetNextSibling(); >+ } It would simplify things a bit to declare a local variable and use that. nsIAtom* tag = mContent->Tag(); Can you make the second "if" and "else", please, and put "tag == nsGkAtoms::mover_" in an NS_ASSERTION? >+ if (mContent->Tag() == nsGkAtoms::munderover_) { >+ return nsMathMLmsubsupFrame::PlaceSubSupScript(PresContext(), >+ aRenderingContext, >+ aPlaceOrigin, >+ aDesiredSize, >+ this, 0, 0, >+ nsPresContext::CSSPointsToAppUnits(0.5f)); >+ } else if (mContent->Tag() == nsGkAtoms::munder_) { >+ return nsMathMLmsubFrame::PlaceSubScript(PresContext(), >+ aRenderingContext, >+ aPlaceOrigin, >+ aDesiredSize, >+ this, 0, >+ nsPresContext::CSSPointsToAppUnits(0.5f)); >+ } else if (mContent->Tag() == nsGkAtoms::mover_) { >+ return nsMathMLmsupFrame::PlaceSuperScript(PresContext(), >+ aRenderingContext, >+ aPlaceOrigin, >+ aDesiredSize, >+ this, 0, >+ nsPresContext::CSSPointsToAppUnits(0.5f)); >+ } Can you create local variables, please, for mContent->Tag() (for the whole function) and nsPresContext::CSSPointsToAppUnits(0.5f) (for this block)? Here again, I think the last "else if" can be changed to "else" with an assertion. > } >- >+ Unnecessary whitespace change. >+ if (mContent->Tag() == nsGkAtoms::munder_ || >+ mContent->Tag() == nsGkAtoms::munderover_) { >+ GetReflowAndBoundingMetricsFor(underFrame, underSize, bmUnder); >+ } >+ if (mContent->Tag() == nsGkAtoms::mover_ || >+ mContent->Tag() == nsGkAtoms::munderover_) { >+ GetReflowAndBoundingMetricsFor(overFrame, overSize, bmOver); >+ } It's probably simpler here and below to use tests of the form "if (underFrame) {". Can you copy the "Rule 12, App. G, TeXbook" comment and "// also ensure at least x-height above the baseline of the base" from moverFrame, please?

Thanks for making those changes. There's still a large comment in moverFrame about "Rule 12, App. G, TeXbook" that I would like to see copied over. I'd prefer to wait on landing this until we understand and preferably fix the remaining issue in bug 669932. Bug 669932 tests that these patches achieve the desired results.