Bug 138732 – Refactor RenderMathMLOperator to avoid relayout during layout

• Status: RESOLVED
• Resolution: INVALID
• Component: MathML

[Alejandro G. Castro]

The row layout code checks the stretch distance and this function updates style which causes a layout and width calculation. This should not be required just parsing the values when they change and dividing the updateStyle funcion in two, creating a calculateStretchDistance function.

[Alejandro G. Castro]

Created attachment 241963 [details]
Patch

[Alejandro G. Castro]

Created attachment 242170 [details]
Patch

[Alejandro G. Castro]

Updated patch, I found a better refactor of some parts and fixed a couple of errors in the previous code.

[Alejandro G. Castro]

Created attachment 242173 [details]
Patch

[Alejandro G. Castro]

Created attachment 242234 [details]
Patch

[Alejandro G. Castro]

Last modifications are improving the rationale of the layout process in the rows:

  - layout every children and calculate the stretch size height of that row based on the children that are not stretchy.
  - calculate the new stretch size for the children that are operators and if the stretch size changes layout again those children.

In other to do that we mark for layout and run the layout again. I still have doubts about the solution for the tables because apparently mtd tags are not implemented using inferred rows like proposed in the spec.

[Martin Robinson]

Comment on attachment 242234 [details]
Patch

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

Here are a couple comments about style, but when you get a moment I'd like to walk through the substantive changes with you.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1214
> +        return entry->lspace * style().font().size() / 18;

It looks like you could put style().font().size() / 18 into a helper function that returns the factor.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1228
> +const MathMLOperatorDictionary::Entry* RenderMathMLOperator::getDictionaryEntry()

getOperatorDictionaryEntry maybe?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1230
> +    if (m_textContent) {

Looks like this should be an early return?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1241
> +        const MathMLOperatorDictionary::Entry* entry = tryBinarySearch<const MathMLOperatorDictionary::Entry, MathMLOperatorDictionary::Key>(MathMLOperatorDictionary::dictionary, MATHML_OPDICT_SIZE, MathMLOperatorDictionary::Key(m_textContent, m_operatorForm), MathMLOperatorDictionary::ExtractKey);
> +        if (entry)
> +            return entry;

Could this be moved above the if (!isAnonymous()) block?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1244
> +        if (!explicitForm) {
> +            // If we did not find the desired operator form and if it was not set explicitely, we use the first one in the following order: Infix, Prefix, Postfix.

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1258
> +#undef MATHML_OPDICT_SIZE

Where is this defined? Perhaps it should be a const int?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1277
> +void RenderMathMLOperator::setOperatorFlagsFromOpDictEntry(const MathMLOperatorDictionary::Entry* entry, MathMLOperatorDictionary::Flag flag)

RenderMathMLOperator::setOperatorFlagsFromOpDictEntry -> RenderMathMLOperator::setOperatorFlagsFromOperatorDictionaryEntry

[Alejandro G. Castro]

Thanks for the comments martin, I'm checking the option to use a little bit more of the flexbox stretch to make the code clearer, I'll upload a patch after checking if it is possible with the changes you propose.

(In reply to comment #7)
> Comment on attachment 242234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242234&action=review
> 
> Here are a couple comments about style, but when you get a moment I'd like
> to walk through the substantive changes with you.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1214
> > +        return entry->lspace * style().font().size() / 18;
> 
> It looks like you could put style().font().size() / 18 into a helper
> function that returns the factor.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1228
> > +const MathMLOperatorDictionary::Entry* RenderMathMLOperator::getDictionaryEntry()
> 
> getOperatorDictionaryEntry maybe?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1230
> > +    if (m_textContent) {
> 
> Looks like this should be an early return?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1241
> > +        const MathMLOperatorDictionary::Entry* entry = tryBinarySearch<const MathMLOperatorDictionary::Entry, MathMLOperatorDictionary::Key>(MathMLOperatorDictionary::dictionary, MATHML_OPDICT_SIZE, MathMLOperatorDictionary::Key(m_textContent, m_operatorForm), MathMLOperatorDictionary::ExtractKey);
> > +        if (entry)
> > +            return entry;
> 
> Could this be moved above the if (!isAnonymous()) block?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1244
> > +        if (!explicitForm) {
> > +            // If we did not find the desired operator form and if it was not set explicitely, we use the first one in the following order: Infix, Prefix, Postfix.
> 
> Ditto.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1258
> > +#undef MATHML_OPDICT_SIZE
> 
> Where is this defined? Perhaps it should be a const int?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1277
> > +void RenderMathMLOperator::setOperatorFlagsFromOpDictEntry(const MathMLOperatorDictionary::Entry* entry, MathMLOperatorDictionary::Flag flag)
> 
> RenderMathMLOperator::setOperatorFlagsFromOpDictEntry ->
> RenderMathMLOperator::setOperatorFlagsFromOperatorDictionaryEntry

[Alejandro G. Castro]

After the proposal in https://lists.webkit.org/pipermail/webkit-dev/2015-December/027840.html, and the branch in https://github.com/alexgcastro/webkit/tree/MathMLLayout. This refactor does not make sense anymore.