Bug 160339 – Determine operator direction in MathMLOperatorElement and reduce relayout

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

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

After bug 160336, m_isVertical is the only remaining property set in RenderMathMLOperator::setOperatorProperties. We calculate it in MathMLOperatorElement and so RenderMathMLOperator::setOperatorProperties can be removed. It seems that we can then remove many needs-relayout calls without breaking tests.

I'm not sure for mfenced and I think we don't really have good test coverage for this element. So for now to be safe let's kept the calls RenderMathMLFencedOperator::setOperatorProperties in various functions by overriding the RenderMathMLOperator implementations.

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

Created attachment 284863 [details]
Patch

[Darin Adler]

Comment on attachment 284863 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284863&action=review

Change is  OK, but I think we should consider some slightly different idioms here.

> Source/WebCore/mathml/MathMLOperatorElement.cpp:67
>      m_operatorText = Optional<UChar>(parseOperatorText(textContent()));

I don’t think the typecast is needed here. Should be able to take out Optional<UChar>().

> Source/WebCore/mathml/MathMLOperatorElement.cpp:77
> +    if (!m_operatorText)
> +        operatorText();

This is a messy idiom. Would be better to make the function that updates m_operatorText and m_operatorTextIsVertical separate and not just exist as a side effect of calling operatorText(). If you take my suggestion of making the OperatorText struct below then there’s another way to do it with a private function that returns a const OperatorText& and public inline functions that extract the individual data members from the structure.

> Source/WebCore/mathml/MathMLOperatorElement.h:57
>      Optional<UChar> m_operatorText;
> +    bool m_operatorTextIsVertical;

Since these are computed together, the correct idiom would be to have them in a struct and have the struct be the Optional data member. It’s not good to have the boolean have its validity based on the presence of the other member.

    struct OperatorText {
        UChar character;
        bool isVertical;
    };
    Optional<OperatorText> m_operatorText;

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:76
> +    setOperatorProperties();

Given how this is used, the function has a peculiar name. Normally we call an operator like this something like "update" not "set". Generally speaking I am concerned about this pattern where you have to remember to call this function before calling the base class. Seems error prone as we refactor the base class and possibly add additional functions. We should consider a different idiom that isn’t as fragile.

> Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.h:60
>      UChar m_textContent { 0 };
> +    bool m_isVertical { true };

These could use the same struct as the other class above.

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

Created attachment 285111 [details]
Patch

(In reply to comment #2)
> > Source/WebCore/rendering/mathml/RenderMathMLFencedOperator.cpp:76
> > +    setOperatorProperties();
> 
> Given how this is used, the function has a peculiar name. Normally we call
> an operator like this something like "update" not "set". Generally speaking
> I am concerned about this pattern where you have to remember to call this
> function before calling the base class. Seems error prone as we refactor the
> base class and possibly add additional functions. We should consider a
> different idiom that isn’t as fragile.
> 

OK, after rebasing all the patches before that one it turns out that the update can be simplified a lot for mfenced operators too. Essentially, the properties only depend on what was specified in the constructor and of the text content so we can update all of them in RenderMathMLFencedOperator::updateOperatorContent. It seems that the call to setOperatorProperties() were mostly necessary to resolve the LayoutUnit again (e.g. if the style changes) but this can now just be done in minSize, maxSize, leadingSpace and trailingSpace. Actually, as noted by Javier Fernández in a previous patch, minSize and maxSize are just constant for the mfenced operators since they are not specified in the operator dictionary.

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

Created attachment 285127 [details]
Patch for EWS testing

[Build Bot]

Comment on attachment 285127 [details]
Patch for EWS testing

Attachment 285127 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1800193

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 285135 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

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

Created attachment 285138 [details]
Patch

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

Committed r204075: <http://trac.webkit.org/changeset/204075>