[webkit-reviews] review granted: [Bug 30926] Optimizations to Element::getAttribute : [Attachment 42154] patch 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jens Alfke <snej at chromium.org>'s
request for review:
Bug 30926: Optimizations to Element::getAttribute
https://bugs.webkit.org/show_bug.cgi?id=30926

Attachment 42154: patch 3
https://bugs.webkit.org/attachment.cgi?id=42154&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
You didn’t answer my question from the original review: "Since this is a
performance optimization, were you able to measure the speedup?"

> +    bool ignoreCase = shouldIgnoreAttributeCase(this);
> +    // Update the 'style' attribute if it's invalid and being requested:
> +    if (!m_isStyleAttributeValid && equalPossiblyIgnoringCase(name,
styleAttr.localName(), ignoreCase))
>	   updateStyleAttribute();

I would have put a blank line before the comment to make the paragraphing look
right.

> -    if (namedAttrMap)
> -	   if (Attribute* a = namedAttrMap->getAttributeItem(name,
shouldIgnoreAttributeCase(this)))
> -	       return a->value();
> +    if (namedAttrMap) {
> +	   Attribute* attribute = namedAttrMap->getAttributeItem(name,
ignoreCase);
> +	   if (attribute)
> +	       return attribute->value();
> +    }

You took the definition of the local variable out of the if statement. Why?

> +    return ignoreCase ? equalIgnoringCase(a,b) : (a == b);

Need a space after the comma.

Since the only comments here are a small number of trivial formatting things,
I'll say r=me