Bug 74885 – Rename Element::setAttributeMap to parserSetAttributeMap and limit its use to the parser

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Rename Element::setAttributeMap to parserSetAttributeMap and limit its use to the parser

[Adam Klein]

Created attachment 119917 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 119917 [details]
Patch

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

This looks great in general. Might want abarth or eseidel to have a look as well.

> Source/WebCore/html/HTMLViewSourceDocument.cpp:82
> +    div->setAttribute(classAttr, "webkit-line-gutter-backdrop");

Maybe we should define static local AtomicString for these?

> Source/WebCore/html/track/WebVTTParser.cpp:365
> +            if (m_token.classes().size() > 0)
> +                child->setAttribute(classAttr, AtomicString(m_token.classes().data(), m_token.classes().size()));
>              if (child->hasTagName(qTag))
> -                child->setAttribute(titleAttr, String(m_token.annotation().data(), m_token.annotation().size()));
> +                child->setAttribute(titleAttr, AtomicString(m_token.annotation().data(), m_token.annotation().size()));

Why don't we use parserSetAttributeMap here?  I guess because NamedNodeMap had not been allocated yet? If so, can we assert that?

[Ryosuke Niwa]

I mean *have abarth or eseidel take a look*

[Adam Klein]

Comment on attachment 119917 [details]
Patch

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

>> Source/WebCore/html/HTMLViewSourceDocument.cpp:82
>> +    div->setAttribute(classAttr, "webkit-line-gutter-backdrop");
> 
> Maybe we should define static local AtomicString for these?

I doubt this code is performance-sensitive enough to matter...

>> Source/WebCore/html/track/WebVTTParser.cpp:365
>> +                child->setAttribute(titleAttr, AtomicString(m_token.annotation().data(), m_token.annotation().size()));
> 
> Why don't we use parserSetAttributeMap here?  I guess because NamedNodeMap had not been allocated yet? If so, can we assert that?

Assert what? The reason setAttributeMap is useful in HTMLConstructionSite is that the NamedNodeMap is allocated by a different class (AtomicHTMLToken).  Sure, we could change this code to create a named node map, initialize it, and then pass it in, but it's not clear why that would be any more efficient than just calling setAttribute directly.

[Adam Barth]

Comment on attachment 119917 [details]
Patch

Looks great.

[Adam Klein]

Committed r103340: <http://trac.webkit.org/changeset/103340>