nsMathMLContainerFrame::ReLayoutChildren sets redundant dirty bits

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: dbaron, Assigned: anujagarwal464)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

As described in the XXX comments in http://hg.mozilla.org/mozilla-central/rev/81bf5017af71 , 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++]
Created attachment 8371468 [details]
Example for RelayoutChildren
(Assignee)

Comment 2

5 years ago
Created attachment 8372030 [details] [diff] [review]
rev1 - removes code from ReLayoutChildren
Attachment #8372030 - Flags: review?(fred.wang)
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 https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests 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):

https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mo
https://developer.mozilla.org/en-US/docs/Web/MathML/Element/munderover

The idea for the reftests would be to compare a page that is modified via Javascript against a static reference. See for example
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/mathml/displaystyle-4.html?force=1
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/mathml/displaystyle-4-ref.html?force=1

::: 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?(fred.wang)
An example for mo@movablelimits is already given in the testcase. For the 3 other attributes, try to compare:

    <math>
      <munderover><mtext>X</mtext><mo>O</mo><mo>O</mo></munderover>
    </math>

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

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

    <math>
      <munderover accentunder="true"><mtext>X</mtext><mo>O</mo><mo>O</mo></munderover>
    </math>
(Assignee)

Comment 5

5 years ago
Created attachment 8372180 [details] [diff] [review]
rev2 - Removed frame->AddStateBits from nsMathMLContainerFrame::ReLayoutChildren  and nsMathMLContainerFrame::ChildListChanged
Attachment #8372180 - Flags: review?(fred.wang)
Attachment #8372180 - Flags: review?(fred.wang) → review+
(Assignee)

Updated

5 years ago
Attachment #8372030 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1790ca834d56
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Comment 8

5 years ago
Created attachment 8373961 [details]
Reftest.zip
Attachment #8373961 - Flags: review?(fred.wang)
Comment on attachment 8373961 [details]
Reftest.zip

@Anuj:

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 (https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch). You can create patches using a mercurial queue (see https://developer.mozilla.org/en-US/docs/Mercurial_Queues). 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?(fred.wang) → review-
Blocks: 970977
You need to log in before you can comment on or make changes to this bug.