Bug 160339

Summary: Determine operator direction in MathMLOperatorElement and reduce relayout
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bfulgham, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 160336    
Bug Blocks: 156537    
Attachments:
Description Flags
Patch
darin: review+
Patch
none
Patch for EWS testing
buildbot: commit-queue-
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch none

Description Frédéric Wang (:fredw) 2016-07-29 06:16:53 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2016-07-29 06:49:27 PDT
Created attachment 284863 [details]
Patch
Comment 2 Darin Adler 2016-07-31 21:20:42 PDT
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.
Comment 3 Frédéric Wang (:fredw) 2016-08-02 08:41:16 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2016-08-02 12:48:29 PDT
Created attachment 285127 [details]
Patch for EWS testing
Comment 5 Build Bot 2016-08-02 13:48:04 PDT
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.
Comment 6 Build Bot 2016-08-02 13:48:07 PDT
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
Comment 7 Frédéric Wang (:fredw) 2016-08-02 14:18:41 PDT
Created attachment 285138 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2016-08-03 00:02:06 PDT
Committed r204075: <http://trac.webkit.org/changeset/204075>