668204 - Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jonathan Hage]

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

Comment 1

[Jonathan Hage]

Attachment: Patch merge munder and mover into munderover

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 3

[Frédéric Wang (:fredw)]

(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.

Comment 4

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame

Comment 5

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmsubFrame and nsMathMLmsupFrame into nsMathMLmsubsupFrame

Comment 6

[Jonathan Hage]

Attachment: Test

Comment 7

[Frédéric Wang (:fredw)]

Attachment: test msubsup empty scripts

Comment 8

[Frédéric Wang (:fredw)]

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.

Comment 9

[Frédéric Wang (:fredw)]

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

Comment 10

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame

Comment 11

[Karl Tomlinson (:karlt)]

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?

Comment 12

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V2

Comment 13

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V3

Comment 14

[Karl Tomlinson (:karlt)]

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.

Comment 15

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4

Comment 16

[Jonathan Hage]

Attachment: Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/a3adb8c01a19