Closed Bug 668204 Opened 8 years ago Closed 8 years ago

Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame

Categories

(Core :: MathML, defect)

All
Other
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: hage.jonathan, Assigned: hage.jonathan)

References

Details

Attachments

(3 files, 7 obsolete files)

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
Component: Layout: Text → MathML
Assignee: nobody → hage.jonathan
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: layout.fonts-and-text → mathml
Attachment #543816 - Flags: review?(karlt)
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.
Status: NEW → ASSIGNED
Attachment #543816 - Attachment is obsolete: true
Attachment #543816 - Flags: review?(karlt)
Attached file Test
Attachment #543910 - Flags: review?(karlt)
Attachment #543911 - Flags: review?(karlt)
Attachment #543924 - Attachment mime type: text/plain → text/html
Attachment #543924 - Attachment mime type: text/html → text/plain
Depends on: 669932
Attachment #543924 - Attachment mime type: text/plain → text/html
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?
Attachment #543911 - Flags: review?(karlt)
Attachment #543911 - Attachment is obsolete: true
Attachment #543910 - Attachment is obsolete: true
Attachment #545349 - Flags: review?(karlt)
Attachment #543910 - Flags: review?(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?
Attachment #545349 - Flags: review?(karlt) → review+
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.
Keywords: checkin-needed
Blocks: 669713
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/a3adb8c01a19
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.