Closed Bug 477915 Opened 16 years ago Closed 11 years ago

nsMathMLContainerFrame::ReLayoutChildren sets redundant dirty bits


(Core :: MathML, defect)

Not set





(Reporter: dbaron, Assigned: anujagarwal464)



(Whiteboard: [mentor=fredw][lang=c++])


(3 files, 1 obsolete file)

As described in the XXX comments in , nsMathMLContainerFrame::ReLayoutChildren sets redundant dirty bits, since:
 * NS_FRAME_IS_DIRTY implies NS_FRAME_IS_DIRTY on all descendants
 * (because of previous) NS_FRAME_IS_DIRTY implies NS_FRAME_HAS_DIRTY_CHILDREN

(It's also not clear to me it needs to be doing quite so much marking dirty.)
Blocks: 744783
Assignee: nobody → anujagarwal464
Whiteboard: [mentor=fredw][lang=c++]
Attachment #8372030 - Flags: review?(
Comment on attachment 8372030 [details] [diff] [review]
rev1 - removes code from ReLayoutChildren

Review of attachment 8372030 [details] [diff] [review]:

> And I don’t know how to write reftests but willing to learn if you help. 

See for general information for how to write and run the tests.

Normally, the patch should not modify the current behavior but I think we could at least write reftests for the two ReLayoutChildren calls in nsMathMLmoFrame.cpp and nsMathMLmunderoverFrame.cpp ; that is when accent/movablelimits and accent/accentunder changed (the tescase shows the example of movablelimits):

The idea for the reftests would be to compare a page that is modified via Javascript against a static reference. See for example

::: layout/mathml/nsMathMLContainerFrame.cpp
@@ +703,1 @@

Can you delete the entire line (i.e. don't leave a blank line). Also can you remove the comment above "mark the frame dirty... work this does."

There is a similar AddStateBits in sMathMLContainerFrame::ReLayoutChildren that should be removed.
Attachment #8372030 - Flags: review?(
An example for mo@movablelimits is already given in the testcase. For the 3 other attributes, try to compare:


      <munderover><mtext>X</mtext><mo accent="true">O</mo><mo>O</mo></munderover>

      <munderover accent="true"><mtext>X</mtext><mo>O</mo><mo>O</mo></munderover>

      <munderover accentunder="true"><mtext>X</mtext><mo>O</mo><mo>O</mo></munderover>
Attachment #8372180 - Flags: review?( → review+
Attachment #8372030 - Attachment is obsolete: true
Keywords: checkin-needed
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attached file
Attachment #8373961 - Flags: review?(
Comment on attachment 8373961 [details]


thank you. In general when you ask for checkin the bug will be closed, so you should do that only when the work is done. Can you please open a new bug to add these reftests?

Note that for review you should attach a patch, not a zip archives ( You can create patches using a mercurial queue (see To add the new files, use "hg add".

Regarding the reftests, they look good:
- perhaps you could find better names and HTML title for the files. In general the reference name ends by "-ref".
- I would put the static files as the reference.
- the reftest.list as well as the HTML files should be in the layout/reftests/mathml/ directory.

Also, please try to run the tests locally if you didn't already.
Attachment #8373961 - Flags: review?( → review-
Blocks: 970977
You need to log in before you can comment on or make changes to this bug.