Bug 138732 - Refactor RenderMathMLOperator to avoid relayout during layout
Summary: Refactor RenderMathMLOperator to avoid relayout during layout
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords:
Depends on:
Blocks: 133845
  Show dependency treegraph
 
Reported: 2014-11-14 02:09 PST by Alejandro G. Castro
Modified: 2016-02-05 03:23 PST (History)
11 users (show)

See Also:


Attachments
Patch (28.98 KB, patch)
2014-11-20 10:51 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (32.25 KB, patch)
2014-11-24 13:02 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (32.14 KB, patch)
2014-11-24 14:14 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (32.09 KB, patch)
2014-11-26 10:31 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2014-11-14 02:09:16 PST
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.
Comment 1 Alejandro G. Castro 2014-11-20 10:51:56 PST
Created attachment 241963 [details]
Patch
Comment 2 Alejandro G. Castro 2014-11-24 13:02:17 PST
Created attachment 242170 [details]
Patch
Comment 3 Alejandro G. Castro 2014-11-24 13:03:35 PST
Updated patch, I found a better refactor of some parts and fixed a couple of errors in the previous code.
Comment 4 Alejandro G. Castro 2014-11-24 14:14:31 PST
Created attachment 242173 [details]
Patch
Comment 5 Alejandro G. Castro 2014-11-26 10:31:30 PST
Created attachment 242234 [details]
Patch
Comment 6 Alejandro G. Castro 2014-11-26 10:39:29 PST
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.
Comment 7 Martin Robinson 2014-12-07 08:08:24 PST
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
Comment 8 Alejandro G. Castro 2015-01-14 00:51:03 PST
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
Comment 9 Alejandro G. Castro 2016-02-05 03:23:21 PST
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.