RESOLVED FIXED 155019
Refactor RenderMathMLMenclose
https://bugs.webkit.org/show_bug.cgi?id=155019
Summary Refactor RenderMathMLMenclose
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
Patch (117.92 KB, patch)
2016-03-11 01:27 PST, Frédéric Wang (:fredw)
no flags
Patch (117.87 KB, patch)
2016-03-31 08:20 PDT, Frédéric Wang (:fredw)
no flags
Patch (117.81 KB, patch)
2016-04-01 06:11 PDT, Frédéric Wang (:fredw)
no flags
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
Patch (84.19 KB, patch)
2016-04-20 07:45 PDT, Frédéric Wang (:fredw)
no flags
Patch (119.74 KB, patch)
2016-04-20 07:48 PDT, Frédéric Wang (:fredw)
no flags
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
Patch (129.36 KB, patch)
2016-04-20 08:54 PDT, Frédéric Wang (:fredw)
no flags
Patch for EWS testing (95.90 KB, patch)
2016-04-22 00:33 PDT, Frédéric Wang (:fredw)
no flags
Patch for EWS testing (131.45 KB, patch)
2016-04-22 00:41 PDT, Frédéric Wang (:fredw)
no flags
Patch (131.62 KB, patch)
2016-04-22 01:00 PDT, Frédéric Wang (:fredw)
no flags
Patch (133.30 KB, patch)
2016-04-22 03:13 PDT, Frédéric Wang (:fredw)
svillar: review+
Patch (134.46 KB, patch)
2016-04-25 02:06 PDT, Frédéric Wang (:fredw)
no flags
Frédéric Wang (:fredw)
Comment 1 2016-03-10 03:41:41 PST
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
Frédéric Wang (:fredw)
Comment 4 2016-04-01 06:11:46 PDT
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
Frédéric Wang (:fredw)
Comment 7 2016-04-20 07:48:01 PDT
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
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
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
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
Note You need to log in before you can comment on or make changes to this bug.