Bug 153918 – Refactor RenderMathMLScripts layout function to avoid using flexbox

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Alejandro G. Castro]

Next patch in the series adding code to handle the layout of the scripts without the flexbox.

[Alejandro G. Castro]

Created attachment 270746 [details]
Patch

[Frédéric Wang (:fredw)]

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]

(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]

Created attachment 271165 [details]
Patch

[Frédéric Wang (:fredw)]

(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)]

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)]

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)]

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)]

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 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)]

Created attachment 275286 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 275399 [details]
Patch

[Alejandro G. Castro]

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)]

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)]

Created attachment 276229 [details]
Patch

Same patch, but just to check EWS...

[Build Bot]

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]

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)]

Created attachment 276232 [details]
Patch

[Build Bot]

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]

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)]

Created attachment 276237 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 276411 [details]
Patch

[Martin Robinson]

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 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)]

Created attachment 276623 [details]
Patch

@Martin: can you please check this new patch? Thanks!

[Martin Robinson]

Comment on attachment 276623 [details]
Patch

That's a lot easier to read now. Thanks!

[Frédéric Wang (:fredw)]

Committed r199665: <http://trac.webkit.org/changeset/199665>

[Frédéric Wang (:fredw)]

Committed r199666: <http://trac.webkit.org/changeset/199666>