Remove Attr child nodes (making Attr a Node, not a ContainerNode)

Remove Attr child nodes (making Attr a Node, not a ContainerNode)

Intent to Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/4Hg0l42QHH0/H59LyoiJlI4J

Rationale for some of the non-obvious test changes:

LayoutTests/dom/html/level1/core/hc_attrsetvalue2.html:
LayoutTests/dom/xhtml/level1/core/hc_attrsetvalue2.xhtml:

  Removing the child node aspects makes these tests equivalent to
  hc_attrsetvalue1, so they were removed.

LayoutTests/dom/html/level1/core/hc_elementnormalize2.html:
LayoutTests/dom/xhtml/level1/core/hc_elementnormalize2.xhtml:

  These test that Element.normalize() also normalizes the Element's
  Attr's children, so with no children there's nothing left to test.

LayoutTests/fast/dom/Element/normalize-crash.html:

  http://trac.webkit.org/changeset/59109

  The crash was in Element::normalizeAttributes(), which is removed.

LayoutTests/fast/dom/Element/normalize-crash2.html:

  https://code.google.com/p/chromium/issues/detail?id=394026

  The crash was in Element::normalizeAttributes(), which is removed.

LayoutTests/fast/dom/insertedIntoDocument-child.html:
LayoutTests/fast/dom/insertedIntoDocument-sibling.html:

  http://trac.webkit.org/changeset/81611

  Even before this change, the DOMNodeRemoved event listener was not
  actually run in these tests.

LayoutTests/fast/dom/normalize-attributes-mutation-event-crash.html:

  http://trac.webkit.org/changeset/137341

  The crash was in Element::normalizeAttributes(), which is removed.

LayoutTests/fast/storage/storage-detached-iframe.html:

  Already damaged by the replaceWholeText() removal in r185541, nothing
  useful of the subtest remains when removing the firstChild bits.

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

[philipj_slow, 2015-05-27 07:55:24]

philipj@opera.com changed reviewers:
+ davve@opera.com

[philipj_slow, 2015-05-27 07:55:25]

PTAL

[davve, 2015-05-28 14:32:15]

LGTM

Also Attribute.h has a stale comment stating Attr has a text child.

https://codereview.chromium.org/1158433004/diff/1/LayoutTests/svg/custom/imag...
File LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html
(right):

https://codereview.chromium.org/1158433004/diff/1/LayoutTests/svg/custom/imag...
LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html:25:
svgImage.href.baseVal = "no-image";
Nit: may want to change href through .value to get as similar behavior as
possible to the previous one.

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

https://codereview.chromium.org/1158433004/diff/1/Source/core/dom/Element.cpp...
Source/core/dom/Element.cpp:1087:
ensureUniqueElementData().attributes().at(index).setValue(newValue);
We may want to keep the newValue != existingAttributeValue check in order to not
make unique element data unless we have to.

[philipj_slow, 2015-05-28 15:28:36]

Also updated Attribute.h to be a bit more informative.

https://codereview.chromium.org/1158433004/diff/1/LayoutTests/svg/custom/imag...
File LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html
(right):

https://codereview.chromium.org/1158433004/diff/1/LayoutTests/svg/custom/imag...
LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html:25:
svgImage.href.baseVal = "no-image";
On 2015/05/28 14:32:14, David Vest wrote:
> Nit: may want to change href through .value to get as similar behavior as
> possible to the previous one.

Done.

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

https://codereview.chromium.org/1158433004/diff/1/Source/core/dom/Element.cpp...
Source/core/dom/Element.cpp:1087:
ensureUniqueElementData().attributes().at(index).setValue(newValue);
On 2015/05/28 14:32:14, David Vest wrote:
> We may want to keep the newValue != existingAttributeValue check in order to
not
> make unique element data unless we have to.

Done.

[philipj_slow, 2015-05-28 15:29:05]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-05-28 15:29:06]

The patchset sent to the CQ was uploaded after l-g-t-m from davve@opera.com
Link to the patchset: https://codereview.chromium.org/1158433004/#ps20001
(title: "address feedback")

[commit-bot: I haz the power, 2015-05-28 15:29:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433004/20001

[philipj_slow, 2015-05-28 15:41:54]

The CQ bit was unchecked by philipj@opera.com

[philipj_slow, 2015-05-28 19:20:57]

philipj@opera.com changed reviewers:
+ tkent@chromium.org

[philipj_slow, 2015-05-28 19:20:58]

tkent@, could you sanity-check this as well? This is old code that I don't think
anyone has any sense of ownership for, but you've been here a few times lately,
at least.

(Looking for reviewers I also found  https://trac.webkit.org/changeset/131104
where the FIXMEs were added, but those involved are at Apple.)

[tkent, 2015-05-29 03:34:07]

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (left):

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cp...
Source/core/dom/Attr.cpp:121: invalidateNodeListCachesInAncestors(&name,
m_element);
Is it safe to remove invalidateNodeListCachesInAncestors?

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (right):

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cp...
Source/core/dom/Attr.cpp:92: m_element->setAttribute(qualifiedName(), value);
Does this cause infinite loop?
  Element::setAttribute() -> Element::setAttributeInternal -> Attr::setValue

[davve, 2015-05-29 06:29:14]

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (left):

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cp...
Source/core/dom/Attr.cpp:121: invalidateNodeListCachesInAncestors(&name,
m_element);
On 2015/05/29 03:34:07, tkent wrote:
> Is it safe to remove invalidateNodeListCachesInAncestors?

invalidateNodeListCachesInAncestors should still be called, but through
Element::setAttribute (-> setAttributeInternal -> didModifyAttribute ->
attributeChanged -> invalidateNodeListCachesInAncestors).

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (right):

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cp...
Source/core/dom/Attr.cpp:92: m_element->setAttribute(qualifiedName(), value);
On 2015/05/29 03:34:07, tkent wrote:
> Does this cause infinite loop?
>   Element::setAttribute() -> Element::setAttributeInternal -> Attr::setValue

Element::setAttributeInternal shouldn't call into Attr (anymore). It sets the
value on Attribute, the internal representation of an attribute.

[tkent, 2015-05-29 07:49:15]

lgtm

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (right):

https://codereview.chromium.org/1158433004/diff/20001/Source/core/dom/Attr.cp...
Source/core/dom/Attr.cpp:92: m_element->setAttribute(qualifiedName(), value);
On 2015/05/29 06:29:14, David Vest wrote:
> On 2015/05/29 03:34:07, tkent wrote:
> > Does this cause infinite loop?
> >   Element::setAttribute() -> Element::setAttributeInternal -> Attr::setValue
> 
> Element::setAttributeInternal shouldn't call into Attr (anymore). It sets the
> value on Attribute, the internal representation of an attribute.

Ah, I see.  Attr doesn't need to update the value while it's owned by an
Element.

[philipj_slow, 2015-05-29 08:24:02]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-05-29 08:24:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433004/20001

[commit-bot: I haz the power, 2015-05-29 08:29:29]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196128