Remove unnecessary explicit parsing for mo@maxsize value "infinity"

• Component: MathML
• Status: RESOLVED FIXED

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

We parse this but it is treated the same as invalid/default maxsize. I think we can remove it safely.

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

(In reply to Frédéric Wang (:fredw) from comment #0)
> We parse this but it is treated the same as invalid/default maxsize. I think
> we can remove it safely.

See MathMLOperatorElement::maxSize(), RenderMathMLOperator::minSize(), RenderMathMLBlock's toUserUnits. I think we can even remove LengthType::Infinity

[Delan Azabani]

Created attachment 395561 [details]
Patch

[Rob Buis]

Comment on attachment 395561 [details]
Patch

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

I think we have to double check with Fred if we can just remove this or make it dependent on the MatHMLCore runtime flag.

> Source/WebCore/ChangeLog:10
> +        the spec at some point in the future (mathml-refresh/mathml#107).

Using footnotes may be nicer.

> Source/WebCore/ChangeLog:13
> +        an invalid or missing value, so there shouldn't be any visible changes.

What about Firefox?

> Source/WebCore/mathml/MathMLOperatorElement.cpp:205
> +    m_maxSize = parseMathMLLength(value);

Nice to see some of this code go! I think there is no need for the value variable anymore.

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

Comment on attachment 395561 [details]
Patch

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

Maybe you should rename this bug "Remove unnecessary special parsing for mo@maxsize = 'infinite'" or similar. "infinite" is interpreted as "no maximum size" which is exactly the same as the default/invalid behavior, so there is no need for special parsing.

>> Source/WebCore/ChangeLog:10
>> +        the spec at some point in the future (mathml-refresh/mathml#107).
> 
> Using footnotes may be nicer.

Perhaps mention that mstyle@maxsize has never been supported by WebKit so "infinity" has never had any special effect.

>> Source/WebCore/ChangeLog:13
>> +        an invalid or missing value, so there shouldn't be any visible changes.
> 
> What about Firefox?

Everybody should treat them as invalid/unspecified, since "infinite" has always been treated as "no max size".

In the ChangeLog you should explain clearly (with the detailed steps) that we are indeed treated default/invalid behavior value as intMaxForLayoutUnit in the callers of MathMLOperatorElement::maxSize(), as that's not obvious from the patch (I had to re-read the code again).

>> Source/WebCore/mathml/MathMLOperatorElement.cpp:205
>> +    m_maxSize = parseMathMLLength(value);
> 
> Nice to see some of this code go! I think there is no need for the value variable anymore.

Probably even return cachedMathMLLength(MathMLNames::maxizeAttr, m_maxSize) should work now?

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

(In reply to Rob Buis from comment #3)
> I think we have to double check with Fred if we can just remove this or make
> it dependent on the MatHMLCore runtime flag.

Runtime flag is not necessary since there is no behavior change.

[Delan Azabani]

Created attachment 395816 [details]
Patch

[Delan Azabani]

Thanks for the feedback! I’ve updated the ChangeLog accordingly, but I’ll have to leave the code changes for tomorrow.

(In reply to Frédéric Wang (:fredw) from comment #4)
> Maybe you should rename this bug "Remove unnecessary special parsing for
> mo@maxsize = 'infinite'" or similar.

Renamed ChangeLog summary only, but I can’t find a way to rename the bug itself. Perhaps I can’t because I’m not the reporter?

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

(In reply to Delan Azabani from comment #7)
> Thanks for the feedback! I’ve updated the ChangeLog accordingly, but I’ll
> have to leave the code changes for tomorrow.
> 
> (In reply to Frédéric Wang (:fredw) from comment #4)
> > Maybe you should rename this bug "Remove unnecessary special parsing for
> > mo@maxsize = 'infinite'" or similar.
> 
> Renamed ChangeLog summary only, but I can’t find a way to rename the bug
> itself. Perhaps I can’t because I’m not the reporter?

I assigned the bug to you. Can you please try again?

[Delan Azabani]

Created attachment 395916 [details]
Patch

[Delan Azabani]

(In reply to Frédéric Wang (:fredw) from comment #4)
> Probably even return cachedMathMLLength(MathMLNames::maxizeAttr, m_maxSize)
> should work now?

(In reply to Frédéric Wang (:fredw) from comment #8)
> I assigned the bug to you. Can you please try again?

Done and done. Let me know what you think!

[EWS]

Committed r259785: <https://trac.webkit.org/changeset/259785>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395916 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61502981>