Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(219)

Issue 375293002: Node.insertBefore and Node.appendChild do not use custom binding anymore (Closed)

Created:
6 years, 5 months ago by kangil_
Modified:
6 years, 5 months ago
Reviewers:
tkent, haraken, Jens Widell
CC:
blink-reviews, arv+blink, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, Inactive, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Best is always good #

Total comments: 8

Patch Set 3 : Need discussion on this patch #

Patch Set 4 : Take review comment into consideration #

Total comments: 10

Patch Set 5 : Fix failed layout test cases #

Patch Set 6 : Replace assert with condition #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -86 lines) Patch
M LayoutTests/fast/block/basic/empty-anonymous-block-remove-crash.html View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/css/invalidation/style-update-with-added-stylesheet.html View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/dom/Node/fragment-mutation.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M LayoutTests/fast/dom/move-nodes-across-documents.html View 1 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/html/details-summary-document-child.html View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/svg/animations/mpath-remove-from-dependents-on-delete-crash.html View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/core/v8/custom/V8NodeCustom.cpp View 1 2 3 2 chunks +0 lines, -44 lines 0 comments Download
M Source/core/dom/ContainerNode.h View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/dom/ContainerNode.cpp View 1 2 3 4 5 6 chunks +35 lines, -20 lines 0 comments Download
M Source/core/dom/Node.h View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/dom/Node.cpp View 1 2 3 2 chunks +10 lines, -8 lines 0 comments Download
M Source/core/dom/Node.idl View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
kangil_
PTAL
6 years, 5 months ago (2014-07-09 11:00:18 UTC) #1
haraken
jl@: Are you fine with this change? Should we probably use [TypeCheck=Interface] instead?
6 years, 5 months ago (2014-07-09 12:12:16 UTC) #2
Jens Widell
On 2014/07/09 12:12:16, haraken wrote: > jl@: Are you fine with this change? Should we ...
6 years, 5 months ago (2014-07-09 12:21:35 UTC) #3
kangil_
On 2014/07/09 12:21:35, Jens Lindström wrote: > On 2014/07/09 12:12:16, haraken wrote: > > jl@: ...
6 years, 5 months ago (2014-07-09 12:23:33 UTC) #4
haraken
On 2014/07/09 12:23:33, kangil_ wrote: > On 2014/07/09 12:21:35, Jens Lindström wrote: > > On ...
6 years, 5 months ago (2014-07-09 12:25:31 UTC) #5
kangil_
On 2014/07/09 12:25:31, haraken wrote: > On 2014/07/09 12:23:33, kangil_ wrote: > > On 2014/07/09 ...
6 years, 5 months ago (2014-07-09 12:27:06 UTC) #6
Jens Widell
> On 2014/07/09 12:23:33, kangil_ wrote: > > Yes, so my plan is to do ...
6 years, 5 months ago (2014-07-09 12:28:32 UTC) #7
kangil_
On 2014/07/09 12:28:32, Jens Lindström wrote: > > On 2014/07/09 12:23:33, kangil_ wrote: > > ...
6 years, 5 months ago (2014-07-09 12:32:27 UTC) #8
kangil_
On 2014/07/09 12:27:06, kangil_ wrote: > On 2014/07/09 12:25:31, haraken wrote: > > On 2014/07/09 ...
6 years, 5 months ago (2014-07-10 06:10:08 UTC) #9
Jens Widell
On 2014/07/10 06:10:08, kangil_ wrote: > There seem not a cleaner way to return node ...
6 years, 5 months ago (2014-07-10 06:33:09 UTC) #10
kangil_
On 2014/07/10 06:33:09, Jens Lindström wrote: > Right, because the C++ implementations (e.g. Node::insertBefore()) doesn't ...
6 years, 5 months ago (2014-07-10 07:04:26 UTC) #11
Jens Widell
On 2014/07/10 07:04:26, kangil_ wrote: > On 2014/07/10 06:33:09, Jens Lindström wrote: > > Right, ...
6 years, 5 months ago (2014-07-10 07:13:53 UTC) #12
haraken
On 2014/07/10 07:13:53, Jens Lindström wrote: > On 2014/07/10 07:04:26, kangil_ wrote: > > On ...
6 years, 5 months ago (2014-07-10 07:17:57 UTC) #13
kangil_
Try best approach. PTAL.
6 years, 5 months ago (2014-07-10 08:59:21 UTC) #14
Jens Widell
https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp File Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp#newcode180 Source/core/dom/ContainerNode.cpp:180: Node* ContainerNode::sameNode(Node* node) I don't see why this is ...
6 years, 5 months ago (2014-07-10 09:14:55 UTC) #15
haraken
https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp File Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp#newcode190 Source/core/dom/ContainerNode.cpp:190: PassRefPtrWillBeRawPtr<Node> ContainerNode::insertBefore(PassRefPtrWillBeRawPtr<Node> newChild, Node* refChild, ExceptionState& exceptionState) insertBefore is ...
6 years, 5 months ago (2014-07-10 09:16:02 UTC) #16
Jens Widell
https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp File Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp#newcode202 Source/core/dom/ContainerNode.cpp:202: appendChild(newChild, exceptionState); We could deal with appendChild(), replaceChild() and ...
6 years, 5 months ago (2014-07-10 09:18:27 UTC) #17
kangil_
Thanks for the review comments. I was totally confused by specific test case. PTAL! https://codereview.chromium.org/375293002/diff/20001/Source/core/dom/ContainerNode.cpp ...
6 years, 5 months ago (2014-07-11 02:09:26 UTC) #18
haraken
LGTM https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/custom/V8NodeCustom.cpp File Source/bindings/core/v8/custom/V8NodeCustom.cpp (right): https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/custom/V8NodeCustom.cpp#newcode77 Source/bindings/core/v8/custom/V8NodeCustom.cpp:77: void V8Node::removeChildMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info) You can consider removing ...
6 years, 5 months ago (2014-07-11 02:18:24 UTC) #19
kangil_
PTAL https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/custom/V8NodeCustom.cpp File Source/bindings/core/v8/custom/V8NodeCustom.cpp (right): https://codereview.chromium.org/375293002/diff/60001/Source/bindings/core/v8/custom/V8NodeCustom.cpp#newcode77 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 ...
6 years, 5 months ago (2014-07-11 02:39:42 UTC) #20
haraken
LGTM
6 years, 5 months ago (2014-07-11 02:42:22 UTC) #21
kangil_
The CQ bit was checked by kangil.han@samsung.com
6 years, 5 months ago (2014-07-11 02:45:24 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kangil.han@samsung.com/375293002/80001
6 years, 5 months ago (2014-07-11 02:45:56 UTC) #23
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
6 years, 5 months ago (2014-07-11 03:52:59 UTC) #24
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-11 04:57:02 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/15954)
6 years, 5 months ago (2014-07-11 04:57:04 UTC) #26
kangil_
On 2014/07/11 04:57:04, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 5 months ago (2014-07-11 07:41:13 UTC) #27
kangil_
The CQ bit was checked by kangil.han@samsung.com
6 years, 5 months ago (2014-07-11 10:51:12 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kangil.han@samsung.com/375293002/100001
6 years, 5 months ago (2014-07-11 10:51:56 UTC) #29
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
6 years, 5 months ago (2014-07-11 11:59:00 UTC) #30
commit-bot: I haz the power
6 years, 5 months ago (2014-07-11 13:06:34 UTC) #31
Message was sent while issue was closed.
Change committed as 177940

Powered by Google App Engine
This is Rietveld 408576698