Bug 161371 – More refactoring of RenderMathMLScripts

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

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

- Make clear that getBaseAndScripts verify the validity, store reference children into a struct and improve the browsing of the child list.
- Split getScriptMetricsAndLayoutIfNeeded into three separate parts (see bug 161084 comment 4)

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

Created attachment 287379 [details]
Patch

[Javier Fernandez]

Comment on attachment 287379 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:179
> +    if (!validateAndGetReferenceChildren(reference))

We only use this method to create a valid instance of ReferenceChildren. Why not just define the typical "create" function, which returns a pointer ? In case of no valid reference, then we will just get nullptr.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:222
> +RenderMathMLScripts::VerticalParameters RenderMathMLScripts::verticalParameters() const

do you need the class name in the return type ?

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:224
> +    VerticalParameters parameters;

The lines to get each parameter are indeed too long, but perhaps you can find a way to implement it clear and use static initializers, like this:

return { baseAscent, baseDescent, ... }

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:251
> +    VerticalParameters parameters = verticalParameters();

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:303
> +        auto subScript = reference.firstPostScript;

suggestion: why not in a  single line ? 

   auto subscript = reference.firstPostScript ? reference.firstPostScript : reference.firstPreScript;

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:356
> +    if (!validateAndGetReferenceChildren(reference)) {

Ditto.

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

Comment on attachment 287379 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:179
>> +    if (!validateAndGetReferenceChildren(reference))
> 
> We only use this method to create a valid instance of ReferenceChildren. Why not just define the typical "create" function, which returns a pointer ? In case of no valid reference, then we will just get nullptr.

So I believe the idea would be to return an Optional<ReferenceChildren> where Nullopt means a failure. I considered that but then validateAndGetReferenceChildren needed a bit more refactoring and that made the use of "reference" elsewhere more verbose, so I suspected it would complicate the readability/review.

>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:222
>> +RenderMathMLScripts::VerticalParameters RenderMathMLScripts::verticalParameters() const
> 
> do you need the class name in the return type ?

Yes.

>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:224
>> +    VerticalParameters parameters;
> 
> The lines to get each parameter are indeed too long, but perhaps you can find a way to implement it clear and use static initializers, like this:
> 
> return { baseAscent, baseDescent, ... }

Not sure this will work very well here.

>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:251
>> +    VerticalParameters parameters = verticalParameters();
> 
> Ditto.

I guess you mean initializing the VerticalMetrics below with { 0, 0, 0, 0 }?

>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:303
>> +        auto subScript = reference.firstPostScript;
> 
> suggestion: why not in a  single line ? 
> 
>    auto subscript = reference.firstPostScript ? reference.firstPostScript : reference.firstPreScript;

Yes I can do that, thanks for the suggestion.

[Darin Adler]

Comment on attachment 287379 [details]
Patch

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

>>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:179
>>> +    if (!validateAndGetReferenceChildren(reference))
>> 
>> We only use this method to create a valid instance of ReferenceChildren. Why not just define the typical "create" function, which returns a pointer ? In case of no valid reference, then we will just get nullptr.
> 
> So I believe the idea would be to return an Optional<ReferenceChildren> where Nullopt means a failure. I considered that but then validateAndGetReferenceChildren needed a bit more refactoring and that made the use of "reference" elsewhere more verbose, so I suspected it would complicate the readability/review.

You can write it like this:

    auto possibleReference = validateAndGetReferenceChildren();
    if (!possibleReference)
        return;
    auto& reference = possibleReference.value();

There is no need to make the code below more verbose. The only annoying thing is the need to have two different names for the local variable. I think the idea is a good one. We should move away from out arguments and also move away from function interfaces where we return multiple independent values, but you should not look at one based on the other.

>>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:222
>>> +RenderMathMLScripts::VerticalParameters RenderMathMLScripts::verticalParameters() const
>> 
>> do you need the class name in the return type ?
> 
> Yes.

If you want to avoid it you can use the "auto" syntax:

    auto RenderMathMLScripts::verticalParameters() const -> VerticalParameters

>>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:224
>>> +    VerticalParameters parameters;
>> 
>> The lines to get each parameter are indeed too long, but perhaps you can find a way to implement it clear and use static initializers, like this:
>> 
>> return { baseAscent, baseDescent, ... }
> 
> Not sure this will work very well here.

I think it’s worth, at some point, investigating alternate coding patterns. It’s not great to have all this boilerplate code.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:257
> +    VerticalMetrics metrics;
> +    metrics.subShift = 0;
> +    metrics.supShift = 0;
> +    metrics.descent = 0;
> +    metrics.ascent = 0;

Yes, = { 0, 0, 0, 0 } would be very nice here.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:284
> +        LayoutUnit subAscent = ascentForChild(*reference.firstPostScript);
> +        LayoutUnit subDescent = reference.firstPostScript->logicalHeight() - subAscent;

I like auto better than LayoutUnit for these.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:292
> +        LayoutUnit supAscent = ascentForChild(*reference.firstPostScript);
> +        LayoutUnit supDescent = reference.firstPostScript->logicalHeight() - supAscent;

And these. And lots more places below. Not sure everyone agrees though.

>> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:356
>> +    if (!validateAndGetReferenceChildren(reference)) {
> 
> Ditto.

I agree that the Optional trick would be cleaner here.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:61
> +    struct ReferenceChildren {
> +        RenderBox* base;
> +        RenderBox* prescriptDelimiter;
> +        RenderBox* firstPostScript;
> +        RenderBox* firstPreScript;
> +    };

This struct doesn’t have to be in the header. It can be forward declared here in the class definition:

    struct ReferenceChildren;

And then in the .cpp file:

    struct RenderMathMLScripts::ReferenceChildren {
        ...
    };

As long as the code here simply declares functions using the type and doesn’t actually contain code that uses the type, which is the case here.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:74
> +    struct VerticalParameters {
> +        LayoutUnit subscriptShiftDown;
> +        LayoutUnit superscriptShiftUp;
> +        LayoutUnit subscriptBaselineDropMin;
> +        LayoutUnit superScriptBaselineDropMax;
> +        LayoutUnit subSuperscriptGapMin;
> +        LayoutUnit superscriptBottomMin;
> +        LayoutUnit subscriptTopMax;
> +        LayoutUnit superscriptBottomMaxWithSubscript;
> +    };

Same thing here.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:81
> +    struct VerticalMetrics {
>          LayoutUnit subShift;
>          LayoutUnit supShift;
>          LayoutUnit ascent;
>          LayoutUnit descent;
>      };

Same thing here.

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

Created attachment 287941 [details]
Patch

> The lines to get each parameter are indeed too long, but perhaps you can find a way to implement it clear and use static initializers, like this:
> I like auto better than LayoutUnit for these.
> This struct doesn’t have to be in the header.

The changeset of this patch is already big, these three points are not specific to scripts and should be considered for other MathML classes too and I won't be able to come back to this soon. So I'll leave them for another patch.

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

Committed r205441: <http://trac.webkit.org/changeset/205441>