Bug 156950 - Introduce MathOperator::OperatorType
Summary: Introduce MathOperator::OperatorType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 156906 156910 156913 156921
Blocks: 152244 157071
  Show dependency treegraph
 
Reported: 2016-04-23 01:30 PDT by Frédéric Wang (:fredw)
Modified: 2016-06-09 07:48 PDT (History)
10 users (show)

See Also:


Attachments
Patch (8.95 KB, patch)
2016-04-23 01:33 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (11.40 KB, patch)
2016-04-25 01:15 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2016-04-27 06:25 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (12.32 KB, patch)
2016-05-09 00:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS testing (77.68 KB, patch)
2016-05-11 02:16 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS testing (77.55 KB, patch)
2016-05-11 02:18 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2016-05-13 05:14 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (11.84 KB, patch)
2016-05-13 09:31 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (11.84 KB, patch)
2016-06-08 23:24 PDT, Frédéric Wang (:fredw)
svillar: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-04-23 01:30:33 PDT
Yet another piece from bug 152244.
Comment 1 Frédéric Wang (:fredw) 2016-04-23 01:33:18 PDT
Created attachment 277145 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2016-04-25 01:15:51 PDT
Created attachment 277232 [details]
Patch

Adding the ChangeLog info...
Comment 3 Frédéric Wang (:fredw) 2016-04-27 06:25:13 PDT
Created attachment 277474 [details]
Patch

Minor change: add the ASSERT to MathOperator::setGlyphAssembly now that we can exclude DisplayOperator type.
Comment 4 Frédéric Wang (:fredw) 2016-05-09 00:51:35 PDT
Created attachment 278391 [details]
Patch

Updating after https://trac.webkit.org/changeset/200569
Comment 5 Manuel Rego Casasnovas - OOO (back 1st Sep) 2016-05-10 03:50:40 PDT
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.
Comment 6 Frédéric Wang (:fredw) 2016-05-10 04:13:49 PDT
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.
Comment 7 Frédéric Wang (:fredw) 2016-05-11 02:16:25 PDT
Created attachment 278604 [details]
Patch for EWS testing
Comment 8 Frédéric Wang (:fredw) 2016-05-11 02:18:16 PDT
Created attachment 278605 [details]
Patch for EWS testing
Comment 9 Sergio Villar Senin 2016-05-13 02:05:01 PDT
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.
Comment 10 Frédéric Wang (:fredw) 2016-05-13 02:12:44 PDT
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.
Comment 11 Frédéric Wang (:fredw) 2016-05-13 05:14:42 PDT
Created attachment 278836 [details]
Patch

Addressing Sergio's review comments...
Comment 12 Darin Adler 2016-05-13 08:43:17 PDT
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.
Comment 13 Frédéric Wang (:fredw) 2016-05-13 09:31:39 PDT
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.
Comment 14 Frédéric Wang (:fredw) 2016-06-08 23:24:13 PDT
Created attachment 280886 [details]
Patch
Comment 15 Sergio Villar Senin 2016-06-09 07:08:53 PDT
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.
Comment 16 Frédéric Wang (:fredw) 2016-06-09 07:48:59 PDT
Committed r201862: <http://trac.webkit.org/changeset/201862>