Bug 156913

Summary: RenderMathMLOperator: refactor management of stretchy data and italic correction
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, mrobinson, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 156906, 156910    
Bug Blocks: 152244, 156921, 156950, 157071    
Attachments:
Description Flags
Patch (for EWS testing)
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Frédéric Wang (:fredw) 2016-04-22 09:14:40 PDT
One more step towards bug 152244.
Comment 1 Frédéric Wang (:fredw) 2016-04-22 09:34:05 PDT
Created attachment 277066 [details]
Patch (for EWS testing)
Comment 2 Frédéric Wang (:fredw) 2016-04-22 09:36:45 PDT
Created attachment 277067 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2016-04-22 13:06:58 PDT
Comment on attachment 277067 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:552
>                  if (size >= stretchSize()) 

this should be targetSize
Comment 4 Frédéric Wang (:fredw) 2016-04-25 00:49:04 PDT
Created attachment 277230 [details]
Patch

Update patch after http://trac.webkit.org/changeset/199978
Comment 5 Frédéric Wang (:fredw) 2016-04-27 05:56:13 PDT
Created attachment 277471 [details]
Patch

Minor change: the removal of the character parameter of findStretchyData is done in bug 156910.
Comment 6 Alejandro G. Castro 2016-05-06 05:42:14 PDT
Comment on attachment 277471 [details]
Patch

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

The refactor looks good, just some minor points.

> Source/WebCore/ChangeLog:10
> +        We use union to simplify the structure used to manage the glyph assembly and the type of stretching.

I would say the union does not simplify, it saves memory and explains we can not use both members at the same time. Is it like this? I do not see them in WebKit that frequently but in this case it makes sense to add meaning about just one of the options is valid.

> Source/WebCore/ChangeLog:11
> +        We also modify the signature of some functions to retrieve display operator and stretchy data.

Can you add the reason to modify the signature of these functions?

> Source/WebCore/ChangeLog:21
> +        (WebCore::RenderMathMLOperator::findDisplayStyleLargeOperator): We rename the function and
> +        change its signature to remove the return value.
> +        We now also set the italic correction when a display operator is found.

Join the lines that are about the same function.

> Source/WebCore/ChangeLog:37
> +        (WebCore::RenderMathMLOperator::italicCorrection): Deleted.

The function is not deleted but moved to the header.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:308
> +        findDisplayStyleLargeOperator();

If I understood correctly this function I'd rather use a name such as: calculateLargeOperatorDisplayStyle.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:385
> +    ASSERT(sizeVariant.isValid() && sizeVariant.font->mathData());

Is the mathData assertion checked every time we call this function?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:396
> +bool RenderMathMLOperator::getGlyphAssemblyFallBack(Vector<OpenTypeMathData::AssemblyPart> assemblyParts, GlyphAssemblyData& assemblyData) const

I would use again calculate in the name of the function.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:498
> +void RenderMathMLOperator::findDisplayStyleLargeOperator()

See previous comment about the name of the function.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:504
>      if (!getBaseGlyph(style(), baseGlyph) || !baseGlyph.font->mathData())
> -        return data;
> +        return;

Are we sure the values are the same we got when returning data?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:524
> +void RenderMathMLOperator::findStretchyData(float* maximumGlyphWidth, LayoutUnit targetSize)

I would use calculate again here.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:89
> +    struct GlyphAssemblyData {
> +        union {
> +            GlyphData top;
> +            GlyphData right;
> +        };
> +        GlyphData extension;
> +        union {
> +            GlyphData bottom;
> +            GlyphData left;
> +        };
> +        GlyphData middle;
> +    GlyphAssemblyData()
> +    : top(), extension(), bottom(), middle() { }
>      };

We usually put the constructor as the first member in the structs, also use one line per field initialized. Check other definitions for reference.
Comment 7 Frédéric Wang (:fredw) 2016-05-06 06:24:45 PDT
Comment on attachment 277471 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        We use union to simplify the structure used to manage the glyph assembly and the type of stretching.
> 
> I would say the union does not simplify, it saves memory and explains we can not use both members at the same time. Is it like this? I do not see them in WebKit that frequently but in this case it makes sense to add meaning about just one of the options is valid.

The old code already uses a shared m_data array and some member functions of different names. So the memory management & presentation of members do not really change: it's just becomes a simple struct instead of a more complex class. And yes, there are already ASSERT checking that the we correctly call the members depending of direction / kind of structure.

>> Source/WebCore/ChangeLog:11
>> +        We also modify the signature of some functions to retrieve display operator and stretchy data.
> 
> Can you add the reason to modify the signature of these functions?

Yes I'll do that. It's just because in the MathOperator class we will directly set the members instead of returning them.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:385
>> +    ASSERT(sizeVariant.isValid() && sizeVariant.font->mathData());
> 
> Is the mathData assertion checked every time we call this function?

Currently this function is called in two places where sizeVariant.font is set to the font that was used to retrieve sizeVariant.glyph value... from the MATH table. So font->mathData() is checked before the use.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:396
>> +bool RenderMathMLOperator::getGlyphAssemblyFallBack(Vector<OpenTypeMathData::AssemblyPart> assemblyParts, GlyphAssemblyData& assemblyData) const
> 
> I would use again calculate in the name of the function.

I'm not sure I understand the subtility... At the end the goal is to set the variant/assembly in the MathOperator class so I see that set/calculate is better for findDisplayOperator & findStretchyData. However, here the use is different: We only convert the MATH table's GlyphAssembly structure into our fallback GlyphAssemblyData structure (the assembly is only set in findStretchyData).

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:504
>> +        return;
> 
> Are we sure the values are the same we got when returning data?

This would return an unstretched data which is how it is initialized in RenderMathMLOperator::updateStyle.
Comment 8 Frédéric Wang (:fredw) 2016-05-06 06:55:57 PDT
Created attachment 278250 [details]
Patch
Comment 9 Darin Adler 2016-05-07 15:19:04 PDT
Comment on attachment 278250 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:78
> +        GlyphAssemblyData()
> +        : top(), extension(), bottom(), middle() { }

This formatting is not so good. Either put it all one one line, or indent the second line please.

I don’t think the explicit extension and middle initialization is needed.

What happens if you omit this constructor? Do we get a compile time error? Is there some way to get the compiler to generate this without us.

I suggest considering omitting these unions since the types are the same. Just:

    GlyphData topOrRight;

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:94
> +    enum StretchType {
> +        Unstretched,
> +        SizeVariant,
> +        GlyphAssembly
> +    };

I think this usually looks better when all on a single line. Also worth considering using an enum class rather than old-style enum.
Comment 10 Darin Adler 2016-05-07 15:19:50 PDT
Comment on attachment 278250 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:118
> +    bool calculateGlyphAssemblyFallBack(Vector<OpenTypeMathData::AssemblyPart>, GlyphAssemblyData&) const;

Peculiar to take a Vector as an argument. Usually it would be const Vector& or Vector&&.
Comment 11 Frédéric Wang (:fredw) 2016-05-08 22:51:00 PDT
Committed r200569: <http://trac.webkit.org/changeset/200569>
Comment 12 Frédéric Wang (:fredw) 2016-05-08 23:07:42 PDT
OK, I've removed the union for GlyphData and used the enum class.

The comment about Vector was actually already handled in bug 156921, apparently I forgot it when I split the patch.