Bug 155019 – Refactor RenderMathMLMenclose

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

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

With the new MathML layout, the RenderMathMLMenclose class probably needs to be rewritten too to implement computePreferredLogicalWidths, layoutBlock, firstBaseLine etc since (because of the notations) the metrics and child placement do not really exactly correspond to RenderMathMLRow. I suspect the list of notations could also be more efficiently managed with an integer m_Notations and corresponding bit mask for each notation as done in Gecko. Last but not least, we should avoid creating anonymous RenderMathMLSquareRoot.

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

Created attachment 273558 [details]
Patch

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

Created attachment 273708 [details]
Patch

I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.

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

Created attachment 275290 [details]
Patch

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

Created attachment 275401 [details]
Patch

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

Created attachment 276822 [details]
Patch (to apply after the patch for bug 153987)

Updating patch.

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

Created attachment 276823 [details]
Patch

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

Created attachment 276824 [details]
Patch

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

I've uploaded a new version of the patch that now applies to trunk of repository. I'm thus removing dependencies on 153987 and 155018 and  the refactoring of RenderMathMLRoot and RenderMathMLEnclose can now be reviewed independently.

The differences between the old and new patch are essentially:

1) We remove the anonymous constructors from RenderMathMLSquareRoot too, since that class has not been removed yet (it is removed in the patch for bug 153987).

2) We include the changes from RenderMathMLRow to make its layout functions usable by its descendants (these changes are also in the patch for bug 153987).

[Build Bot]

Comment on attachment 276824 [details]
Patch

Attachment 276824 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1191273

New failing tests:
mathml/presentation/menclose-notation-values.html
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 276825 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

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

Created attachment 276826 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 276826 [details]
Patch

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

Awesome rewrite. I was about to give the r+ but I'll wait for the replies to my questions.

> Source/WebCore/rendering/RenderObject.h:-391
> -    virtual bool isRenderMathMLMenclose() const { return false; }

I don't get this change.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:41
> +const unsigned short longDivLeftSpace = 10;

Where does this magic number come from?

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95
> +    }

We need an explanation for those magic numbers (some of them don't look "consistent", extraSpace uses 4 for all the cases except this last one). Mentioning the spec/implementation note will be enough.

If they actually mean something consider giving them a name.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:176
> +    info.context().setFillColor(Color::transparent);

Do we have any test which checks colors, style and thickness of the strokes?

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:183
> +        LayoutUnit yEnd = m_contentRect.maxY() + 4 * thickness;

More magic numbers.

Ditto for all the other cases.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:-50
> -SPECIALIZE_TYPE_TRAITS_RENDER_OBJECT(RenderMathMLMenclose, isRenderMathMLMenclose())

This is related to the removal in RenderObject.h

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

Comment on attachment 276826 [details]
Patch

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

>> Source/WebCore/rendering/RenderObject.h:-391
>> -    virtual bool isRenderMathMLMenclose() const { return false; }
> 
> I don't get this change.

This was added in bug 153208 only to for the workaround in RenderMathMLRow::layoutRowItems. Now it's not used anymore. Not sure we want to keep it or not...

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:41
>> +const unsigned short longDivLeftSpace = 10;
> 
> Where does this magic number come from?

This is arbitrary, the MathML spec does not give any indication and the implementation note says to draw it as a radical but using U+0028 LEFT PARENTHESIS instead of U+221A SQUARE ROOT. But we can't do that before bug 152244 and anway I'm not sure it's really important so I kept the current Bézier curver implementation for now.

Maybe I should explain that in a comment.

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95
>> +    }
> 
> We need an explanation for those magic numbers (some of them don't look "consistent", extraSpace uses 4 for all the cases except this last one). Mentioning the spec/implementation note will be enough.
> 
> If they actually mean something consider giving them a name.

Again, the MathML spec does not explain anything. Here I follow the implementation note (except for longdiv) where \xi_8, Overbar* and Underbar* are replaced with thickness for now. See http://www.mathml-association.org/MathMLinHTML5/S3.html#SS3.SSS9

I'll add a comment.

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:176
>> +    info.context().setFillColor(Color::transparent);
> 
> Do we have any test which checks colors, style and thickness of the strokes?

Normally yes but I'll check that.

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:-50
>> -SPECIALIZE_TYPE_TRAITS_RENDER_OBJECT(RenderMathMLMenclose, isRenderMathMLMenclose())
> 
> This is related to the removal in RenderObject.h

Yes.

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

Created attachment 277016 [details]
Patch for EWS testing

I slightly change horizontal/vertical strike so running again to get ios/Mac results.

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

Created attachment 277018 [details]
Patch for EWS testing

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

Created attachment 277020 [details]
Patch

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

Comment on attachment 277018 [details]
Patch for EWS testing

Well not need for that one since the patch applies on trunk...

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

Created attachment 277037 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 277037 [details]
Patch

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

Fantastic job.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:231
> +        Path line;

Super-nit, move the declaration down to the point were it's first used.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:255
> +        Path line;

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:268
> +        Path line;

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:279
> +        Path line;

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:285
> +        info.context().strokePath(line);

I've just realized that all these previous if blocks do basically the same but with different x,y coordinates computation. We should move the implementation to a private method which receives those two parameters and perform the required operations with the Path line.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:294
> +        info.context().strokePath(line);

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:303
> +        info.context().strokePath(line);

Ditto.

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

Created attachment 277234 [details]
Patch

Update patch to address review comments and fix conflicts after bug 156846.

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

Committed r199980: <http://trac.webkit.org/changeset/199980>