Bug 160336

Summary: More cleanup of MathML operator parsing
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Severity: Normal CC: alex, bfulgham, commit-queue, dbarton, esprehn+autocc, glenn, jfernandez, kondapallykalyan, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 160301    
Bug Blocks: 156537, 160339    
Description Flags
Patch (applies after bugs 160245, 160239, 160190 and 160301)
darin: review+
Patch (applies after bug 160190 and bug 160301)
Patch for EWS testing none

Description Frédéric Wang (:fredw) 2016-07-29 03:20:57 PDT
Some cleanup are possible once the attribute parsing is moved into MathMLOperatorElement.
Comment 1 Frédéric Wang (:fredw) 2016-07-29 03:45:28 PDT
Created attachment 284858 [details]
Patch (applies after bugs 160245, 160239, 160190 and 160301)
Comment 2 Darin Adler 2016-07-31 21:33:39 PDT
Comment on attachment 284858 [details]
Patch (applies after bugs 160245, 160239, 160190 and 160301)

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

> Source/WebCore/mathml/MathMLOperatorElement.cpp:163
> +    return m_properties.flags & flag;

The caller already knows how to mask off the flag and in fact has code to do that in the non-dirty case. So the parse function should probably just set the flags and not try to return a boolean.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:73
> +    UChar text = textContent();

The local variable name should be "character", not "text", I think.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:74
> +    return 0x2061 <= text && text <= 0x2064;

A function like this needs a comment to indicate why the range U+2061-U+2064 is correct, otherwise the code is mysterious. In particular, what guarantees these are the only 4 invisible operators.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:89
> +    if (leadingSpace < 0)
> +        leadingSpace = 0;
> +    return leadingSpace;

Typically we like to write this as an expression rather than an if. If it was an integer it would be:

    return std::max(0, leadingSpace);

Maybe we can find a similar idiom for LayoutUnit?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:94
> +   // FIXME: Negative trailing spaces must be implemented (https://webkit.org/b/124830).

Missing space here, incorrect indentation.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:152
> +            aspect = float(minSizeValue) / size;

We usually try to steer clear of function call style type casting for scalars. Here I would suggest:

    aspect = minSizeValue.toFloat() / size;

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:156
> +                aspect = float(maxSizeValue) / size;


> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:81
>      // The following operators are invisible: U+2061 FUNCTION APPLICATION, U+2062 INVISIBLE TIMES, U+2063 INVISIBLE SEPARATOR, U+2064 INVISIBLE PLUS.

This comment needs to move in so it’s with the isInvisibleOperator function body; does not belong in the header.
Comment 3 Frédéric Wang (:fredw) 2016-08-02 06:59:33 PDT
Created attachment 285107 [details]
Patch (applies after bug 160190 and bug 160301)
Comment 4 Frédéric Wang (:fredw) 2016-08-02 12:47:02 PDT
Created attachment 285126 [details]
Patch for EWS testing
Comment 5 Frédéric Wang (:fredw) 2016-08-02 13:27:33 PDT
Committed r204038: <http://trac.webkit.org/changeset/204038>