[webkit-reviews] review denied: [Bug 86780] Remove unnecessary overhead when setting id, class and style attributes. : [Attachment 142567] Pretty much a patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Andreas Kling <kling at webkit.org>'s
request for review:
Bug 86780: Remove unnecessary overhead when setting id, class and style
attributes.
https://bugs.webkit.org/show_bug.cgi?id=86780

Attachment 142567: Pretty much a patch
https://bugs.webkit.org/attachment.cgi?id=142567&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=142567&action=review


r- due those xhtml test failures. I think there is also room for optimizing bit
more.

> Source/WebCore/dom/StyledElement.cpp:146
>  {
> +    if (document()->isHTMLDocument()) {
> +	   // Fast paths for the "id", "class" and "style" attributes.
> +	   // They should never have to go through parseAttribute(), nor are
they presentational attributes.
> +
> +	   // FIXME: We could do this for all document types, if it weren't for
HTMLMapElement expecting to
> +	   //	     hear about the "id" attribute in parseAttribute() in XML
documents.

I'm confused. Are isHTMLDocument() and isXHTMLDocument() mutually exclusive?
Why do they have separate bit?

> Source/WebCore/dom/StyledElement.cpp:157
> +	   if (isIdAttributeName(attribute.name()))
> +	       return Element::attributeChanged(attribute);
> +	   if (attribute.name() == classAttr) {
> +	       classAttributeChanged(attribute.value());
> +	       return Element::attributeChanged(attribute);
> +	   }
> +	   if (attribute.name() == styleAttr) {
> +	       styleAttributeChanged(attribute.value());
> +	       return Element::attributeChanged(attribute);
> +	   }

Since we are optimizing here, it seems bit sad to call to the generic
Element::attributeChanged() which again does a bunch of similar branches. Add
Element::idAttributeChanged() etc perhaps?

> Source/WebCore/html/HTMLElement.cpp:-217
>  void HTMLElement::parseAttribute(const Attribute& attribute)
>  {
> -    if (isIdAttributeName(attribute.name()) || attribute.name() == classAttr
|| attribute.name() == styleAttr)
> -	   return StyledElement::parseAttribute(attribute);
> -

Would be helpful to assert that we never get here with any of these?