setAttributeNode() does not properly normalize case.

setAttributeNode() does not properly normalize case.

setAttributeNode() verifies for existing attributes in a case sensitive manner.
Thus, it would add another attribute if specified in a case different from the
existing one. Instead, like setAttribute(), it too should modify the existing
attribute's value.

Added a new method: matchesIgnoringCaseForLocalName() which should be called
only when trying to do a localname comparison ignoring case.

Layout testcase added for verifying the behavior for both html as well as xhtml
documents.

WebKit changeset: http://trac.webkit.org/changeset/154054

R=eseidel@chromium.org, ojan@chromium.org, esprehn@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156179

[eseidel, 2013-08-14 06:51:25]

Does this behavior make sense to you arv?

[esprehn, 2013-08-14 06:52:57]

Why not just make the regular matches() function have this extra boolean
argument?

https://codereview.chromium.org/22803008/diff/1/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/22803008/diff/1/Source/core/dom/Element.h#new...
Source/core/dom/Element.h:1040: inline size_t
ElementData::getAttributeItemIndex(const QualifiedName& name, bool
shouldIgnoreAttributeCase) const
shouldIgnoreCase, you already know it's attribute related based on the method
name.

https://codereview.chromium.org/22803008/diff/1/Source/core/dom/Element.h#new...
Source/core/dom/Element.h:1045: if
(attribute.name().matchesIgnoringCaseForLocalName(name,
shouldIgnoreAttributeCase))
The method name is weird since it doesn't really ignore the case unless you pass
true right?

[arpitab_, 2013-08-14 07:15:54]

On 2013/08/14 06:52:57, esprehn wrote:
Thanks for the review esprehn!

> Why not just make the regular matches() function have this extra boolean
> argument?
> 
This is coz matches() is called upon from other places which don't necessarily
require a case sensitive comparison. So there might be a performance impact for
those cases.

> https://codereview.chromium.org/22803008/diff/1/Source/core/dom/Element.h
> File Source/core/dom/Element.h (right):
> 
>
https://codereview.chromium.org/22803008/diff/1/Source/core/dom/Element.h#new...
> Source/core/dom/Element.h:1040: inline size_t
> ElementData::getAttributeItemIndex(const QualifiedName& name, bool
> shouldIgnoreAttributeCase) const
> shouldIgnoreCase, you already know it's attribute related based on the method
> name.
> 
Will do.

>
https://codereview.chromium.org/22803008/diff/1/Source/core/dom/Element.h#new...
> Source/core/dom/Element.h:1045: if
> (attribute.name().matchesIgnoringCaseForLocalName(name,
> shouldIgnoreAttributeCase))
> The method name is weird since it doesn't really ignore the case unless you
pass
> true right?
Perhaps matchesPossiblyIgnoringCase() would be a better option?

[arv (Not doing code reviews), 2013-08-14 14:50:18]

On 2013/08/14 06:51:25, eseidel wrote:
> Does this behavior make sense to you arv?

This makes sense to me. In practice it doesn't matter much because people do not
use XHTML. Also, these methods are deprecated/removed from the DOM specs.

As long as this has not perf impact, and based on the code it should not, I'm
fine with it.

[eseidel, 2013-08-15 21:13:05]

lgtm

Wow.  XHTML just doens't really matter on the web these days.  It's way less
than 1% of all page views last I knew. :)

That said, happy to accept changes to Blink to make us more standards compliant.

[commit-bot: I haz the power, 2013-08-15 21:13:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/22803008/9001

[commit-bot: I haz the power, 2013-08-15 23:13:30]

Change committed as 156179