Bug 153987

Summary: Refactor RenderMathMLRoot layout function to avoid using flexbox
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bfulgham, cfleizach, commit-queue, darin, dbarton, esprehn+autocc, fred.wang, glenn, jdiggs, kondapallykalyan, mrobinson, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.mathml-association.org/MathMLinHTML5/S3.html#SS3.SSS3
Bug Depends on: 152244, 156836    
Bug Blocks: 133567, 134018, 134020, 143397, 153991, 155018, 155638, 157568, 159333    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
bfulgham: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch for EWS testing
buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch bfulgham: review+

Description Alejandro G. Castro 2016-02-08 08:14:57 PST
Next patch in the series refactoring the MathML code to remove flexbox dependency.
Comment 1 Alejandro G. Castro 2016-02-08 08:54:50 PST
Created attachment 270857 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2016-03-07 10:18:28 PST
Created attachment 273193 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2016-03-08 01:53:43 PST
Created attachment 273283 [details]
Patch
Comment 4 Frédéric Wang (:fredw) 2016-03-11 01:26:24 PST
Created attachment 273705 [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.
Comment 5 Frédéric Wang (:fredw) 2016-03-31 08:19:27 PDT
Created attachment 275288 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2016-04-01 06:11:16 PDT
Created attachment 275400 [details]
Patch
Comment 7 Brent Fulgham 2016-04-01 11:56:49 PDT
Comment on attachment 275400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275400&action=review

This looks like a very nice cleanup of a lot of complicated code. I can't approve it until I see passing tests, so please address the handful of minor comments I made, and upload a rebaselined patch. Thanks! r- due to the need to rebaseline.

> Source/WebCore/ChangeLog:12
> +        msqrt (row of children under a square root) is now implemented directly in RenderMathMLRoot, so RenderMathMLSquareRoot is removed and RenderMathMLRoot now inherits from RenderMathMLRow.

Could you please wrap these on ~100 character boundaries?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3784
> +    return children[0].get();

Nice! Much simpler!

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3796
> +    return children[1].get();

Ditto.

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:60
> +        if (is<RenderMathMLRow>(*renderer()) && !hasTagName(mrootTag))

This might be better as:

if (renderer() && is<RenderMathMLRow>(*renderer()) && !hasTagName(mrootTag))

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:105
> +    // FIXME: The layout of RenderMathMLMenclose should probably be rewritten from stratch to remove anonymous nodes and style change.

"rewritten from scratch"

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:114
> +    for (size_t i = 0; i < notationalValueSize; i++) {

This would be better as:

for (auto& notionalValue : notionalValues) {
    if (notionalValue == "circle")

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
> +    virtual void layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeight = 0) override;

Don't write virtual with override.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:-82
> -    ASSERT(!isEmpty());

Are you sure we want to lose this check? isValid() only checks isEmpty if it's not a square root. Is it perhaps impossible to call getBase on square roots?

Maybe is should be ASSERT(m_kind != SquareRoot)?

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:140
> +    // See http://wkb.ug/130326 and http://wkb.ug/155019

We usually use "http://webkit.org/b/130326". I'm not sure "wkb.ug" is a WebKit domain.
Comment 8 Frédéric Wang (:fredw) 2016-04-04 02:11:03 PDT
Comment on attachment 275400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275400&action=review

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:114
>> +    for (size_t i = 0; i < notationalValueSize; i++) {
> 
> This would be better as:
> 
> for (auto& notionalValue : notionalValues) {
>     if (notionalValue == "circle")

Yes, this is part of the old code and will go away later. This syntax should be used for bug 155019, though.

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
>> +    virtual void layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeight = 0) override;
> 
> Don't write virtual with override.

Oops, I forgot to update this after the style change.

>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:-82
>> -    ASSERT(!isEmpty());
> 
> Are you sure we want to lose this check? isValid() only checks isEmpty if it's not a square root. Is it perhaps impossible to call getBase on square roots?
> 
> Maybe is should be ASSERT(m_kind != SquareRoot)?

ASSERT(m_kind == RootWithIndex) is stronger than ASSERT(m_kind != SquareRoot) and so ensures that getBase is not called for m_kind == SquareRoot.

The syntax of m_kind == SquareRoot is <msqrt> child1 child2 child3 ... childN </msqrt> so getBase does not make sense in that case (despite what the a11y code does). The layout functions just relies on RenderMathMLRow's implementation and adjusts the metrics and position to add the radical symbol and overbar.
Comment 9 Frédéric Wang (:fredw) 2016-04-08 05:16:17 PDT
Created attachment 275997 [details]
Patch

This patch addressed Brent's comments and do other minor fixes (like adding link to FIXME comment, using logical height/width etc).

However, it still needs to be applied over the 4 other patches.
Comment 10 Frédéric Wang (:fredw) 2016-04-18 05:36:53 PDT
Created attachment 276632 [details]
Patch
Comment 11 Frédéric Wang (:fredw) 2016-04-26 08:54:15 PDT
Created attachment 277375 [details]
Patch
Comment 12 Frédéric Wang (:fredw) 2016-05-09 00:53:44 PDT
Created attachment 278395 [details]
Patch

Updating after https://trac.webkit.org/changeset/200569
Comment 13 Frédéric Wang (:fredw) 2016-05-11 03:56:36 PDT
Created attachment 278615 [details]
Patch for EWS testing
Comment 14 Build Bot 2016-05-11 04:55:01 PDT
Comment on attachment 278615 [details]
Patch for EWS testing

Attachment 278615 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1303287

New failing tests:
mathml/radical-fallback.html
mathml/presentation/roots.xhtml
Comment 15 Build Bot 2016-05-11 04:55:05 PDT
Created attachment 278616 [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.11.4
Comment 16 Frédéric Wang (:fredw) 2016-05-11 05:02:18 PDT
Created attachment 278617 [details]
Patch
Comment 17 Frédéric Wang (:fredw) 2016-06-17 02:46:40 PDT
Created attachment 281553 [details]
Patch
Comment 18 Build Bot 2016-06-17 03:48:23 PDT
Comment on attachment 281553 [details]
Patch

Attachment 281553 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1517389

New failing tests:
mathml/radical-fallback.html
Comment 19 Build Bot 2016-06-17 03:48:28 PDT
Created attachment 281555 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 20 Frédéric Wang (:fredw) 2016-06-17 07:42:03 PDT
Created attachment 281563 [details]
Patch

Adding the ios-simulator references for the radical fallback test.
Comment 21 Brent Fulgham 2016-06-17 09:07:14 PDT
Comment on attachment 281563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281563&action=review

So much deleted code! :-) Looks good. r=me.

> LayoutTests/TestExpectations:-120
> -

Yay!

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:243
> +    for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {

Probably could have been 'auto'
Comment 22 Frédéric Wang (:fredw) 2016-06-17 09:30:17 PDT
Committed r202168: <http://trac.webkit.org/changeset/202168>