[webkit-reviews] review denied: [Bug 20247] setAttributeNode() does not work when attribute name has a capital letter in it : [Attachment 22938] Proposed fix: add bool to getAttributeItem to tune case sensitivity

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 20247: setAttributeNode() does not work when attribute name has a capital
letter in it
https://bugs.webkit.org/show_bug.cgi?id=20247

Attachment 22938: Proposed fix: add bool to getAttributeItem to tune case
sensitivity
https://bugs.webkit.org/attachment.cgi?id=22938&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks like a great fix!

 10	    Add a boolean parameter to getAttributeItem to choose between case
sensitive and case insisitive

Typo here. It should be "insensitive".

 177		 if (name == (shouldIgnoreAttributeCase ?
m_attributes[i]->name().toString().lower() :
m_attributes[i]->name().toString()))

Calling lower() is a slower way to do a case insensitive comparison that often
allocates memory. A faster way is to call equalIgnoringCase.

 71	Attribute* getAttributeItem(const String& name, bool
shouldIgnoreAttributeCase = false) const;

Can we do without the default value? How many callers are there that rely on
the default?

What's the performance impact of this change? The function seems simple enough
that I could imagine having multiple copies of it if there are callers who have
no reason to pass the boolean, but on the other hand it's possibly slow enough
already that this extra work has little impact.

 82 function testAttribNodeNamePreservesCaseGetNode2() {

I'd prefer that even JavaScript follow our coding guidelines and put braces on
a separate line for function definitions.

 92	if (!a) {
 93	    return "FAIL";
 94	}

And omit braces for one line if statements.

review- because of the equalIgnoringCase issue; please consider the other
comments too.