Never cast Attr to ContainerNode based on nodeType ATTRIBUTE_NODE

Never cast Attr to ContainerNode based on nodeType ATTRIBUTE_NODE

In https://codereview.chromium.org/1158433004 Attr began inheriting from
Node instead of ContainerNode, but there remained cases where
toContainerNode() was called for Attr.

All instances of toContainerNode() and other casts to ContainerNode were
inspected and only these two were found to be problematic, although some
instances in core/editing are non-obvious.

Because textContent is an alias of value in Attr.idl, web content
setting attr.textContent directly will not take the Node code path in
question. However, it is possible with some effort:

var attr = document.createAttribute("name");
Object.getOwnPropertyDescriptor(Node.prototype, "textContent").set.call(attr, "value");

This will now do nothing, and the equivalent code with the getter would
already return null before this change. This is strange, but most
closely resembles the spec, which doesn't handle Attr in the textContent
setter because per spec it isn't a Node at all:
https://dom.spec.whatwg.org/#dom-node-textcontent

No calls to Node::textContent() or Node::setTextContent() where the Node
could be an Attr were found, other than V8Node.cp in generated bindings.

BUG=305105, 494043
TEST=Source/core/dom/AttrTest.cpp (also tests nodeValue)

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

[philipj_slow, 2015-06-02 18:39:34]

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

[philipj_slow, 2015-06-02 18:39:34]

PTAL

[tkent, 2015-06-02 22:23:32]

lgtm

nit: Unit test is more suitable.

[davve, 2015-06-03 06:31:05]

Non-owner LGTM.

Ideally I wish we didn't do type conversions based on nodeType since we already
have a system for doing safe type conversions. Having two ways is obviously not
optimal.

[philipj_slow, 2015-06-03 09:18:03]

tkent@, thanks for that suggestion, a unit test was indeed a better fit here.
Can you take a look at that? Also, see the updated the description about the
asymmetry in Node::textContent() and setTextContent(). Since it's not actually
called internally I think that's tolerable, but either of these options would
work too:

1. ASSERT in both that it's not an Attr node
2. Make setTextContent() do nothing for Attr

Making Node::textContent() work for Attr would add code to a very common code
path that's not needed, so I don't propose that.

WDYT?

[tkent, 2015-06-03 09:45:08]

Patch Set 2 still LGTM.

On 2015/06/03 09:18:03, philipj wrote:
> tkent@, thanks for that suggestion, a unit test was indeed a better fit here.
> Can you take a look at that? Also, see the updated the description about the
> asymmetry in Node::textContent() and setTextContent(). Since it's not actually
> called internally I think that's tolerable, but either of these options would
> work too:
> 
> 1. ASSERT in both that it's not an Attr node
> 2. Make setTextContent() do nothing for Attr
> 
> Making Node::textContent() work for Attr would add code to a very common code
> path that's not needed, so I don't propose that.
> 
> WDYT?

Are you sure that Node::textContent and setTextContent for Attr are never called
from C++ code?

[philipj_slow, 2015-06-03 10:29:15]

On 2015/06/03 09:45:08, tkent wrote:
> Patch Set 2 still LGTM.
> 
> On 2015/06/03 09:18:03, philipj wrote:
> > tkent@, thanks for that suggestion, a unit test was indeed a better fit
here.
> > Can you take a look at that? Also, see the updated the description about the
> > asymmetry in Node::textContent() and setTextContent(). Since it's not
actually
> > called internally I think that's tolerable, but either of these options
would
> > work too:
> > 
> > 1. ASSERT in both that it's not an Attr node
> > 2. Make setTextContent() do nothing for Attr
> > 
> > Making Node::textContent() work for Attr would add code to a very common
code
> > path that's not needed, so I don't propose that.
> > 
> > WDYT?
> 
> Are you sure that Node::textContent and setTextContent for Attr are never
called
> from C++ code?

I've gone through all the call sites using cs.chromium.org and for both
textContent() and setTextContent() the only call sites where it's a Node as
opposed to some specific non-Attr type of Node is in bindings, V8Node.cpp.
Because Attr overrides textContent (and nodeValue) I think it's impossible to
reach the code with Attr.

If textContent were a function it would be possible to do something like
Node.prototype.textContent.call(attr) and bypass the bindings, but I haven't
found a way to do this with an attribute, even though they're now on the
prototype chain. I'll ask a bindings expert about this, though.

[philipj_slow, 2015-06-03 13:51:18]

On 2015/06/03 10:29:15, philipj wrote:
> On 2015/06/03 09:45:08, tkent wrote:
> > Patch Set 2 still LGTM.
> > 
> > On 2015/06/03 09:18:03, philipj wrote:
> > > tkent@, thanks for that suggestion, a unit test was indeed a better fit
> here.
> > > Can you take a look at that? Also, see the updated the description about
the
> > > asymmetry in Node::textContent() and setTextContent(). Since it's not
> actually
> > > called internally I think that's tolerable, but either of these options
> would
> > > work too:
> > > 
> > > 1. ASSERT in both that it's not an Attr node
> > > 2. Make setTextContent() do nothing for Attr
> > > 
> > > Making Node::textContent() work for Attr would add code to a very common
> code
> > > path that's not needed, so I don't propose that.
> > > 
> > > WDYT?
> > 
> > Are you sure that Node::textContent and setTextContent for Attr are never
> called
> > from C++ code?
> 
> I've gone through all the call sites using http://cs.chromium.org and for both
> textContent() and setTextContent() the only call sites where it's a Node as
> opposed to some specific non-Attr type of Node is in bindings, V8Node.cpp.
> Because Attr overrides textContent (and nodeValue) I think it's impossible to
> reach the code with Attr.
> 
> If textContent were a function it would be possible to do something like
> Node.prototype.textContent.call(attr) and bypass the bindings, but I haven't
> found a way to do this with an attribute, even though they're now on the
> prototype chain. I'll ask a bindings expert about this, though.

I asked jl@ and we came up with a way, now noted in the description:
var attr = document.createAttribute("name");
Object.getOwnPropertyDescriptor(Node.prototype, "textContent").set.call(attr,
"value");

Sorry to drag this out, but I'd like to ask for your final check of PS3 in light
of this. The situation is weird, but would be made sane again by either making
Attr not a Node or reverting that change from the spec and going back to having
textContent only on Node.

[tkent, 2015-06-04 00:03:00]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-06-04 00:03:00]

lgtm

[tkent, 2015-06-04 00:03:00]

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/1148003007/#ps40001
(title: "new deal: setTextContent(attr) does nothing")

[commit-bot: I haz the power, 2015-06-04 00:03:23]

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

[commit-bot: I haz the power, 2015-06-04 00:41:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-04 00:41:40]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57585)

[tkent, 2015-06-04 01:35:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2015-06-04 01:35:19]

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

[commit-bot: I haz the power, 2015-06-04 03:14:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-04 03:14:12]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/65173)

[tkent, 2015-06-04 03:14:46]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2015-06-04 03:15:02]

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

[commit-bot: I haz the power, 2015-06-04 04:18:25]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196452