Bug 202720 - Remove unnecessary explicit parsing for mo@maxsize value "infinity"
Summary: Remove unnecessary explicit parsing for mo@maxsize value "infinity"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Delan Azabani
URL:
Keywords: InRadar
Depends on:
Blocks: mathml-core
  Show dependency treegraph
 
Reported: 2019-10-09 00:04 PDT by Frédéric Wang (:fredw)
Modified: 2020-04-09 00:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.62 KB, patch)
2020-04-06 06:26 PDT, Delan Azabani
no flags Details | Formatted Diff | Diff
Patch (5.40 KB, patch)
2020-04-08 08:44 PDT, Delan Azabani
no flags Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2020-04-08 23:28 PDT, Delan Azabani
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2019-10-09 00:04:04 PDT
We parse this but it is treated the same as invalid/default maxsize. I think we can remove it safely.
Comment 1 Frédéric Wang (:fredw) 2020-02-26 23:27:41 PST
(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
Comment 2 Delan Azabani 2020-04-06 06:26:43 PDT
Created attachment 395561 [details]
Patch
Comment 3 Rob Buis 2020-04-06 06:43:38 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2020-04-06 12:25:45 PDT
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?
Comment 5 Frédéric Wang (:fredw) 2020-04-06 12:26:34 PDT
(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.
Comment 6 Delan Azabani 2020-04-08 08:44:29 PDT
Created attachment 395816 [details]
Patch
Comment 7 Delan Azabani 2020-04-08 08:47:05 PDT
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?
Comment 8 Frédéric Wang (:fredw) 2020-04-08 09:58:52 PDT
(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?
Comment 9 Delan Azabani 2020-04-08 23:28:32 PDT
Created attachment 395916 [details]
Patch
Comment 10 Delan Azabani 2020-04-08 23:30:52 PDT
(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!
Comment 11 EWS 2020-04-09 00:57:21 PDT
Committed r259785: <https://trac.webkit.org/changeset/259785>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395916 [details].
Comment 12 Radar WebKit Bug Importer 2020-04-09 00:58:14 PDT
<rdar://problem/61502981>