Bug 155019

Summary:

Refactor RenderMathMLMenclose

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

MathML

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cfleizach, commit-queue, darin, dbarton, esprehn+autocc, glenn, jfernandez, kondapallykalyan, mrobinson, rego, svillar

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

URL:

http://www.mathml-association.org/MathMLinHTML5/S3.html#SS3.SSS9

Bug Depends on:

153208

Bug Blocks:

127466, 153991

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch (to apply after the patch for bug 153987)	none
Patch	none
Patch	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Patch	none
Patch for EWS testing	none
Patch for EWS testing	none
Patch	none
Patch	svillar: review+
Patch	none

Frédéric Wang (:fredw)

Reported 2016-03-04 08:03:38 PST

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.

Attachments
Patch (114.26 KB, patch) 2016-03-10 03:41 PST, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (117.92 KB, patch) 2016-03-11 01:27 PST, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (117.87 KB, patch) 2016-03-31 08:20 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (117.81 KB, patch) 2016-04-01 06:11 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (to apply after the patch for bug 153987) (117.95 KB, patch) 2016-04-20 07:13 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (84.19 KB, patch) 2016-04-20 07:45 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (119.74 KB, patch) 2016-04-20 07:48 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (644.46 KB, application/zip) 2016-04-20 08:43 PDT, Build Bot	no flags	Details
Patch (129.36 KB, patch) 2016-04-20 08:54 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch for EWS testing (95.90 KB, patch) 2016-04-22 00:33 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch for EWS testing (131.45 KB, patch) 2016-04-22 00:41 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (131.62 KB, patch) 2016-04-22 01:00 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (133.30 KB, patch) 2016-04-22 03:13 PDT, Frédéric Wang (:fredw)	svillar: review+	Details Formatted Diff Diff
Patch (134.46 KB, patch) 2016-04-25 02:06 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2016-03-10 03:41:41 PST

Created attachment 273558 [details] Patch

Frédéric Wang (:fredw)

Comment 2 2016-03-11 01:27:59 PST

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)

Comment 3 2016-03-31 08:20:21 PDT

Created attachment 275290 [details] Patch

Frédéric Wang (:fredw)

Comment 4 2016-04-01 06:11:46 PDT

Created attachment 275401 [details] Patch

Frédéric Wang (:fredw)

Comment 5 2016-04-20 07:13:31 PDT

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

Frédéric Wang (:fredw)

Comment 6 2016-04-20 07:45:00 PDT

Created attachment 276823 [details] Patch

Frédéric Wang (:fredw)

Comment 7 2016-04-20 07:48:01 PDT

Created attachment 276824 [details] Patch

Frédéric Wang (:fredw)

Comment 8 2016-04-20 07:55:17 PDT

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 9 2016-04-20 08:43:30 PDT

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

Comment 10 2016-04-20 08:43:36 PDT

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)

Comment 11 2016-04-20 08:54:27 PDT

Created attachment 276826 [details] Patch

Sergio Villar Senin

Comment 12 2016-04-21 07:49:14 PDT

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 13 2016-04-21 08:06:20 PDT

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)

Comment 14 2016-04-22 00:33:54 PDT

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)

Comment 15 2016-04-22 00:41:00 PDT

Created attachment 277018 [details] Patch for EWS testing

Frédéric Wang (:fredw)

Comment 16 2016-04-22 01:00:33 PDT

Created attachment 277020 [details] Patch

Frédéric Wang (:fredw)

Comment 17 2016-04-22 01:10:08 PDT

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)

Comment 18 2016-04-22 03:13:17 PDT

Created attachment 277037 [details] Patch

Sergio Villar Senin

Comment 19 2016-04-25 01:06:40 PDT

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)

Comment 20 2016-04-25 02:06:14 PDT

Created attachment 277234 [details] Patch Update patch to address review comments and fix conflicts after bug 156846.

Frédéric Wang (:fredw)

Comment 21 2016-04-25 02:46:14 PDT

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

Note You need to log in before you can comment on or make changes to this bug.