[webkit-reviews] review denied: [Bug 30926] Optimizations to Element::getAttribute : [Attachment 42140] oops, correct patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 42140: oops, correct patch
https://bugs.webkit.org/attachment.cgi?id=42140&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Patches for review need change log entries in the patch. Please don't put them
up for review without the change log entry since it makes extra work for
reviewers.

Since this is a performance optimization, were you able to measure the speedup?


> Index: WebCore/dom/Element.cpp
> ===================================================================
> --- WebCore/dom/Element.cpp	(revision 50224)
> +++ WebCore/dom/Element.cpp	(working copy)
> @@ -488,9 +488,12 @@ static inline bool shouldIgnoreAttribute
>  
>  const AtomicString& Element::getAttribute(const String& name) const
>  {
> -    String localName = shouldIgnoreAttributeCase(this) ? name.lower() :
name;
> -    if (localName == styleAttr.localName() && !m_isStyleAttributeValid)
> -	   updateStyleAttribute();
> +    bool ignoreCase = shouldIgnoreAttributeCase(this);
> +    // Update the 'style' attribute if it's invalid and being requested:
> +    if (!m_isStyleAttributeValid)
> +	   if (ignoreCase ? equalIgnoringCase(name, styleAttr.localName())
> +			  : (name == styleAttr.localName()))
> +	       updateStyleAttribute();

Multiple line if statement body needs braces to match WebKit style guide.

I also think you should avoid the unusual formatting here -- we frown on lining
code up like the "?" and ":" here since it's high maintenance (has to be
reformatted if you rename things).

One way to avoid that would be to put the comparison into an inline function
that takes a boolean:

    static bool equalPossiblyIgnoringCase(const String& a, const AtomicString&
b, bool ignoreCase)
    {
	if (ignoreCase)
	    return equalIgnoringCase(a, b);
	return a == b;
    }

Then you could change things back to a single line if,.

    if (!m_isStyleAttributeValue && equalPossiblyIgnoringCase(name,
styleAttr.localName(), ignoreCase))
	updateStyleAttribute();

Maybe you like this, or maybe you will find some other way of eliminating the
unusual formatting, or maybe you will want to start a thread on webkit-dev
suggesting we do this style of formatting ;-)

>      if (namedAttrMap)
> -	   if (Attribute* a = namedAttrMap->getAttributeItem(name,
shouldIgnoreAttributeCase(this)))
> +	   if (Attribute* a = namedAttrMap->getAttributeItem(name, ignoreCase))

>	       return a->value();

Since you're touching this code, you should add the braces we would have used
if writing this code anew, and give the local variable a name that's a word
instead of a letter.

> +	       // FIXME: Would be faster to do this test without generating a
temporary string.

I think the FIXME needs to explain why you didn't do it. Maybe just wording it
this way: "Would be faster if we could compare a QualifiedName to a string
without generating a temporary string."