Add [TypeChecking=Interface] to Node interface

Add [TypeChecking=Interface] to Node interface

This changes the behavior of the methods

  compareDocumentPosition(Node)
  contains(Node?)
  isEqualNode(Node?)
  isSameNode(Node?)

that now throw TypeError exceptions instead of returning false when called
with invalid arguments. The argument to isSameNode() is also made
required instead of optional, meaning it will also throw TypeError when
called without arguments.

The new behavior matches Firefox, except in the case of isSameNode() which
Firefox doesn't support. (The method has been removed from the spec.)

The new behavior of compareDocumentPosition() and isEqualNode() also matches
IE11.

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

[Jens Widell, 2015-03-05 12:36:38]

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

[Jens Widell, 2015-03-05 12:36:38]

PTAL

[philipj_slow, 2015-03-05 14:54:52]

The contains change looks like the biggest risk, but since it's nullable it
would have to be an argument of some non-Node type so it'll probably be OK.

I actually think we should change isSameNode to look like isEqualNode, since
that's what it will look like if added back to the spec, but that doesn't need
to happen in this CL.

[philipj_slow, 2015-03-05 14:56:12]

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

[philipj_slow, 2015-03-05 14:56:13]

Philip, second opinion on the risk? I think it looks safe enough to bypass all
process and use counters.

[Jens Widell, 2015-03-05 14:59:28]

On 2015/03/05 14:54:52, philipj_UTC7 wrote:
> The contains change looks like the biggest risk, but since it's nullable it
> would have to be an argument of some non-Node type so it'll probably be OK.

Where did you look to determine that contains() was a bigger risk?

> I actually think we should change isSameNode to look like isEqualNode, since
> that's what it will look like if added back to the spec, but that doesn't need
> to happen in this CL.

Make the argument non-optional too?

[philipj_slow, 2015-03-05 15:02:57]

On 2015/03/05 14:59:28, Jens Widell wrote:
> On 2015/03/05 14:54:52, philipj_UTC7 wrote:
> > The contains change looks like the biggest risk, but since it's nullable it
> > would have to be an argument of some non-Node type so it'll probably be OK.
> 
> Where did you look to determine that contains() was a bigger risk?

Just based on what you said about the IE behavior.

> > I actually think we should change isSameNode to look like isEqualNode, since
> > that's what it will look like if added back to the spec, but that doesn't
need
> > to happen in this CL.
> 
> Make the argument non-optional too?

Yes, exactly.

[Jens Widell, 2015-03-05 15:05:15]

On 2015/03/05 15:02:57, philipj_UTC7 wrote:
> On 2015/03/05 14:59:28, Jens Widell wrote:
> > On 2015/03/05 14:54:52, philipj_UTC7 wrote:
> > > The contains change looks like the biggest risk, but since it's nullable
it
> > > would have to be an argument of some non-Node type so it'll probably be
OK.
> > 
> > Where did you look to determine that contains() was a bigger risk?
> 
> Just based on what you said about the IE behavior.

Oh, right. :-) Forgot I even tested in IE (I so seldom do).

[Jens Widell, 2015-03-05 15:37:52]

On 2015/03/05 15:02:57, philipj_UTC7 wrote:
> On 2015/03/05 14:59:28, Jens Widell wrote:
> > On 2015/03/05 14:54:52, philipj_UTC7 wrote:
> > > I actually think we should change isSameNode to look like isEqualNode,
since
> > > that's what it will look like if added back to the spec, but that doesn't
> need
> > > to happen in this CL.
> > 
> > Make the argument non-optional too?
> 
> Yes, exactly.

Done now.

[philipj_slow, 2015-03-05 16:25:44]

Can you update the description wrt isSameNode as well? This all LGTM, but out of
an abundance of caution I'd like to see what the other Philip says too :)

[pdr., 2015-03-05 21:24:39]

On 2015/03/05 at 16:25:44, philipj wrote:
> Can you update the description wrt isSameNode as well? This all LGTM, but out
of an abundance of caution I'd like to see what the other Philip says too :)

LGTM, this sounds reasonable to me.

[Jens Widell, 2015-03-06 07:43:24]

On 2015/03/05 16:25:44, philipj_UTC7 wrote:
> Can you update the description wrt isSameNode as well?

Description updated.

[Jens Widell, 2015-03-06 07:43:28]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-03-06 07:43:39]

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

[philipj_slow, 2015-03-06 08:22:36]

In light of the test regression from the Range change, have you checked if any
of these are used internally where the argument could possibly be null?

[Jens Widell, 2015-03-06 08:34:33]

On 2015/03/06 08:22:36, philipj_UTC7 wrote:
> In light of the test regression from the Range change, have you checked if any
> of these are used internally where the argument could possibly be null?

In this patch only compareDocumentPosition() is affected on the C++ side, and
it's only called from two places: DocumentOrderedList::add() and
TreeScope::comparePosition(). It doesn't look to me like either one can
reasonably call with null.

By comparison, Range is used *a lot* from C++ code.

[philipj_slow, 2015-03-06 11:07:12]

On 2015/03/06 08:34:33, Jens Widell wrote:
> On 2015/03/06 08:22:36, philipj_UTC7 wrote:
> > In light of the test regression from the Range change, have you checked if
any
> > of these are used internally where the argument could possibly be null?
> 
> In this patch only compareDocumentPosition() is affected on the C++ side, and
> it's only called from two places: DocumentOrderedList::add() and
> TreeScope::comparePosition(). It doesn't look to me like either one can
> reasonably call with null.
> 
> By comparison, Range is used *a lot* from C++ code.

Great, thanks for checking!

[Jens Widell, 2015-03-06 13:18:14]

The CQ bit was unchecked by jl@opera.com

[Jens Widell, 2015-03-06 13:24:35]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2015-03-06 13:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/978223002/#ps40001 (title:
"update test wrt isSameNode() change")

[commit-bot: I haz the power, 2015-03-06 13:24:58]

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

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

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