WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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.
Top of Page
Format For Printing
XML
Clone This Bug