Bug 160461 – Use memoize pattern for the menclose notation attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

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

Currently, MathMLMencloseElement::parseAttribute parse the "notation" attribute each time it is changed. We should follow what is done for other MathML element classes and store m_notationFlags in an Optional<unsigned short> where null means that the attribute is dirty and must be parsed again.

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

Created attachment 285206 [details]
Patch

[Darin Adler]

Comment on attachment 285206 [details]
Patch

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

> Source/WebCore/mathml/MathMLMencloseElement.cpp:66
> +    String(value).split(' ', notationsList);

Should write:

    value.string().split(' ', notationsList);

Also kind of sad to allocate a vector and then allocate lots of memory just to examine substrings of the passed in string, which could be done much more efficiently. WebKit project should come up with a better way to do this without allocating all the strings.

Do we really want to split only on actual spaces and not other kinds of HTML spaces? Do we have test cases covering that?

[Darin Adler]

Comment on attachment 285206 [details]
Patch

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

> Source/WebCore/mathml/MathMLMencloseElement.h:64
> +    Optional<unsigned short> m_notationFlags;

Maybe uint16_t instead?

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

(In reply to comment #2)
> Should write:
> 
>     value.string().split(' ', notationsList);
> 
> Also kind of sad to allocate a vector and then allocate lots of memory just
> to examine substrings of the passed in string, which could be done much more
> efficiently. WebKit project should come up with a better way to do this
> without allocating all the strings.
> 
> Do we really want to split only on actual spaces and not other kinds of HTML
> spaces? Do we have test cases covering that?

I've been preserving the code of the person who has implemented menclose but indeed we could parse the list more efficiently by skipping whitespace and checking substrings. Not sure whether that should be done in that bug. Also, I think any MathML whitespace should be accepted (which unfortunately is defined differently than in HTML space see http://lists.w3.org/Archives/Public/www-math/2016Aug/0000.html).

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

Committed r204721: <http://trac.webkit.org/changeset/204721>