Node.insertBefore and Node.appendChild do not use custom binding anymore

Node.insertBefore and Node.appendChild do not use custom binding anymore

This CL removed custom binding from
Node.insertBefore and Node.appendChild.
So they will not be hidden from code generator.
To do this, C++ implementation now return
Node instead of void.

Next, nullable type was added to refChild
in Node.insertBefore as per spec and
TypeChecking was enabled.
So appropriate TypeError will be thrown.

TEST=fast/dom/Node/fragment-mutation.html
    ,fast/dom/move-nodes-across-documents.html
    ,fast/block/basic/empty-anonymous-block-remove-crash.html
    ,svg/animations/mpath-remove-from-dependents-on-delete-crash.html
    ,fast/css/invalidation/style-update-with-added-stylesheet.html
    ,fast/html/details-summary-document-child.html

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

[kangil_, 2014-07-09 11:00:18]

PTAL

[haraken, 2014-07-09 12:12:16]

jl@: Are you fine with this change? Should we probably use [TypeCheck=Interface]
instead?

[Jens Widell, 2014-07-09 12:21:35]

On 2014/07/09 12:12:16, haraken wrote:
> jl@: Are you fine with this change? Should we probably use
[TypeCheck=Interface]
> instead?

Making the argument nullable is fine; that's what it is in the specification. We
ought to also use [TypeChecking=Interface], since that gives correct behavior
per Web IDL on non-null invalid inputs. (Without it, anything but a Node for the
second argument will come through as null, and then not raise an exception,
since null is a valid value.)

None of this matters since the method has [Custom], but I don't really see why
it does.

[kangil_, 2014-07-09 12:23:33]

On 2014/07/09 12:21:35, Jens Lindström wrote:
> On 2014/07/09 12:12:16, haraken wrote:
> > jl@: Are you fine with this change? Should we probably use
> [TypeCheck=Interface]
> > instead?
> 
> Making the argument nullable is fine; that's what it is in the specification.
We
> ought to also use [TypeChecking=Interface], since that gives correct behavior
> per Web IDL on non-null invalid inputs. (Without it, anything but a Node for
the
> second argument will come through as null, and then not raise an exception,
> since null is a valid value.)
> 
> None of this matters since the method has [Custom], but I don't really see why
> it does.

Yes, so my plan is to do similar stuff like what I did for appendChild.

[haraken, 2014-07-09 12:25:31]

On 2014/07/09 12:23:33, kangil_ wrote:
> On 2014/07/09 12:21:35, Jens Lindström wrote:
> > On 2014/07/09 12:12:16, haraken wrote:
> > > jl@: Are you fine with this change? Should we probably use
> > [TypeCheck=Interface]
> > > instead?
> > 
> > Making the argument nullable is fine; that's what it is in the
specification.
> We
> > ought to also use [TypeChecking=Interface], since that gives correct
behavior
> > per Web IDL on non-null invalid inputs. (Without it, anything but a Node for
> the
> > second argument will come through as null, and then not raise an exception,
> > since null is a valid value.)
> > 
> > None of this matters since the method has [Custom], but I don't really see
why
> > it does.
> 
> Yes, so my plan is to do similar stuff like what I did for appendChild.

I think what jl@ is saying is we should remove the custom binding instead of
just making the IDL conformant with the spec.

[kangil_, 2014-07-09 12:27:06]

On 2014/07/09 12:25:31, haraken wrote:
> On 2014/07/09 12:23:33, kangil_ wrote:
> > On 2014/07/09 12:21:35, Jens Lindström wrote:
> > > On 2014/07/09 12:12:16, haraken wrote:
> > > > jl@: Are you fine with this change? Should we probably use
> > > [TypeCheck=Interface]
> > > > instead?
> > > 
> > > Making the argument nullable is fine; that's what it is in the
> specification.
> > We
> > > ought to also use [TypeChecking=Interface], since that gives correct
> behavior
> > > per Web IDL on non-null invalid inputs. (Without it, anything but a Node
for
> > the
> > > second argument will come through as null, and then not raise an
exception,
> > > since null is a valid value.)
> > > 
> > > None of this matters since the method has [Custom], but I don't really see
> why
> > > it does.
> > 
> > Yes, so my plan is to do similar stuff like what I did for appendChild.
> 
> I think what jl@ is saying is we should remove the custom binding instead of
> just making the IDL conformant with the spec.

OK, let me try it first.(I was hesitate to do because I wasn't sure)

[Jens Widell, 2014-07-09 12:28:32]

> On 2014/07/09 12:23:33, kangil_ wrote:
> > Yes, so my plan is to do similar stuff like what I did for appendChild.

What did you do for appendChild()?

On 2014/07/09 12:25:31, haraken wrote:
> I think what jl@ is saying is we should remove the custom binding instead of
> just making the IDL conformant with the spec.

Yeah, assuming there's no reason to have the custom bindings that I'm missing
here. It's of course always nice to get rid of custom bindings.

Aligning the IDL with the spec can be nice either way, but risks being
misleading when there's a custom binding that overrides parts of the IDL anyway
(such as nullability.)

[kangil_, 2014-07-09 12:32:27]

On 2014/07/09 12:28:32, Jens Lindström wrote:
> > On 2014/07/09 12:23:33, kangil_ wrote:
> > > Yes, so my plan is to do similar stuff like what I did for appendChild.
> 
> What did you do for appendChild()?

https://codereview.chromium.org/374093002/ (I forgot to add a pointer.)

> 
> On 2014/07/09 12:25:31, haraken wrote:
> > I think what jl@ is saying is we should remove the custom binding instead of
> > just making the IDL conformant with the spec.
> 
> Yeah, assuming there's no reason to have the custom bindings that I'm missing
> here. It's of course always nice to get rid of custom bindings.
> 
> Aligning the IDL with the spec can be nice either way, but risks being
> misleading when there's a custom binding that overrides parts of the IDL
anyway
> (such as nullability.)

[kangil_, 2014-07-10 06:10:08]

On 2014/07/09 12:27:06, kangil_ wrote:
> On 2014/07/09 12:25:31, haraken wrote:
> > On 2014/07/09 12:23:33, kangil_ wrote:
> > > On 2014/07/09 12:21:35, Jens Lindström wrote:
> > > > On 2014/07/09 12:12:16, haraken wrote:
> > > > > jl@: Are you fine with this change? Should we probably use
> > > > [TypeCheck=Interface]
> > > > > instead?
> > > > 
> > > > Making the argument nullable is fine; that's what it is in the
> > specification.
> > > We
> > > > ought to also use [TypeChecking=Interface], since that gives correct
> > behavior
> > > > per Web IDL on non-null invalid inputs. (Without it, anything but a Node
> for
> > > the
> > > > second argument will come through as null, and then not raise an
> exception,
> > > > since null is a valid value.)
> > > > 
> > > > None of this matters since the method has [Custom], but I don't really
see
> > why
> > > > it does.
> > > 
> > > Yes, so my plan is to do similar stuff like what I did for appendChild.
> > 
> > I think what jl@ is saying is we should remove the custom binding instead of
> > just making the IDL conformant with the spec.
> 
> OK, let me try it first.(I was hesitate to do because I wasn't sure)

There seem not a cleaner way to return node than 'v8SetReturnValue(info,
info[0]);'

So, IMHO,
  1. keep custom method as it is
  2. mark '?' in idl as per spec
  3. implement custom method like generated one as much as possible
are practical options.

Thoughts?

[Jens Widell, 2014-07-10 06:33:09]

On 2014/07/10 06:10:08, kangil_ wrote:
> There seem not a cleaner way to return node than 'v8SetReturnValue(info,
> info[0]);'
> 
> So, IMHO,
>   1. keep custom method as it is
>   2. mark '?' in idl as per spec
>   3. implement custom method like generated one as much as possible
> are practical options.
> 
> Thoughts?

Right, because the C++ implementations (e.g. Node::insertBefore()) doesn't
return the node. There's a comment in Node.h that they ought, but that this is
"only important for language bindings" which will already know the return value.
This of course forces us to implement a custom binding for those methods.

I wonder if there's any good reason why these functions don't just return Node*,
thus letting us treat them as any other methods.

[kangil_, 2014-07-10 07:04:26]

On 2014/07/10 06:33:09, Jens Lindström wrote:
> Right, because the C++ implementations (e.g. Node::insertBefore()) doesn't
> return the node. There's a comment in Node.h that they ought, but that this is
> "only important for language bindings" which will already know the return
value.
> This of course forces us to implement a custom binding for those methods.
> 
> I wonder if there's any good reason why these functions don't just return
Node*,
> thus letting us treat them as any other methods.

I am little bit cautious to say because I am neither a spec writer nor patch
owner of custom binding,
anyways my two cents are:
  1. I Don't get reason why we should return parameter as a result.(void/boolean
type look enough for me)
  2. Seems relevant node in tree should be found to return result from C++
implementation.(looks harder to implement, slower in performance and more
complicated to read than just return script value)
Feel free to fix me if I am wrong.

[Jens Widell, 2014-07-10 07:13:53]

On 2014/07/10 07:04:26, kangil_ wrote:
> On 2014/07/10 06:33:09, Jens Lindström wrote:
> > Right, because the C++ implementations (e.g. Node::insertBefore()) doesn't
> > return the node. There's a comment in Node.h that they ought, but that this
is
> > "only important for language bindings" which will already know the return
> value.
> > This of course forces us to implement a custom binding for those methods.
> > 
> > I wonder if there's any good reason why these functions don't just return
> Node*,
> > thus letting us treat them as any other methods.
> 
> I am little bit cautious to say because I am neither a spec writer nor patch
> owner of custom binding,
> anyways my two cents are:
>   1. I Don't get reason why we should return parameter as a
result.(void/boolean
> type look enough for me)
>   2. Seems relevant node in tree should be found to return result from C++
> implementation.(looks harder to implement, slower in performance and more
> complicated to read than just return script value)
> Feel free to fix me if I am wrong.

The spec situation is not much to think about; the insertBefore() method should
return its first argument. Good or bad API, it's been like that forever.

The alternatives, as I see it:

 1) fix the C++ implementations to return a value like pretty much all other C++
implementations do, thus removing the need to have a custom binding
 2) keep the custom binding (and continue maintaining it manually, forever)
 3) add support for a [ReturnArgument=newChild] extended attribute and let the
code generator take care of it

IMO, we ought to investigate whether 1 impacts performance negatively, and if
not, just do that, and eliminate the special case.

(I don't really think we should do 3 either; if we can't do 1, then 2 is
probably the better choice.)

[haraken, 2014-07-10 07:17:57]

On 2014/07/10 07:13:53, Jens Lindström wrote:
> On 2014/07/10 07:04:26, kangil_ wrote:
> > On 2014/07/10 06:33:09, Jens Lindström wrote:
> > > Right, because the C++ implementations (e.g. Node::insertBefore()) doesn't
> > > return the node. There's a comment in Node.h that they ought, but that
this
> is
> > > "only important for language bindings" which will already know the return
> > value.
> > > This of course forces us to implement a custom binding for those methods.
> > > 
> > > I wonder if there's any good reason why these functions don't just return
> > Node*,
> > > thus letting us treat them as any other methods.
> > 
> > I am little bit cautious to say because I am neither a spec writer nor patch
> > owner of custom binding,
> > anyways my two cents are:
> >   1. I Don't get reason why we should return parameter as a
> result.(void/boolean
> > type look enough for me)
> >   2. Seems relevant node in tree should be found to return result from C++
> > implementation.(looks harder to implement, slower in performance and more
> > complicated to read than just return script value)
> > Feel free to fix me if I am wrong.
> 
> The spec situation is not much to think about; the insertBefore() method
should
> return its first argument. Good or bad API, it's been like that forever.
> 
> The alternatives, as I see it:
> 
>  1) fix the C++ implementations to return a value like pretty much all other
C++
> implementations do, thus removing the need to have a custom binding
>  2) keep the custom binding (and continue maintaining it manually, forever)
>  3) add support for a [ReturnArgument=newChild] extended attribute and let the
> code generator take care of it
> 
> IMO, we ought to investigate whether 1 impacts performance negatively, and if
> not, just do that, and eliminate the special case.
> 
> (I don't really think we should do 3 either; if we can't do 1, then 2 is
> probably the better choice.)

I'm on the same page with you. 1 sounds the best, and 2 is ok.

[kangil_, 2014-07-10 08:59:21]

Try best approach. PTAL.

[Jens Widell, 2014-07-10 09:14:55]

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:180: Node* ContainerNode::sameNode(Node* node)
I don't see why this is necessary, and performance-wise it'd be an absolute
disaster.

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:203: return lastChild();
You should just return 'newChild' everywhere (or nullptr if an exception was
thrown, although in that case it doesn't really matter.)

[haraken, 2014-07-10 09:16:02]

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:190: PassRefPtrWillBeRawPtr<Node>
ContainerNode::insertBefore(PassRefPtrWillBeRawPtr<Node> newChild, Node*
refChild, ExceptionState& exceptionState)

insertBefore is in hot call paths. Calling sameNode() wouldn't be acceptable for
performance.

Why can't you just make insertBefore return |newNode|?

[Jens Widell, 2014-07-10 09:18:27]

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:202: appendChild(newChild, exceptionState);
We could deal with appendChild(), replaceChild() and removeChild() in the same
patch, I think, in which case this line should just be

  return appendChild(newChild, exceptionState);

[kangil_, 2014-07-11 02:09:26]

Thanks for the review comments.
I was totally confused by specific test case.

PTAL!

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:180: Node* ContainerNode::sameNode(Node* node)
On 2014/07/10 09:14:54, Jens Lindström wrote:
> I don't see why this is necessary, and performance-wise it'd be an absolute
> disaster.

Done.

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:190: PassRefPtrWillBeRawPtr<Node>
ContainerNode::insertBefore(PassRefPtrWillBeRawPtr<Node> newChild, Node*
refChild, ExceptionState& exceptionState)
On 2014/07/10 09:16:02, haraken wrote:
> 
> insertBefore is in hot call paths. Calling sameNode() wouldn't be acceptable
for
> performance.
> 
> Why can't you just make insertBefore return |newNode|?

Done.

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:202: appendChild(newChild, exceptionState);
On 2014/07/10 09:18:27, Jens Lindström wrote:
> We could deal with appendChild(), replaceChild() and removeChild() in the same
> patch, I think, in which case this line should just be
> 
>   return appendChild(newChild, exceptionState);

insertBefore() and appendChild() implementations are associated with.
So I would implement two methods here.
replaceChild() and removeChild() are associated so I will do same in follow
patch.

https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:203: return lastChild();
On 2014/07/10 09:14:54, Jens Lindström wrote:
> You should just return 'newChild' everywhere (or nullptr if an exception was
> thrown, although in that case it doesn't really matter.)

Done.

[haraken, 2014-07-11 02:18:24]

LGTM

https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8NodeCustom.cpp (right):

https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8NodeCustom.cpp:77: void
V8Node::removeChildMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info)

You can consider removing these custom bindings for replaceChild and removeChild
in a follow-up CL.

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:197: return nullptr;

Shall we add ASSERT(exceptionState.hadException())?

We want to keep the invariant that nullptr is returned if and only if an
exception is thrown.

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:221: return nullptr;

Add ASSERT(exceptionState.hadException()).

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:609: return nullptr;

Add ASSERT(exceptionState.hadException()).

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:625: return nullptr;

Add ASSERT(exceptionState.hadException()).

[kangil_, 2014-07-11 02:39:42]

PTAL

https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8NodeCustom.cpp (right):

https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8NodeCustom.cpp:77: void
V8Node::removeChildMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2014/07/11 02:18:23, haraken wrote:
> 
> You can consider removing these custom bindings for replaceChild and
removeChild
> in a follow-up CL.

Sure I will.

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:197: return nullptr;
On 2014/07/11 02:18:23, haraken wrote:
> 
> Shall we add ASSERT(exceptionState.hadException())?
> 
> We want to keep the invariant that nullptr is returned if and only if an
> exception is thrown.

Done.

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:221: return nullptr;
On 2014/07/11 02:18:23, haraken wrote:
> 
> Add ASSERT(exceptionState.hadException()).

Done.

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:609: return nullptr;
On 2014/07/11 02:18:23, haraken wrote:
> 
> Add ASSERT(exceptionState.hadException()).

Done.

https://codereview.chromium.org/375293002/diff/60001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:625: return nullptr;
On 2014/07/11 02:18:23, haraken wrote:
> 
> Add ASSERT(exceptionState.hadException()).

Done.

[haraken, 2014-07-11 02:42:22]

LGTM

[kangil_, 2014-07-11 02:45:24]

The CQ bit was checked by kangil.han@samsung.com

[commit-bot: I haz the power, 2014-07-11 02:45:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kangil.han@samsung.com/375293002/80001

[commit-bot: I haz the power, 2014-07-11 03:52:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16370)

[commit-bot: I haz the power, 2014-07-11 04:57:02]

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

[commit-bot: I haz the power, 2014-07-11 04:57:04]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[kangil_, 2014-07-11 07:41:13]

On 2014/07/11 04:57:04, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_dbg on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

IgnorableExceptionState is used in specific test case.

[kangil_, 2014-07-11 10:51:12]

The CQ bit was checked by kangil.han@samsung.com

[commit-bot: I haz the power, 2014-07-11 10:51:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kangil.han@samsung.com/375293002/100001

[commit-bot: I haz the power, 2014-07-11 11:59:00]

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

[commit-bot: I haz the power, 2014-07-11 13:06:34]

Change committed as 177940