Last Comment Bug 668204 - Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame
: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All Other
: -- normal (vote)
: mozilla8
Assigned To: Jonathan Hage
:
:
Mentors:
Depends on: 669932
Blocks: munderover-align 669713
  Show dependency treegraph
 
Reported: 2011-06-29 07:03 PDT by Jonathan Hage
Modified: 2011-08-14 05:11 PDT (History)
5 users (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch merge munder and mover into munderover (17.25 KB, patch)
2011-07-04 14:10 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame (54.29 KB, patch)
2011-07-05 05:43 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch: Merge nsMathMLmsubFrame and nsMathMLmsupFrame into nsMathMLmsubsupFrame (37.09 KB, patch)
2011-07-05 05:45 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Test (1.15 KB, text/html)
2011-07-05 06:41 PDT, Jonathan Hage
no flags Details
test msubsup empty scripts (1.75 KB, text/html)
2011-07-10 01:33 PDT, Frédéric Wang (:fredw)
no flags Details
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame (55.93 KB, patch)
2011-07-12 03:12 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V2 (57.50 KB, patch)
2011-07-13 02:13 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V3 (57.45 KB, patch)
2011-07-13 02:39 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4 (59.20 KB, patch)
2011-07-18 06:26 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4 (59.21 KB, patch)
2011-08-09 02:46 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review

Description Jonathan Hage 2011-06-29 07:03:30 PDT
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 2011-07-04 14:10:21 PDT
Created attachment 543816 [details] [diff] [review]
Patch merge munder and mover into munderover
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-07-04 14:14:26 PDT
One space between 'if' and the following '(', everywhere, please.
Comment 3 Frédéric Wang (:fredw) 2011-07-04 14:24:14 PDT
(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 2011-07-05 05:43:30 PDT
Created attachment 543910 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame
Comment 5 Jonathan Hage 2011-07-05 05:45:32 PDT
Created attachment 543911 [details] [diff] [review]
Patch: Merge nsMathMLmsubFrame and nsMathMLmsupFrame into nsMathMLmsubsupFrame
Comment 6 Jonathan Hage 2011-07-05 06:41:28 PDT
Created attachment 543924 [details]
Test
Comment 7 Frédéric Wang (:fredw) 2011-07-10 01:33:19 PDT
Created attachment 545054 [details]
test msubsup empty scripts
Comment 8 Frédéric Wang (:fredw) 2011-07-10 01:38:49 PDT
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) 2011-07-11 10:12:49 PDT
Jonathan, do you have an updated version of the munderover patch ready?
Comment 10 Jonathan Hage 2011-07-12 03:12:51 PDT
Created attachment 545349 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame
Comment 11 Karl Tomlinson (:karlt) 2011-07-12 23:46:54 PDT
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 2011-07-13 02:13:50 PDT
Created attachment 545626 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V2
Comment 13 Jonathan Hage 2011-07-13 02:39:50 PDT
Created attachment 545631 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V3
Comment 14 Karl Tomlinson (:karlt) 2011-07-13 20:14:41 PDT
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 2011-07-18 06:26:47 PDT
Created attachment 546515 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4
Comment 16 Jonathan Hage 2011-08-09 02:46:04 PDT
Created attachment 551707 [details] [diff] [review]
Patch: Merge nsMathMLmunderFrame and nsMathMLmoverFrame into nsMathMLmunderoverFrame V4
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-14 05:11:27 PDT
http://hg.mozilla.org/mozilla-central/rev/a3adb8c01a19

Note You need to log in before you can comment on or make changes to this bug.