Bug 156950 – Introduce MathOperator::OperatorType

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

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

Yet another piece from bug 152244.

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

Created attachment 277145 [details]
Patch

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

Created attachment 277232 [details]
Patch

Adding the ChangeLog info...

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

Created attachment 277474 [details]
Patch

Minor change: add the ASSERT to MathOperator::setGlyphAssembly now that we can exclude DisplayOperator type.

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

Created attachment 278391 [details]
Patch

Updating after https://trac.webkit.org/changeset/200569

[Manuel Rego Casasnovas]

Comment on attachment 278391 [details]
Patch

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

The patch seems good to me, just a few minor comments.

> Source/WebCore/rendering/mathml/MathOperator.cpp:80
> +    , m_operatorType(OperatorType::UndefinedOperator)

Can't you omit "OperatorType::"?
As the enum is defined by this class and the information is somehow redundant.

> Source/WebCore/rendering/mathml/MathOperator.cpp:117
> +    ASSERT(m_operatorType == OperatorType::DisplayOperator);

You're changing m_isVertical by VerticalOperator in the rest of the patch, except here.
What's the reason?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:386
> +    MathOperator::OperatorType type;
> +    if (isLargeOperatorInDisplayStyle())
> +        type = MathOperator::OperatorType::DisplayOperator;
> +    else if (m_isVertical)
> +        type = MathOperator::OperatorType::VerticalOperator;
> +    else
> +        type = MathOperator::OperatorType::HorizontalOperator;
> +    m_mathOperator.setOperator(m_textContent, type);

Maybe you can extract this to a helper method, as it's repeated twice
first in computePreferredLogicalWidths() and then here.

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

Comment on attachment 278391 [details]
Patch

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

>> Source/WebCore/rendering/mathml/MathOperator.cpp:80
>> +    , m_operatorType(OperatorType::UndefinedOperator)
> 
> Can't you omit "OperatorType::"?
> As the enum is defined by this class and the information is somehow redundant.

Darin suggested to use class enum for StretchType instead of old-style enum in a previous bug so I'm doing the same here. In that case, the prefix becomes mandatory.

>> Source/WebCore/rendering/mathml/MathOperator.cpp:117
>> +    ASSERT(m_operatorType == OperatorType::DisplayOperator);
> 
> You're changing m_isVertical by VerticalOperator in the rest of the patch, except here.
> What's the reason?

The client can stretch the operator in three different ways:

1) horizontally to match a target size
2) vertically to match a target size
3) as a display operator (e.g. big summation symbol) for which you don't have a target size.

I did not include the difference between 2) and 3) in previous bugs (both of them use vertical constructions internally) but that's the whole of point the present patch.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:386
>> +    m_mathOperator.setOperator(m_textContent, type);
> 
> Maybe you can extract this to a helper method, as it's repeated twice
> first in computePreferredLogicalWidths() and then here.

This is a temporary hack. In follow-up pieces of the MathOperator refactoring, setOperator will only be called once.

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

Created attachment 278604 [details]
Patch for EWS testing

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

Created attachment 278605 [details]
Patch for EWS testing

[Sergio Villar Senin]

Comment on attachment 278391 [details]
Patch

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

Looking good.

The only thing I'm missing is where m_isVertical is initialized, because I guess we should take into account the specific operator.

> Source/WebCore/ChangeLog:18
> +        (WebCore::MathOperator::setGlyphAssembly): Add an assert to ensure that the function is correctly use.

Nit: /use/used

> Source/WebCore/rendering/mathml/MathOperator.h:69
> +    LayoutUnit stretchSize() const { return m_operatorType == OperatorType::HorizontalOperator ? m_width : m_ascent + m_descent; }

Is valid to call stretchSize for UndefinedOperator or DisplayOperator?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:276
> +        type = MathOperator::OperatorType::HorizontalOperator;

We can simplify this a bit doing

else
    type = m_isVertical ? MathOperator::OperatorType::VerticalOperator : MathOperator::OperatorType::HorizontalOperator;

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:385
> +        type = MathOperator::OperatorType::HorizontalOperator;

Ditto.

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

Comment on attachment 278391 [details]
Patch

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

m_isVertical is initialized in existing code when the operator content is parsed.

>> Source/WebCore/rendering/mathml/MathOperator.h:69
>> +    LayoutUnit stretchSize() const { return m_operatorType == OperatorType::HorizontalOperator ? m_width : m_ascent + m_descent; }
> 
> Is valid to call stretchSize for UndefinedOperator or DisplayOperator?

No, I should probably add an ASSERT and do not invert the two directions.

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

Created attachment 278836 [details]
Patch

Addressing Sergio's review comments...

[Darin Adler]

Comment on attachment 278836 [details]
Patch

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

> Source/WebCore/rendering/mathml/MathOperator.cpp:80
> +    , m_operatorType(OperatorType::UndefinedOperator)

This initialization (and the rest of the initialization) should be done in the class definition where the data members are defined, rather than here in the destructor.

> Source/WebCore/rendering/mathml/MathOperator.h:42
> +    enum class OperatorType { UndefinedOperator, DisplayOperator, VerticalOperator, HorizontalOperator };

Given that these things are scoped to the class and to the enum also, I suggest:

    enum class Type { Undefined, Display, Vertical, Horizontal };

Otherwise, MathOperator::OperatorType::UndefinedOperator repeats the word “operator” three times. In the scope of the MathOperator class, Type is sufficiently specific and there is little risk of confusing it with, say, StretchType.

Using the longer names makes the code rather wordy.

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

Created attachment 278841 [details]
Patch

This addressed Darin's comments.

> This initialization (and the rest of the initialization) should be done in the class definition where the data members are defined, rather than here in the destructor.

When I get review comments on the big code move of bug 156921, I'll upload a new patch for bug 156921 including the change for the rest of the init.

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

Created attachment 280886 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 280886 [details]
Patch

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

> Source/WebCore/rendering/mathml/MathOperator.h:73
> +    }

If the method body is longer than 1 line it's typically not included here but instead defined inlined after the class definition in order to keep the class API clean.

> Source/WebCore/rendering/mathml/MathOperator.h:88
> +    Type m_operatorType = Type::UndefinedOperator;

I think Type m_operatorType { Type::UnderfinedOperator } is preferred nowadays.

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

Committed r201862: <http://trac.webkit.org/changeset/201862>