Bug 160336 – More cleanup of MathML operator parsing

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

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

Some cleanup are possible once the attribute parsing is moved into MathMLOperatorElement.

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

Created attachment 284858 [details]
Patch (applies after bugs 160245, 160239, 160190 and 160301)

[Darin Adler]

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;

Ditto.

> 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.

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

Created attachment 285107 [details]
Patch (applies after bug 160190 and bug 160301)

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

Created attachment 285126 [details]
Patch for EWS testing

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

Committed r204038: <http://trac.webkit.org/changeset/204038>