Correctly handle accessing a replaced Attr object's attribute value.

Correctly handle accessing a replaced Attr object's attribute value.

If element.setAttributeNode(attr) is performed on an "element" with
an attribute that matches "attr.name", that attribute's value is
updated and attr is attached to "element".

Later accesses of "attr.value" must continue to resolve to "element"'s
underlying attribute, matching (local) names following the case
sensitivity of the element.

A normalized local name was previously assumed for the attribute,
leading to crashing conditions on both setting and getting "attr.value".
Address by having Attr record the local name of the element attribute
it is attached to and use that when looking up the attribute.

R=adamk@chromium.org
BUG=376718
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175556

[sof, 2014-06-03 14:55:29]

Please take a look.

[Inactive, 2014-06-03 15:11:39]

https://codereview.chromium.org/316583002/diff/1/Source/core/html/HTMLSelectE...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/316583002/diff/1/Source/core/html/HTMLSelectE...
Source/core/html/HTMLSelectElement.cpp:326: if (Attribute* sizeAttribute =
ensureUniqueElementData().getAttributeItem(sizeAttr, true))
Gosh, another boolean argument :) Could we make this an enum so that the code
becomes readable?

e.g. getAttributeItem(sizeAttr, IgnoreCase)

[sof, 2014-06-03 15:21:50]

https://codereview.chromium.org/316583002/diff/1/Source/core/html/HTMLSelectE...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/316583002/diff/1/Source/core/html/HTMLSelectE...
Source/core/html/HTMLSelectElement.cpp:326: if (Attribute* sizeAttribute =
ensureUniqueElementData().getAttributeItem(sizeAttr, true))
On 2014/06/03 15:11:39, Chris Dumez wrote:
> Gosh, another boolean argument :) Could we make this an enum so that the code
> becomes readable?
> 
> e.g. getAttributeItem(sizeAttr, IgnoreCase)

That would break with the convention followed elsewhere for encoding match
case-sensitivity.

[eseidel, 2014-06-03 16:45:44]

I had always assumed the crashes were part of our deprcation strategy for attr
nodes. :p

[eseidel, 2014-06-03 16:47:37]

I'm really having trouble caring about this CL.  I'm glad to see a crash fixed,
but this looks really fragile.  It would be trivial for someone to forget to
pass along the case-insentivity, no?  Can't we have the attr store that part or
somehow encapsulate this logic so we dont' have ot pass the bool?  That said..
as I started with, I have trouble caring if this is correct given the
non-existant usage of this deprecated feature on the web.  We could also just
fix it not to crash and be done. :)

[sof, 2014-06-04 11:33:18]

Thanks for the feedback. Taken into account by having the Attr track the local
name of the element's attribute. Should it differ, that is. Used when accessing
the attached element.

[sof, 2014-06-04 21:42:04]

adamk: eseidel is taking a well-earned break, it seems - might you be interested
in having a look at the fix for this crasher?

[adamk, 2014-06-05 00:26:33]

lgtm, I think you've addressed eseidel's worry, and I agree with him that the
details of the behavior here aren't nearly as important as not crashing.

https://codereview.chromium.org/316583002/diff/20001/LayoutTests/fast/dom/Att...
File LayoutTests/fast/dom/Attr/update-attribute-node-no-crash.html (right):

https://codereview.chromium.org/316583002/diff/20001/LayoutTests/fast/dom/Att...
LayoutTests/fast/dom/Attr/update-attribute-node-no-crash.html:13: // This aligns
with Trident rather than Gecko.
Sounds like we're getting no help from the DOM spec here, since setAttributeNode
is deprecated?

[sof, 2014-06-05 06:58:26]

Thanks for the reviews everyone.

https://codereview.chromium.org/316583002/diff/20001/LayoutTests/fast/dom/Att...
File LayoutTests/fast/dom/Attr/update-attribute-node-no-crash.html (right):

https://codereview.chromium.org/316583002/diff/20001/LayoutTests/fast/dom/Att...
LayoutTests/fast/dom/Attr/update-attribute-node-no-crash.html:13: // This aligns
with Trident rather than Gecko.
On 2014/06/05 00:26:34, adamk wrote:
> Sounds like we're getting no help from the DOM spec here, since
setAttributeNode
> is deprecated?

Yes, thoroughly removed from sight there.

The setAttributeNode() text in the DOM Level1 spec sides with Gecko I'd say
("the previously existing Attr node is returned".) Presto follows WebKit + Blink
as well.

Doing spec compat changes here is questionable at this stage; I've added a FIXME
in setAttributeNode() about it though, so that it isn't forgotten about
completely.

[sof, 2014-06-05 06:58:36]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-05 06:59:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/316583002/40001

[commit-bot: I haz the power, 2014-06-05 08:26:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10380)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13233)

[commit-bot: I haz the power, 2014-06-05 11:20:51]

Change committed as 175556