477915 - nsMathMLContainerFrame::ReLayoutChildren sets redundant dirty bits

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[David Baron :dbaron:]

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.)

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Example for RelayoutChildren

Comment 2

[Anuj Agarwal [:anujagarwal464]]

Attachment: rev1 - removes code from ReLayoutChildren

Comment 3

[Frédéric Wang (:fredw)]

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.

Comment 4

[Frédéric Wang (:fredw)]

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>

Comment 5

[Anuj Agarwal [:anujagarwal464]]

Attachment: rev2 - Removed frame->AddStateBits from nsMathMLContainerFrame::ReLayoutChildren and nsMathMLContainerFrame::ChildListChanged

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1790ca834d56

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1790ca834d56

Comment 8

[Anuj Agarwal [:anujagarwal464]]

Attachment: Reftest.zip

Comment 9

[Frédéric Wang (:fredw)]

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.