WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153918
Refactor RenderMathMLScripts layout function to avoid using flexbox
https://bugs.webkit.org/show_bug.cgi?id=153918
Summary
Refactor RenderMathMLScripts layout function to avoid using flexbox
Alejandro G. Castro
Reported
2016-02-05 08:42:26 PST
Next patch in the series adding code to handle the layout of the scripts without the flexbox.
Attachments
Patch
(10.45 KB, patch)
2016-02-05 08:53 PST
,
Alejandro G. Castro
no flags
Details
Formatted Diff
Diff
GTK a11y test ref update
(1.24 KB, patch)
2016-02-12 02:13 PST
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(11.77 KB, patch)
2016-02-12 04:27 PST
,
Alejandro G. Castro
no flags
Details
Formatted Diff
Diff
Patch
(94.36 KB, patch)
2016-03-04 07:34 PST
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(193.77 KB, patch)
2016-03-07 23:51 PST
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(198.10 KB, patch)
2016-03-11 01:24 PST
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(192.97 KB, patch)
2016-03-31 08:18 PDT
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(193.00 KB, patch)
2016-04-01 06:10 PDT
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(193.43 KB, patch)
2016-04-08 04:38 PDT
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(193.89 KB, patch)
2016-04-12 04:06 PDT
,
Frédéric Wang (:fredw)
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2
(861.07 KB, application/zip)
2016-04-12 04:58 PDT
,
Build Bot
no flags
Details
Patch
(203.24 KB, patch)
2016-04-12 06:50 PDT
,
Frédéric Wang (:fredw)
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2
(593.38 KB, application/zip)
2016-04-12 07:41 PDT
,
Build Bot
no flags
Details
Patch
(203.23 KB, patch)
2016-04-12 08:36 PDT
,
Frédéric Wang (:fredw)
no flags
Details
Formatted Diff
Diff
Patch
(200.98 KB, patch)
2016-04-14 12:24 PDT
,
Frédéric Wang (:fredw)
mrobinson
: review-
Details
Formatted Diff
Diff
Patch
(201.19 KB, patch)
2016-04-18 02:06 PDT
,
Frédéric Wang (:fredw)
mrobinson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(15)
View All
Add attachment
proposed patch, testcase, etc.
Alejandro G. Castro
Comment 1
2016-02-05 08:53:33 PST
Created
attachment 270746
[details]
Patch
Frédéric Wang (:fredw)
Comment 2
2016-02-12 02:13:02 PST
Created
attachment 271155
[details]
GTK a11y test ref update This patch modifies the reference of one GTK accessibility test (when I apply it on master after the patches from the previous MathML refactoring bugs). It's not a real test failure, just some bounding box changes. See attachment.
Alejandro G. Castro
Comment 3
2016-02-12 04:06:13 PST
(In reply to
comment #2
)
> Created
attachment 271155
[details]
> GTK a11y test ref update > > This patch modifies the reference of one GTK accessibility test (when I > apply it on master after the patches from the previous MathML refactoring > bugs). It's not a real test failure, just some bounding box changes. See > attachment.
Thanks for the test, I'll upload the patch with this modification.
Alejandro G. Castro
Comment 4
2016-02-12 04:27:08 PST
Created
attachment 271165
[details]
Patch
Frédéric Wang (:fredw)
Comment 5
2016-02-12 04:54:50 PST
(In reply to
comment #3
)
> (In reply to
comment #2
) > > Created
attachment 271155
[details]
> > GTK a11y test ref update > > > > This patch modifies the reference of one GTK accessibility test (when I > > apply it on master after the patches from the previous MathML refactoring > > bugs). It's not a real test failure, just some bounding box changes. See > > attachment. > > Thanks for the test, I'll upload the patch with this modification.
So you'll also need to add a changelog for this test change.
Frédéric Wang (:fredw)
Comment 6
2016-03-04 07:34:41 PST
Created
attachment 272854
[details]
Patch Attached is a complete refactoring of RenderMathMLScript to avoid anonymous wrappers and flexboxes. There are a couple of things to adjust for mac tests, so I'll check that on Monday. But I'm already attaching for feedback.
Frédéric Wang (:fredw)
Comment 7
2016-03-07 23:51:32 PST
Created
attachment 273277
[details]
Patch OK, I checked the tests on mac. Now, I think it's ready for review.
Frédéric Wang (:fredw)
Comment 8
2016-03-11 00:12:14 PST
The timeout to wait web font in LayoutTests/mathml/mathml-in-html5/subsup-parameters-1.html is a bit hacky. I'll upload a new version later that uses document.fonts.ready (implemented in
bug 153348
).
Frédéric Wang (:fredw)
Comment 9
2016-03-11 01:24:57 PST
Created
attachment 273703
[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 10
2016-03-23 01:24:18 PDT
Comment on
attachment 273703
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=273703&action=review
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:415 > + return Optional<int>(base->top() + base->firstLineBaseline().value());
As said in the comment conversions between firstLineBaseline (int) and LayoutUnit (fractional pixel) is bad for the layout accuracy. However, a better rounding of the value here seems to improve the rendering: return Optional<int>(static_cast<int>(lroundf(base->top() + base->firstLineBaseline().value())));
Frédéric Wang (:fredw)
Comment 11
2016-03-31 08:18:28 PDT
Created
attachment 275286
[details]
Patch
Frédéric Wang (:fredw)
Comment 12
2016-04-01 06:10:32 PDT
Created
attachment 275399
[details]
Patch
Alejandro G. Castro
Comment 13
2016-04-07 09:55:36 PDT
Comment on
attachment 275399
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=275399&action=review
LGTM, it is a complex layout but with the refactor it is clear how it works. I've implemented part of the code so I'll let other reviewer to r+.
> Source/WebCore/ChangeLog:24 > + (WebCore::RenderMathMLScripts::getScriptMetricsAndLayoutIfNeeded): Helpef function to get the maximum ascent/descent of all the scripts and determine the minimal sub/sup shifts to apply.
Nit, Helpef -> Helper
Frédéric Wang (:fredw)
Comment 14
2016-04-08 04:38:14 PDT
Created
attachment 275993
[details]
Patch Thanks Alex. I'm uploading a new patch with the typo fix and other trivial changes (line wrap ~100 chars in changelog, use logical Width/Height, add link to
bug 156401
etc)
Frédéric Wang (:fredw)
Comment 15
2016-04-12 04:06:10 PDT
Created
attachment 276229
[details]
Patch Same patch, but just to check EWS...
Build Bot
Comment 16
2016-04-12 04:58:32 PDT
Comment on
attachment 276229
[details]
Patch
Attachment 276229
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/1141404
New failing tests: mathml/opentype/large-operators-italic-correction.html mathml/presentation/roots.xhtml mathml/presentation/multiscripts-positions.html mathml/presentation/scripts-subsup.html mathml/presentation/scripts-height.html
Build Bot
Comment 17
2016-04-12 04:58:37 PDT
Created
attachment 276230
[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Frédéric Wang (:fredw)
Comment 18
2016-04-12 06:50:19 PDT
Created
attachment 276232
[details]
Patch
Build Bot
Comment 19
2016-04-12 07:41:28 PDT
Comment on
attachment 276232
[details]
Patch
Attachment 276232
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/1142034
New failing tests: mathml/presentation/scripts-height.html
Build Bot
Comment 20
2016-04-12 07:41:32 PDT
Created
attachment 276233
[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Frédéric Wang (:fredw)
Comment 21
2016-04-12 08:36:50 PDT
Created
attachment 276237
[details]
Patch
Frédéric Wang (:fredw)
Comment 22
2016-04-14 12:24:11 PDT
Created
attachment 276411
[details]
Patch
Martin Robinson
Comment 23
2016-04-15 09:05:02 PDT
Comment on
attachment 276411
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=276411&action=review
Looks good to me, other than two issues and then a few nits.
> LayoutTests/mathml/mathml-in-html5/LICENSE:3 > +Tests in this directory have been imported from > +
https://github.com/MathML/MathMLinHTML5-tests
> +
We need to figure out if this license is compatible with WebKit and what the policy is going to be for importing mathml-in-html5 tests.
> Source/WebCore/css/mathml.css:104 > +/* This is a special style for erroneous markup */
Super nit: this sentence is missing a period.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:76 > + if (!base)
This function is really complicated! I think it may need some comments linking each decision to the spec and to decrease the complexity of the control.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:170 > + } break;
This should probably have its own line.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:172 > + default: > + ASSERT_NOT_REACHED();
This can be omitted if there is not other member in the enum.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:246 > + for (subScript = script; subScript && !isPrescript(*subScript); subScript = supScript->nextSiblingBox()) {
I think you can define subScript inside the for loop like: for (RenderBox* subScript =...)
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:247 > + supScript = subScript->nextSiblingBox();
This probably needs an ASSERT, since you rely on it later.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:357 > + for (subScript = firstPreScript; subScript; subScript = supScript->nextSiblingBox()) {
For each of these loops, it's probably a good idea to define subScript inside of the for loop: for (RenderBox* subScript =...)
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:358 > + supScript = subScript->nextSiblingBox();
You should probably ASSERT(supScript)
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:364 > + supScript = subScript->nextSiblingBox();
You should probably ASSERT(supScript).
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:371 > + supScript = subScript->nextSiblingBox();
Ditto.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:385 > + supScript = subScript->nextSiblingBox();
Ditto.
> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:399 > + default: > + ASSERT_NOT_REACHED(); > }
You can probably just remove this if there are no other members.
Frédéric Wang (:fredw)
Comment 24
2016-04-18 02:05:02 PDT
Comment on
attachment 276411
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=276411&action=review
>> LayoutTests/mathml/mathml-in-html5/LICENSE:3 >> + > > We need to figure out if this license is compatible with WebKit and what the policy is going to be for importing mathml-in-html5 tests.
OK, I found the tests from the Test The Web Forward project:
http://trac.webkit.org/browser/trunk/LayoutTests/imported/w3c/web-platform-tests
They use the same license and try and keep the structure of
https://github.com/w3c/web-platform-tests
So I'll basically do the same for
https://github.com/MathML/MathMLinHTML5-tests
>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:76 >> + if (!base) > > This function is really complicated! I think it may need some comments linking each decision to the spec and to decrease the complexity of the control.
OK, I think I'll be able to simplify the code and add many comments.
Frédéric Wang (:fredw)
Comment 25
2016-04-18 02:06:35 PDT
Created
attachment 276623
[details]
Patch @Martin: can you please check this new patch? Thanks!
Martin Robinson
Comment 26
2016-04-18 07:35:34 PDT
Comment on
attachment 276623
[details]
Patch That's a lot easier to read now. Thanks!
Frédéric Wang (:fredw)
Comment 27
2016-04-18 07:48:56 PDT
Committed
r199665
: <
http://trac.webkit.org/changeset/199665
>
Frédéric Wang (:fredw)
Comment 28
2016-04-18 08:23:40 PDT
Committed
r199666
: <
http://trac.webkit.org/changeset/199666
>
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