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, commitqueue, darin, dbarton, esprehn+autocc, glenn, jfernandez, kondapallykalyan, mrobinson, rego, svillar  
Priority:  P2  
Version:  WebKit Nightly Build  
Hardware:  Unspecified  
OS:  Unspecified  
URL:  http://www.mathmlassociation.org/MathMLinHTML5/S3.html#SS3.SSS9  
Bug Depends on:  153208  
Bug Blocks:  127466, 153991  
Attachments: 

Description
Frédéric Wang (:fredw)
20160304 08:03:38 PST
Created attachment 273558 [details]
Patch
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. Created attachment 275290 [details]
Patch
Created attachment 275401 [details]
Patch
Created attachment 276822 [details] Patch (to apply after the patch for bug 153987) Updating patch. Created attachment 276823 [details]
Patch
Created attachment 276824 [details]
Patch
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). Comment on attachment 276824 [details] Patch Attachment 276824 [details] did not pass iossimews (iossimulatorwk2): Output: http://webkitqueues.webkit.org/results/1191273 New failing tests: mathml/presentation/menclosenotationvalues.html mathml/presentation/menclosenotationdefaultlongdiv.html Created attachment 276825 [details]
Archive of layouttestresults from ews123 for iossimulatorwk2
The attached test failures were seen while running runwebkittests on the iossimews.
Bot: ews123 Port: iossimulatorwk2 Platform: Mac OS X 10.10.5
Created attachment 276826 [details]
Patch
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 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.mathmlassociation.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. Created attachment 277016 [details]
Patch for EWS testing
I slightly change horizontal/vertical strike so running again to get ios/Mac results.
Created attachment 277018 [details]
Patch for EWS testing
Created attachment 277020 [details]
Patch
Comment on attachment 277018 [details]
Patch for EWS testing
Well not need for that one since the patch applies on trunk...
Created attachment 277037 [details]
Patch
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; Supernit, 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. Created attachment 277234 [details] Patch Update patch to address review comments and fix conflicts after bug 156846. Committed r199980: <http://trac.webkit.org/changeset/199980> 