The default bug view has changed. See this FAQ.

Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame

RESOLVED FIXED in mozilla8

Status

()

Core
MathML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jonathan Hage, Assigned: Jonathan Hage)

Tracking

Trunk
mozilla8
All
Other
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 557476
(Assignee)

Updated

6 years ago
Component: Layout: Text → MathML
Assignee: nobody → hage.jonathan
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: layout.fonts-and-text → mathml
(Assignee)

Comment 1

6 years ago
Created attachment 543816 [details] [diff] [review]
Patch merge munder and mover into munderover
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
(Assignee)

Comment 4

6 years ago
Created attachment 543910 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame
Attachment #543816 - Attachment is obsolete: true
Attachment #543816 - Flags: review?(karlt)
(Assignee)

Comment 5

6 years ago
Created attachment 543911 [details] [diff] [review]
Patch: Merge nsMathMLmsubFrame and nsMathMLmsupFrame into nsMathMLmsubsupFrame
(Assignee)

Comment 6

6 years ago
Created attachment 543924 [details]
Test
(Assignee)

Updated

6 years ago
Attachment #543910 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
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
Created attachment 545054 [details]
test msubsup empty scripts
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
(Assignee)

Comment 10

6 years ago
Created attachment 545349 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame
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+
(Assignee)

Comment 12

6 years ago
Created attachment 545626 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V2
Attachment #545349 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 545631 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V3
Attachment #545626 - Attachment is obsolete: true
Keywords: checkin-needed
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
(Assignee)

Comment 15

6 years ago
Created attachment 546515 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4
Attachment #545631 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
Created attachment 551707 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4
Attachment #546515 - Attachment is obsolete: true
Keywords: checkin-needed

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/a3adb8c01a19
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.