Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1085843002:
Implement DOM: prepend, append, before, after & replaceWith (Closed)
|
Created:
5 years, 8 months ago by Paritosh Kumar Modified:
5 years, 5 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionImplement DOM: prepend, append, before, after & replaceWith
As per https://dom.spec.whatwg.org/#childnode ChildNode interface
should contain before(), after() and replaceWith() API.
As per https://dom.spec.whatwg.org/#parentnode ParentNode interface
should contain prepend(), append() API.
Intent to Implement and ship link: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/paritosh/blink-dev/efUPtYm1PP8/MGoTi17AYpcJ
BUG=255482
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198629
Patch Set 1 : #Patch Set 2 : #Patch Set 3 #
Total comments: 21
Patch Set 4 : #Patch Set 5 #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #
Total comments: 10
Patch Set 8 : #
Total comments: 10
Patch Set 9 : #
Total comments: 14
Patch Set 10 : #Patch Set 11 : #
Total comments: 6
Patch Set 12 : #Patch Set 13 : #
Total comments: 15
Patch Set 14 : #Patch Set 15 : #
Total comments: 16
Patch Set 16 : #
Total comments: 3
Patch Set 17 #
Total comments: 4
Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Patch Set 22 : Last Reviewed #
Total comments: 2
Patch Set 23 : #Messages
Total messages: 65 (12 generated)
Patchset #1 (id:1) has been deleted
paritosh.in@samsung.com changed reviewers: + haraken@chromium.org, tkent@chromium.org
Please have a look.
Please send "Intent to implement and Ship:" mail to blink-dev.
On 2015/04/14 12:05:51, tkent wrote: > Please send "Intent to implement and Ship:" mail to blink-dev. will We include other ChildNode API, after() and replacewith() in the same Intent or in different Intent mail.
> will We include other ChildNode API, after() and replacewith() > in the same Intent or in different Intent mail. One intent mail is enough if you (or your team) are going to implement them.
On 2015/04/14 12:10:46, tkent wrote: > > will We include other ChildNode API, after() and replacewith() > > in the same Intent or in different Intent mail. > > One intent mail is enough if you (or your team) are going to implement them. Intent to Implement Link: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/efUPtYm1PP8
paritosh.in@samsung.com changed reviewers: + philipj@opera.com
On 2015/04/15 08:57:32, Paritosh Kumar wrote: > On 2015/04/14 12:10:46, tkent wrote: > > > will We include other ChildNode API, after() and replacewith() > > > in the same Intent or in different Intent mail. > > > > One intent mail is enough if you (or your team) are going to implement them. > > Intent to Implement Link: > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/efUPtYm1PP8 As https://code.google.com/p/chromium/issues/detail?id=462916 get fixed now, we can proceed on this implementation.
Do you intend to add all 5 APIs in this CL? If you're going to use [Unscopeable], it would also be nice if you could add it to ChildNode.remove().
- The CL subject has [WIP]. So, I assume this CL is not ready for review. - Use [Unscopeable] - Add a runtime flag for new ChildNode functions
Thanks tkent & Philip > Do you intend to add all 5 APIs Updated cl with other 4 APIs. > The CL subject has [WIP]. So, I assume this CL is not ready for review. Still WIP. Have to add some more tests. And runtime flag, you mentioned. > Use [Unscopeable] Using it in new cl. Please have a look, is it right way or not?
@tkent: Need some inputs.
When writing tests for this, I'm expecting
shouldThrow('test.prepend(null)'); should throw TypeError Exception.(right?)
But this is not throwing any exception, instead it treats
null as a string for NodeOrString, and creates a new Text node and
prepends this.
Actually problem is, when checking nodes[i].isString() it returns true for
null value of nodes[i], that we have passed through JS.
Please suggest..
On 2015/04/24 06:42:08, Paritosh Kumar wrote:
> shouldThrow('test.prepend(null)'); should throw TypeError Exception.(right?)
>
> But this is not throwing any exception, instead it treats
> null as a string for NodeOrString, and creates a new Text node and
> prepends this.
It's an expected behavior. See the specification.
http://heycam.github.io/webidl/#es-union
>> If types includes a string type, then return the result of converting V to
that type.
It would be good to write these tests using testharness.js so that they can be upstreamed to web-platform-tests: https://github.com/w3c/web-platform-tests/tree/master/dom/nodes You might even want to copy one of those.
Thanks tkent & Philip @tkent: > It's an expected behavior. See the specification. > http://heycam.github.io/webidl/#es-union Ok, Got It. Thanks. @Philip: > It would be good to write these tests using testharness.js so that they can be > upstreamed to web-platform-tests: > https://github.com/w3c/web-platform-tests/tree/master/dom/nodes Added tests using testharness.js. > You might even want to copy one of those. Should We copy the tests of remove()(in this patch or may be in another one), as we are not using testharness.js for remove() tests? have to add unit tests for newly introduced Union Type(i.e. NodeOrString), should we add this test in this cl only, or should create separate cl after landing this? Removing [WIP] from subject. Please have a look.
Some more feedback, sorry for the delay. https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... File LayoutTests/fast/dom/Comment/after.html (right): https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... LayoutTests/fast/dom/Comment/after.html:4: <script src="../script-tests/childnode-after.js"></script> This pattern is a bit atypical for testharness.js tests unless there is shared code between tests. You could just inline the tests in this file to make the test self-contained. (Anticipating the feedback you'd get if upstreaming to web-platform-tests.) https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... LayoutTests/fast/dom/Comment/after.html:8: node = document.createComment("node"); Indentation should be 2 or 4 spaces. (I tend to use 2 spaces in tests intended for web-platform-tests.) https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/ChildNo... File Source/core/dom/ChildNode.idl (right): https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/ChildNo... Source/core/dom/ChildNode.idl:29: [RaisesException, Unscopeable, CustomElementCallbacks] void before((Node or DOMString) ... Nodes); I like to put the per-spec bits first, so [Unscopeable, RaisesException, CustomElementCallbacks]. Do you know why [CustomElementCallbacks] is needed here? Also, all new bits need to be [RuntimeEnabled=something] to avoid shipping it. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:384: PassRefPtr<Node> Node::mutationMethodMacro(const Vector<NodeOrString>& nodes, ExceptionState& exceptionState) If I'm not mistaken, this should be PassRefPtrWillBeRawPtr<Node>, and the below should be RefPtrWillBeRawPtr<Node>. If in doubt, compile with enable_oilpan=1 in GYP_DEFINES. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:393: return nullptr; This case should be unreachable. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:395: node = DocumentFragment::create(document()); This is what the spec says, but implementing it like this means that any multi-argument call like elm.append(node1, node2) will create an unnecessary DocumentFragment object, which seems unfortunate. Did you look into how much work it would be to avoid this? Maybe collectChildrenAndRemoveFromOldParent would come in handy. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:396: for (size_t i = 0; i < nodes.size(); ++i) { Will a range-based for loop work here? for (NodeOrString& nodeOrString : nodes) https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:402: exceptionState.throwIfNeeded(); This looks unusual, why throwIfNeeded() here? If will boil down to just exceptionState.throwException(), which I also kind used anywhere in core. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:409: There's an extra blank line here. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:532: RefPtr<Node> nodeToPrepend = mutationMethodMacro(nodes, exceptionState); All of these will also need to use the Oilpan transitional types. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:533: this->insertBefore(nodeToPrepend, this->firstChild(), exceptionState); In the spec none of these are expressed in terms of the other Web-exposed methods but internals like https://dom.spec.whatwg.org/#concept-node-pre-insert in this case. If one looks at the spec one finds that it's equivalent, but it's nice when the implementation just does exactly what the spec says. I don't think this CL is a good place to do that refactoring, but if you annotate everything with spec links and spec steps, it will be more obvious that the use of insertBefore is intentional. Something like: // https://dom.spec.whatwg.org/#dom-parentnode-prependnodes // 1. Run the mutation method macro. RefPtrWillBeRawPtr<Node> nodeToPrepend = mutationMethodMacro(nodes, exceptionState); // 2. Pre-insert |node| into the context object before the context object’s first child. this->insertBefore(nodeToPrepend, this->firstChild(), exceptionState);
On 2015/04/29 13:16:57, philipj_UTC2 wrote: > Some more feedback, sorry for the delay. > > https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... > File LayoutTests/fast/dom/Comment/after.html (right): > > https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... > LayoutTests/fast/dom/Comment/after.html:4: <script > src="../script-tests/childnode-after.js"></script> > This pattern is a bit atypical for testharness.js tests unless there is shared > code between tests. You could just inline the tests in this file to make the > test self-contained. (Anticipating the feedback you'd get if upstreaming to > web-platform-tests.) > > https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... > LayoutTests/fast/dom/Comment/after.html:8: node = > document.createComment("node"); > Indentation should be 2 or 4 spaces. (I tend to use 2 spaces in tests intended > for web-platform-tests.) > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/ChildNo... > File Source/core/dom/ChildNode.idl (right): > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/ChildNo... > Source/core/dom/ChildNode.idl:29: [RaisesException, Unscopeable, > CustomElementCallbacks] void before((Node or DOMString) ... Nodes); > I like to put the per-spec bits first, so [Unscopeable, RaisesException, > CustomElementCallbacks]. Do you know why [CustomElementCallbacks] is needed > here? > > Also, all new bits need to be [RuntimeEnabled=something] to avoid shipping it. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cpp > File Source/core/dom/Node.cpp (right): > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:384: PassRefPtr<Node> Node::mutationMethodMacro(const > Vector<NodeOrString>& nodes, ExceptionState& exceptionState) > If I'm not mistaken, this should be PassRefPtrWillBeRawPtr<Node>, and the below > should be RefPtrWillBeRawPtr<Node>. If in doubt, compile with enable_oilpan=1 in > GYP_DEFINES. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:393: return nullptr; > This case should be unreachable. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:395: node = DocumentFragment::create(document()); > This is what the spec says, but implementing it like this means that any > multi-argument call like elm.append(node1, node2) will create an unnecessary > DocumentFragment object, which seems unfortunate. Did you look into how much > work it would be to avoid this? Maybe collectChildrenAndRemoveFromOldParent > would come in handy. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:396: for (size_t i = 0; i < nodes.size(); ++i) { > Will a range-based for loop work here? for (NodeOrString& nodeOrString : nodes) > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:402: exceptionState.throwIfNeeded(); > This looks unusual, why throwIfNeeded() here? If will boil down to just > exceptionState.throwException(), which I also kind used anywhere in core. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:409: > There's an extra blank line here. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:532: RefPtr<Node> nodeToPrepend = > mutationMethodMacro(nodes, exceptionState); > All of these will also need to use the Oilpan transitional types. > > https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... > Source/core/dom/Node.cpp:533: this->insertBefore(nodeToPrepend, > this->firstChild(), exceptionState); > In the spec none of these are expressed in terms of the other Web-exposed > methods but internals like https://dom.spec.whatwg.org/#concept-node-pre-insert > in this case. If one looks at the spec one finds that it's equivalent, but it's > nice when the implementation just does exactly what the spec says. > > I don't think this CL is a good place to do that refactoring, but if you > annotate everything with spec links and spec steps, it will be more obvious that > the use of insertBefore is intentional. Something like: > > // https://dom.spec.whatwg.org/#dom-parentnode-prependnodes > // 1. Run the mutation method macro. > RefPtrWillBeRawPtr<Node> nodeToPrepend = mutationMethodMacro(nodes, > exceptionState); > // 2. Pre-insert |node| into the context object before the context object’s > first child. > this->insertBefore(nodeToPrepend, this->firstChild(), exceptionState); Thanks Philip Sorry! I'm unable to update it till now. I'm working on this, I'll update on this, soon.
Apologize, for too delay in update. Actually, There was some problem in building this after change: https://codereview.chromium.org/1118993002 In this patch we are using HeapVector but, toImplArguments returns Vector for all types of arguments. For that I created one cl: https://codereview.chromium.org/1150863004/ I think after this we can proceed on this cl.
Patchset #4 (id:80001) has been deleted
Hi Philip and tkent Apologize, After a long time, I'm updating this cl. Please take a look. https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... File LayoutTests/fast/dom/Comment/after.html (right): https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... LayoutTests/fast/dom/Comment/after.html:4: <script src="../script-tests/childnode-after.js"></script> On 2015/04/29 13:16:57, philipj wrote: > This pattern is a bit atypical for testharness.js tests unless there is shared > code between tests. You could just inline the tests in this file to make the > test self-contained. (Anticipating the feedback you'd get if upstreaming to > web-platform-tests.) Done. Thanks. https://codereview.chromium.org/1085843002/diff/60001/LayoutTests/fast/dom/Co... LayoutTests/fast/dom/Comment/after.html:8: node = document.createComment("node"); On 2015/04/29 13:16:57, philipj wrote: > Indentation should be 2 or 4 spaces. (I tend to use 2 spaces in tests intended > for web-platform-tests.) Done. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/ChildNo... File Source/core/dom/ChildNode.idl (right): https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/ChildNo... Source/core/dom/ChildNode.idl:29: [RaisesException, Unscopeable, CustomElementCallbacks] void before((Node or DOMString) ... Nodes); On 2015/04/29 13:16:57, philipj wrote: > I like to put the per-spec bits first, so [Unscopeable, RaisesException, CustomElementCallbacks]. Done. > Do you know why [CustomElementCallbacks] is needed here? I think, It will result in an element being inserted, so It could enqueue a Custom Element callback. Tagging with CustomElementCallbacks will invoke the callbacks before returning to script. right? > Also, all new bits need to be [RuntimeEnabled=something] to avoid shipping it. Added, RuntimeEnabled=DOMConvenienceAPI. Thanks. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:384: PassRefPtr<Node> Node::mutationMethodMacro(const Vector<NodeOrString>& nodes, ExceptionState& exceptionState) On 2015/04/29 13:16:57, philipj wrote: > If I'm not mistaken, this should be PassRefPtrWillBeRawPtr<Node>, and the > below should be RefPtrWillBeRawPtr<Node>. Yes, This will be PassRefPtrWillBeRawPtr<Node> and RefPtrWillBeRawPtr<Node>. > If in doubt, compile with enable_oilpan=1 in GYP_DEFINES. Ohh, great. Thanks. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:393: return nullptr; On 2015/04/29 13:16:57, philipj wrote: > This case should be unreachable. Yes, Removing it. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:395: node = DocumentFragment::create(document()); On 2015/04/29 13:16:57, philipj wrote: > This is what the spec says, but implementing it like this means that any > multi-argument call like elm.append(node1, node2) will create an unnecessary > DocumentFragment object, which seems unfortunate. Did you look into how much > work it would be to avoid this? Maybe collectChildrenAndRemoveFromOldParent > would come in handy. I looked into this and found that we may use collectChildrenAndRemoveFromOldParent as in insertBefore. But, our mutationMethodMacro will do only convertion of DOMString to Text Node. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:396: for (size_t i = 0; i < nodes.size(); ++i) { On 2015/04/29 13:16:57, philipj wrote: > Will a range-based for loop work here? for (NodeOrString& nodeOrString : nodes) Yes, Done. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:402: exceptionState.throwIfNeeded(); On 2015/04/29 13:16:57, philipj wrote: > This looks unusual, why throwIfNeeded() here? If will boil down to just > exceptionState.throwException(), which I also kind used anywhere in core. Done. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:409: On 2015/04/29 13:16:57, philipj wrote: > There's an extra blank line here. Done. https://codereview.chromium.org/1085843002/diff/60001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:532: RefPtr<Node> nodeToPrepend = mutationMethodMacro(nodes, exceptionState); On 2015/04/29 13:16:57, philipj wrote: > All of these will also need to use the Oilpan transitional types. Done.
A lot of the tests look similar and ProcessingInstruction is a kind of ChildNode not tested. Could you try to generalize the tests into something like LayoutTests/fast/dom/ChildNode/after.html that tests all kinds of ChildNodes using the very same code path? The same for ParentNode, of which there are three kinds. https://codereview.chromium.org/1085843002/diff/120001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/120001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:401: exceptionState.throwDOMException(exceptionState.code(), exceptionState.message()); Is there any condition under which this exception would be thrown? I suspect that there isn't and that ASSERT_NO_EXCEPTION could be used, in which case mutationMethodMacro doesn't need an ExceptionState& argument.
Thanks Philip, > A lot of the tests look similar and ProcessingInstruction is a kind of ChildNode > not tested. Could you try to generalize the tests into something like > LayoutTests/fast/dom/ChildNode/after.html that tests all kinds of ChildNodes > using the very same code path? The same for ParentNode, of which there are three > kinds. Ok, I'll update this. But many ChildNode and ParentNode tests are in different directories. Will we collect all them together in this cl? Please suggest. > https://codereview.chromium.org/1085843002/diff/120001/Source/core/dom/Node.c... > Source/core/dom/Node.cpp:401: > exceptionState.throwDOMException(exceptionState.code(), > exceptionState.message()); > Is there any condition under which this exception would be thrown? I suspect > that there isn't and that ASSERT_NO_EXCEPTION could be used, in which case > mutationMethodMacro doesn't need an ExceptionState& argument. No, this was possible in case of null, that we discussed on https://codereview.chromium.org/1085843002/#msg14 Now, It seems useless and we can use ASSERT_NO_EXCEPTION. Will update in next cl. Thanks.
@Philip: Updated as per your suggestion, Please have a look.
Some comments on LayoutTests/fast/dom/ChildNode/after.html that apply to all tests. I also see some cases not tested: 1. Each function called with no arguments. 2. before/after/replaceWith/remove called when the context object has no parent node. There could be more, in general make sure that for every line of the spec or the implementation, there is nothing about it that could change without a test failing. https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:10: assert_equals(node.after.length, 0); This makes me thing that after() can be called with no arguments... However, I don't think this test adds much, global-interface-listing.html will prove that the method is on the correct interface objects, and a test with no arguments will show that this is possible and what should happen. https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:11: }, "Comment should support after()"); This and the following tests are on the same form, try rewriting this as function test_after(node, nodeName) { test(function() { ... }, nodeName + ".after() with this or that argument"); } test_after(document.createComment('node'), "Comment"); test_after(document.createElement('node'), "Element"); test_after(new Text('node'), "Text"); Or similar. This should remove many lines of duplicated code if done for all tests. https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:36: assert_equals(parent.childNodes.length, 4, 'parent should have 4 children'); I think you could write these assertions using test.innerHTML to be more readable. https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/ChildN... File Source/core/dom/ChildNode.idl (right): https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/ChildN... Source/core/dom/ChildNode.idl:29: [Unscopeable, RaisesException, CustomElementCallbacks, RuntimeEnabled=DOMConvenienceAPI] void before((Node or DOMString) ... Nodes); Nodes should not be capitalized here. https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:388: if (nodes.size() == 1) { If the bindings guarantees that nodes.size()!=0 here, please ASSERT it, as otherwise one wonders if an empty DocumentFragment is the right thing for when there are no arguments. Also make sure to test the zero-argument case.
The spec just changed in response to the bug you found: https://github.com/whatwg/dom/commit/2ace8fa7dde98eeaf319cc6b1f4cfe16a3264cb4 Can you update this CL to match the new spec?
Thanks Philip for clarification on spec. Updated as per your suggestion. Please take a look. https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:10: assert_equals(node.after.length, 0); On 2015/06/09 12:48:31, philipj wrote: > This makes me thing that after() can be called with no arguments... However, I > don't think this test adds much, global-interface-listing.html will prove that > the method is on the correct interface objects, and a test with no arguments > will show that this is possible and what should happen. Thanks. Updated. https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:11: }, "Comment should support after()"); On 2015/06/09 12:48:31, philipj wrote: > This and the following tests are on the same form, try rewriting this as > > function test_after(node, nodeName) { > test(function() { > ... > }, nodeName + ".after() with this or that argument"); > } > test_after(document.createComment('node'), "Comment"); > test_after(document.createElement('node'), "Element"); > test_after(new Text('node'), "Text"); > > Or similar. This should remove many lines of duplicated code if done for all > tests. Done. https://codereview.chromium.org/1085843002/diff/160001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:36: assert_equals(parent.childNodes.length, 4, 'parent should have 4 children'); On 2015/06/09 12:48:31, philipj wrote: > I think you could write these assertions using test.innerHTML to be more > readable. Done. https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/ChildN... File Source/core/dom/ChildNode.idl (right): https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/ChildN... Source/core/dom/ChildNode.idl:29: [Unscopeable, RaisesException, CustomElementCallbacks, RuntimeEnabled=DOMConvenienceAPI] void before((Node or DOMString) ... Nodes); On 2015/06/09 12:48:32, philipj wrote: > Nodes should not be capitalized here. Done. https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/160001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:388: if (nodes.size() == 1) { On 2015/06/09 12:48:32, philipj wrote: > If the bindings guarantees that nodes.size()!=0 here, please ASSERT it, as > otherwise one wonders if an empty DocumentFragment is the right thing for when > there are no arguments. Also make sure to test the zero-argument case. Updated as per new specs.
https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:8: var innerHtml; Maybe capitalize as innerHTML to match the API. https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:26: }, nodeName + '.after() with empty argument.'); s/empty/no/ to distinguish it from an empty string or similar https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:60: }, nodeName + '.after() with only one element and text as argument.'); I think drop the "only" in the name of this test. And plural "arguments". https://codereview.chromium.org/1085843002/diff/180001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/180001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:389: if (nodes[0].isNode()) This text->node conversion is a separate step in the spec, but I realize writing it as such would require creating a new vector. However, you could still have a helper taking a NodeOrString and returning a Node, which should make for less code in this function, and a single call to appendChild() below. I could be missing something, but I think that should work. https://codereview.chromium.org/1085843002/diff/180001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:554: void Node::replaceWith(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState) This may change: https://github.com/whatwg/dom/issues/32#issuecomment-111518557 No need to wait until that's been done, though.
Thanks Philip Updated as you suggested. Please have a look. https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:8: var innerHtml; On 2015/06/12 16:12:33, philipj wrote: > Maybe capitalize as innerHTML to match the API. Done. https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:26: }, nodeName + '.after() with empty argument.'); On 2015/06/12 16:12:33, philipj wrote: > s/empty/no/ to distinguish it from an empty string or similar Thanks. Using 'without any argument'. https://codereview.chromium.org/1085843002/diff/180001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:60: }, nodeName + '.after() with only one element and text as argument.'); On 2015/06/12 16:12:33, philipj wrote: > I think drop the "only" in the name of this test. And plural "arguments". Yeah. Thanks. https://codereview.chromium.org/1085843002/diff/180001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/180001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:389: if (nodes[0].isNode()) On 2015/06/12 16:12:33, philipj wrote: > This text->node conversion is a separate step in the spec, > but I realize writing it as such would require creating a > new vector. Yes. > However, you could still have a > helper taking a NodeOrString and returning a Node, which > should make for less code in this function, and a single > call to appendChild() below. Created one helper method Node::nodeOrStringToNode, which takes NodeOrString and returns a Node. https://codereview.chromium.org/1085843002/diff/180001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:554: void Node::replaceWith(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState) On 2015/06/12 16:12:33, philipj wrote: > This may change: https://github.com/whatwg/dom/issues/32#issuecomment-111518557 > > No need to wait until that's been done, though. Thanks for info. I'll also watch this.
Some more teeny tiny nits, but this looks pretty much done. https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:34: }, nodeName + '.after() with null as an argument.'); A test with an empty string would also be good, to ensure this creates an empty text node. (That doesn't make much sense, if you think it's terrible maybe open a spec bug.) https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:63: test_after(document.createElement('div'), 'Comment'); I see that each test uses a new div, so you could remove this argument and just do "var parent = document.createElement('div')" instead. https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/replace-with.html (right): https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/replace-with.html:24: expected = ""; Hmm, I think you can just fold the expected bit into the assert_equals here and elsewhere. (If not, it's missing a leading var.) https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/replace-with.html:26: }, nodeName + '.replaceWith() without any argument.'); Also need to test child.replaceWith(something) when child does not have a parent. This is true of before/after too. https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append.html (right): https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:13: }, nodeName + '.append() without any argument, on a parent having no child.'); There are double spaces where and in some other places. https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:57: test_append(document.createElement('div'), 'Element'); What about Document and DocumentFragment? https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:396: node = nodeOrStringToNode(nodes[0]); The spec is on this form, but would this be clearer with a simple return nodeOrStringToNode(nodes[0]) here? It should also avoid some ref counting churn in the case where the single argument is a node, which ought to be a common case. Then the below can begin "RefPtrWillBeRawPtr<DocumentFragment> fragment = DocumentFragment::create(document())" https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.h File Source/core/dom/Node.h (right): https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.h... Source/core/dom/Node.h:216: PassRefPtrWillBeRawPtr<Node> nodeOrStringToNode(const NodeOrString&); Can't these be static in the .cpp file?
On 2015/06/15 10:06:58, philipj wrote: > Some more teeny tiny nits, but this looks pretty much done. Ah, spec changed again: https://github.com/whatwg/dom/commit/30ab745e9c252295155f388b901b15de4d691d6a
Thanks Philip for updating about spec change. I tried to implement according to new spec change. And updated the cl. Please have a look. https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:63: test_after(document.createElement('div'), 'Comment'); On 2015/06/15 10:06:58, philipj (away until June 23) wrote: > I see that each test uses a new div, so you could remove this argument and just > do "var parent = document.createElement('div')" instead. Done. https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append.html (right): https://codereview.chromium.org/1085843002/diff/200001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:13: }, nodeName + '.append() without any argument, on a parent having no child.'); On 2015/06/15 10:06:58, philipj (away until June 23) wrote: > There are double spaces where and in some other places. Done. https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:396: node = nodeOrStringToNode(nodes[0]); On 2015/06/15 10:06:58, philipj (away until June 23) wrote: > The spec is on this form, but would this be clearer with a simple return > nodeOrStringToNode(nodes[0]) here? It should also avoid some ref counting churn > in the case where the single argument is a node, which ought to be a common > case. > > Then the below can begin "RefPtrWillBeRawPtr<DocumentFragment> fragment = > DocumentFragment::create(document())" Thanks. Done. https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.h File Source/core/dom/Node.h (right): https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.h... Source/core/dom/Node.h:216: PassRefPtrWillBeRawPtr<Node> nodeOrStringToNode(const NodeOrString&); On 2015/06/15 10:06:58, philipj (away until June 23) wrote: > Can't these be static in the .cpp file? As I'm using Node::document() method in nodeOrStringToNode method to create a Text node, I'm not making this static. We can pass document() in this method. If it will look fine then we can use it. Please share your opinion.
https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.h File Source/core/dom/Node.h (right): https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.h... Source/core/dom/Node.h:216: PassRefPtrWillBeRawPtr<Node> nodeOrStringToNode(const NodeOrString&); On 2015/06/23 13:32:09, Paritosh Kumar wrote: > On 2015/06/15 10:06:58, philipj (away until June 23) wrote: > > Can't these be static in the .cpp file? > > As I'm using Node::document() method in nodeOrStringToNode method to create a > Text node, I'm not making this static. We can pass document() in this method. If > it will look fine then we can use it. Please share your opinion. Oh, you need the Document to create the Text nodes. Never mind my suggestion then.
Some more comments. When I fiddled with the code around viable siblings I also checked how ugly it would be to make nodeOrStringToNode and convertNodesIntoNode static and it's really just passing and extra Document& to create things with, so I think that's worth it to not have extra stuff in the header. https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/200001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:396: node = nodeOrStringToNode(nodes[0]); On 2015/06/23 13:32:09, Paritosh Kumar wrote: > On 2015/06/15 10:06:58, philipj (away until June 23) wrote: > > The spec is on this form, but would this be clearer with a simple return > > nodeOrStringToNode(nodes[0]) here? It should also avoid some ref counting > churn > > in the case where the single argument is a node, which ought to be a common > > case. > > > > Then the below can begin "RefPtrWillBeRawPtr<DocumentFragment> fragment = > > DocumentFragment::create(document())" > > Thanks. Done. Looks good this way, thanks! https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:392: Node* Node::viablePreviosOrNextSibling(const HeapVector<NodeOrString>& nodes, bool next) The bool argument isn't great. I applied your patch and fiddled a little bit and came up with this: static bool isNodeInNodes(const Node& node, const HeapVector<NodeOrString>& nodes) { for (NodeOrString nodeOrString : nodes) { if (nodeOrString.isNode() && nodeOrString.getAsNode() == &node) return true; } return false; } static Node* viablePreviousSibling(const Node& node, const HeapVector<NodeOrString>& nodes) { for (Node* sibling = node.previousSibling(); sibling; sibling = sibling->previousSibling()) { if (!isNodeInNodes(*sibling, nodes)) return sibling; } return nullptr; } static Node* viableNextSibling(const Node& node, const HeapVector<NodeOrString>& nodes) { for (Node* sibling = node.nextSibling(); sibling; sibling = sibling->nextSibling()) { if (!isNodeInNodes(*sibling, nodes)) return sibling; } return nullptr; } WDYT? https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:399: if (nodeOrStringToNode(nodeOrString)->isSameNode(node)) { It's a waste to convert string to nodes here, instead check if it's a node and if so compare it with node. Also just use == as that's exactly what isSameNode does. https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:581: Node* viableNextSibling = viablePreviosOrNextSibling(nodes, true /* Next */); Here viableNextSibling and nodeToReplaceWith can be moved into the branches where they are actually used.
Thanks Philip Updated as per your suggestion, Please have a look. https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:392: Node* Node::viablePreviosOrNextSibling(const HeapVector<NodeOrString>& nodes, bool next) On 2015/06/25 13:13:23, philipj wrote: > The bool argument isn't great. I applied your patch and fiddled a little bit and > came up with this: > > static bool isNodeInNodes(const Node& node, const HeapVector<NodeOrString>& > nodes) > { > for (NodeOrString nodeOrString : nodes) { > if (nodeOrString.isNode() && nodeOrString.getAsNode() == &node) > return true; > } > return false; > } > > static Node* viablePreviousSibling(const Node& node, const > HeapVector<NodeOrString>& nodes) > { > for (Node* sibling = node.previousSibling(); sibling; sibling = > sibling->previousSibling()) { > if (!isNodeInNodes(*sibling, nodes)) > return sibling; > } > return nullptr; > } > > static Node* viableNextSibling(const Node& node, const HeapVector<NodeOrString>& > nodes) > { > for (Node* sibling = node.nextSibling(); sibling; sibling = > sibling->nextSibling()) { > if (!isNodeInNodes(*sibling, nodes)) > return sibling; > } > return nullptr; > } > > WDYT? Yes, It looks great. Thanks. https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:399: if (nodeOrStringToNode(nodeOrString)->isSameNode(node)) { On 2015/06/25 13:13:23, philipj wrote: > It's a waste to convert string to nodes here, instead check if it's a node and > if so compare it with node. Also just use == as that's exactly what isSameNode > does. Done. https://codereview.chromium.org/1085843002/diff/240001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:581: Node* viableNextSibling = viablePreviosOrNextSibling(nodes, true /* Next */); On 2015/06/25 13:13:23, philipj wrote: > Here viableNextSibling and nodeToReplaceWith can be moved into the branches > where they are actually used. Done.
Getting better and better. I haven't looked closely at the tests this time, if you haven't already please be sure that there are tests covering any corner cases resulting from the viableNextSibling logic. Not sure what they are, but at least such that all the code paths in the implementation are exercised. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:385: static bool isNodeInNodes(const Node& node, const HeapVector<NodeOrString>& nodes) Maybe move this block of statics to just before Node::prepend where they're used? https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:387: for (NodeOrString nodeOrString : nodes) { I guess this should be "for (const NodeOrString& nodeOrString : nodes)" like in the other loop. My bad. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:424: RefPtrWillBeRawPtr<Node> node = DocumentFragment::create(document); For clarity I would name this fragment instead of node. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:551: this->insertBefore(convertNodesIntoNode(nodes, document()), this->firstChild(), exceptionState); Is "this->" needed? https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:564: Node* viablePreviousSiblingNode = viablePreviousSibling(*this, nodes); Can this bit be written like the spec, with an |viablePreviousSiblingNode = viablePreviousSiblingNode->nextSibling() : parent->firstChild()|? That should result in a single call to insertBefore and thus a single call to convertNodesIntoNode. If there was a bug with the missing nextSibling() call, make sure there's a test that fails if that bug is present. On another topic, if you think it would look nicer (matching the spec) you might consider renaming the variable to viablePreviousSibling and the helper method to findViablePreviousSibling() or similar. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:586: if (parent == parentNode()) The spec says "If context object’s parent is parent, replace the context object with node within parent" here but it seems very strange. Nothing between the "parent = parentNode()" and this "parent == parentNode()" check could actually have changed parent or parentNode(), could it? Spec bug?
Thanks Philip Updated the cl as per comments. Please have a look. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:385: static bool isNodeInNodes(const Node& node, const HeapVector<NodeOrString>& nodes) On 2015/06/26 14:13:25, philipj wrote: > Maybe move this block of statics to just before Node::prepend where they're > used? Done. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:387: for (NodeOrString nodeOrString : nodes) { On 2015/06/26 14:13:26, philipj wrote: > I guess this should be "for (const NodeOrString& nodeOrString : nodes)" like in > the other loop. My bad. Ohh, sorry. Thanks. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:424: RefPtrWillBeRawPtr<Node> node = DocumentFragment::create(document); On 2015/06/26 14:13:26, philipj wrote: > For clarity I would name this fragment instead of node. Done. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:551: this->insertBefore(convertNodesIntoNode(nodes, document()), this->firstChild(), exceptionState); On 2015/06/26 14:13:26, philipj wrote: > Is "this->" needed? No. https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:564: Node* viablePreviousSiblingNode = viablePreviousSibling(*this, nodes); On 2015/06/26 14:13:26, philipj wrote: > Can this bit be written like the spec, with an |viablePreviousSiblingNode = > viablePreviousSiblingNode->nextSibling() : parent->firstChild()|? That should > result in a single call to insertBefore and thus a single call to > convertNodesIntoNode. > > If there was a bug with the missing nextSibling() call, make sure there's a test > that fails if that bug is present. > > On another topic, if you think it would look nicer (matching the spec) you might > consider renaming the variable to viablePreviousSibling and the helper method to > findViablePreviousSibling() or similar. Changed with conditional. Yes, this should be viablePreviousSibling->nextSibling(). https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:586: if (parent == parentNode()) On 2015/06/26 14:13:26, philipj wrote: > The spec says "If context object’s parent is parent, replace the context object > with node within parent" here but it seems very strange. Nothing between the > "parent = parentNode()" and this "parent == parentNode()" check could actually > have changed parent or parentNode(), could it? Spec bug? Hmmm, This looks very strange but @Annevk has taken this from https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724... as per his comment on https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724..., "I like @inexorabletash's this.parent === parent check to preserve the replace mutation records where possible." But I'm not sure, this line is required in our implementation.
https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:551: this->insertBefore(convertNodesIntoNode(nodes, document()), this->firstChild(), exceptionState); On 2015/06/30 10:49:09, Paritosh Kumar wrote: > On 2015/06/26 14:13:26, philipj wrote: > > Is "this->" needed? > > No. How about the "this->" at the beginning of the line here and in append()? https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:564: Node* viablePreviousSiblingNode = viablePreviousSibling(*this, nodes); On 2015/06/30 10:49:09, Paritosh Kumar wrote: > On 2015/06/26 14:13:26, philipj wrote: > > Can this bit be written like the spec, with an |viablePreviousSiblingNode = > > viablePreviousSiblingNode->nextSibling() : parent->firstChild()|? That should > > result in a single call to insertBefore and thus a single call to > > convertNodesIntoNode. > > > > If there was a bug with the missing nextSibling() call, make sure there's a > test > > that fails if that bug is present. > > > > On another topic, if you think it would look nicer (matching the spec) you > might > > consider renaming the variable to viablePreviousSibling and the helper method > to > > findViablePreviousSibling() or similar. > > Changed with conditional. Yes, this should be > viablePreviousSibling->nextSibling(). I see that you changed the before.html test, but can you confirm that it would fail with the original bug in place? https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:586: if (parent == parentNode()) On 2015/06/30 10:49:09, Paritosh Kumar wrote: > On 2015/06/26 14:13:26, philipj wrote: > > The spec says "If context object’s parent is parent, replace the context > object > > with node within parent" here but it seems very strange. Nothing between the > > "parent = parentNode()" and this "parent == parentNode()" check could actually > > have changed parent or parentNode(), could it? Spec bug? > > Hmmm, This looks very strange but @Annevk has taken this from > https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724... > as per his comment on > https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724..., > "I like @inexorabletash's this.parent === parent check to preserve the replace > mutation records where possible." > > But I'm not sure, this line is required in our implementation. Discussed with Anne on IRC: http://krijnhoetmer.nl/irc-logs/whatwg/20150702#l-440 This is because |this| could have been inserted into the fragment by the conversion. This makes it very important that convertNodesIntoNode is called before the check, so if these steps can be structured more like the spec that would probably do it. Please also be sure to write a test that fails with the current code but passes with the fixed code.
Thanks Philip. Updated, Please have a look. On 2015/07/02 09:20:01, philipj wrote: > https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.cpp > File Source/core/dom/Node.cpp (right): > > https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... > Source/core/dom/Node.cpp:551: this->insertBefore(convertNodesIntoNode(nodes, > document()), this->firstChild(), exceptionState); > On 2015/06/30 10:49:09, Paritosh Kumar wrote: > > On 2015/06/26 14:13:26, philipj wrote: > > > Is "this->" needed? > > > > No. > > How about the "this->" at the beginning of the line here and in append()? Ok. Done. > https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... > Source/core/dom/Node.cpp:564: Node* viablePreviousSiblingNode = > viablePreviousSibling(*this, nodes); > On 2015/06/30 10:49:09, Paritosh Kumar wrote: > > On 2015/06/26 14:13:26, philipj wrote: > > > Can this bit be written like the spec, with an |viablePreviousSiblingNode = > > > viablePreviousSiblingNode->nextSibling() : parent->firstChild()|? That > should > > > result in a single call to insertBefore and thus a single call to > > > convertNodesIntoNode. > > > > > > If there was a bug with the missing nextSibling() call, make sure there's a > > test > > > that fails if that bug is present. > > > > > > On another topic, if you think it would look nicer (matching the spec) you > > might > > > consider renaming the variable to viablePreviousSibling and the helper > method > > to > > > findViablePreviousSibling() or similar. > > > > Changed with conditional. Yes, this should be > > viablePreviousSibling->nextSibling(). > > I see that you changed the before.html test, but can you confirm that it would > fail with the original bug in place? Yes, it will fail without viablePreviousSibling->nextSibling(). > https://codereview.chromium.org/1085843002/diff/280001/Source/core/dom/Node.c... > Source/core/dom/Node.cpp:586: if (parent == parentNode()) > On 2015/06/30 10:49:09, Paritosh Kumar wrote: > > On 2015/06/26 14:13:26, philipj wrote: > > > The spec says "If context object’s parent is parent, replace the context > > object > > > with node within parent" here but it seems very strange. Nothing between the > > > "parent = parentNode()" and this "parent == parentNode()" check could > actually > > > have changed parent or parentNode(), could it? Spec bug? > > > > Hmmm, This looks very strange but @Annevk has taken this from > > > https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724... > > as per his comment on > > > https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724..., > > "I like @inexorabletash's this.parent === parent check to preserve the replace > > mutation records where possible." > > > > But I'm not sure, this line is required in our implementation. > > Discussed with Anne on IRC: > http://krijnhoetmer.nl/irc-logs/whatwg/20150702#l-440 > > This is because |this| could have been inserted into the fragment by the > conversion. This makes it very important that convertNodesIntoNode is called > before the check, so if these steps can be structured more like the spec that > would probably do it. > > Please also be sure to write a test that fails with the current code but passes > with the fixed code. Ohhk, Got It !! convertNodesIntoNode may cause change. Added a test with child itself as a parameter of replaceWith. Thanks Philip for sharing this info.
Code looks good, just some possible improvements to the tests. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:33: }, nodeName + '.after() with null as an argument.'); after() with the empty string as an argument would also be a good test. It should insert an empty text node per spec, and you can't test that by checking innerHTML, instead you'll have to look at parent.lastChild or something. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:72: }, nodeName + '.after() containing sibling of child as arguments.'); In before.html the equivalent test is called '.before() with all sibling of child as arguments.' which I think is more descriptive. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:85: }, nodeName + '.after() with one sibling of child and text as arguments.'); A test that includes the context object itself as the argument would be good, something as simple as child.after('text', child) which should be equivalent to child.before('text') Another case worth testing is if you have siblings a, b, c and call a.after(c, b). Then viableNextSibling should be null, and you verify that the pre-insert that follows behaves like an append, so the resulting order is a, c, b. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/before.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/before.html:33: }, nodeName + '.before() with null as an argument.'); The comments for after.html mostly apply here as well. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/replace-with.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/replace-with.html:32: }, nodeName + '.replaceWith() with null as an argument.'); Actually undefined might be worth testing too, because we don't handle that correctly for optional arguments and there may be problems here too: https://code.google.com/p/chromium/issues/detail?id=335871 http://heycam.github.io/webidl/ says "Optional arguments corresponding to a final, variadic argument do not treat undefined as a special “missing” value, however. The undefined value is converted to the type of variadic argument as would be done for a non-optional argument." so I think that child.replaceWith(undefined) should be treated as child.replaceWith('undefined'). https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:69: test_append(document, 'Document'); If you pass in document.implementation.createDocument(null, null) I think you'll be able to run more of the tests for Document, at the least the ones that append only one child. https://codereview.chromium.org/1085843002/diff/320001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/320001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:583: RefPtrWillBeRawPtr<Node> nodeToReplaceWith = convertNodesIntoNode(nodes, document()); The node is either replaced with or inserted before, so just naming this "node" like in the spec seems more accurate.
Thanks Philip Updated cl, please have a look. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:33: }, nodeName + '.after() with null as an argument.'); On 2015/07/03 21:52:05, philipj wrote: > after() with the empty string as an argument would also be a good test. It > should insert an empty text node per spec, and you can't test that by checking > innerHTML, instead you'll have to look at parent.lastChild or something. Done. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:72: }, nodeName + '.after() containing sibling of child as arguments.'); On 2015/07/03 21:52:05, philipj wrote: > In before.html the equivalent test is called '.before() with all sibling of > child as arguments.' which I think is more descriptive. Done. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:85: }, nodeName + '.after() with one sibling of child and text as arguments.'); On 2015/07/03 21:52:05, philipj wrote: > A test that includes the context object itself as the argument would be good, > something as simple as child.after('text', child) which should be equivalent to > child.before('text') > > Another case worth testing is if you have siblings a, b, c and call a.after(c, > b). Then viableNextSibling should be null, and you verify that the pre-insert > that follows behaves like an append, so the resulting order is a, c, b. Done. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/before.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/before.html:33: }, nodeName + '.before() with null as an argument.'); On 2015/07/03 21:52:05, philipj wrote: > The comments for after.html mostly apply here as well. Done. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/replace-with.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/replace-with.html:32: }, nodeName + '.replaceWith() with null as an argument.'); On 2015/07/03 21:52:05, philipj wrote: > Actually undefined might be worth testing too, because we don't handle that > correctly for optional arguments and there may be problems here too: > https://code.google.com/p/chromium/issues/detail?id=335871 > > http://heycam.github.io/webidl/ says "Optional arguments corresponding to a > final, variadic argument do not treat undefined as a special “missing” value, > however. The undefined value is converted to the type of variadic argument as > would be done for a non-optional argument." so I think that > child.replaceWith(undefined) should be treated as > child.replaceWith('undefined'). Done. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:69: test_append(document, 'Document'); On 2015/07/03 21:52:05, philipj wrote: > If you pass in document.implementation.createDocument(null, null) I think you'll > be able to run more of the tests for Document, at the least the ones that append > only one child. I tried this way, but these tests are still failing with cause, 'Nodes of type #text may not be inserted inside nodes of type #document'. Is there any other way in which we can try it? https://codereview.chromium.org/1085843002/diff/320001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/320001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:583: RefPtrWillBeRawPtr<Node> nodeToReplaceWith = convertNodesIntoNode(nodes, document()); On 2015/07/03 21:52:06, philipj wrote: > The node is either replaced with or inserted before, so just naming this "node" > like in the spec seems more accurate. Replaced.
Looking good. tkent@, I think this is pretty much done now, can you please give a second opinion? https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:72: }, nodeName + '.after() containing sibling of child as arguments.'); On 2015/07/06 09:02:27, Paritosh Kumar wrote: > On 2015/07/03 21:52:05, philipj wrote: > > In before.html the equivalent test is called '.before() with all sibling of > > child as arguments.' which I think is more descriptive. > > Done. Oh, it should be a plural "siblings" here and in the other test as well. https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append.html (right): https://codereview.chromium.org/1085843002/diff/320001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:69: test_append(document, 'Document'); On 2015/07/06 09:02:27, Paritosh Kumar wrote: > On 2015/07/03 21:52:05, philipj wrote: > > If you pass in document.implementation.createDocument(null, null) I think > you'll > > be able to run more of the tests for Document, at the least the ones that > append > > only one child. > > I tried this way, but these tests are still failing with cause, 'Nodes of type > #text may not be inserted inside nodes of type #document'. > Is there any other way in which we can try it? I see, on Document you can only append a single Element. Perhaps it would make sense to move the Document tests to a separate test and only test a smaller subset, and also verify that exceptions are thrown for things like doc.append(element, 'text'), and to check if that document is left with element appended or in its original state. https://codereview.chromium.org/1085843002/diff/340001/LayoutTests/fast/dom/C... File LayoutTests/fast/dom/ChildNode/after.html (right): https://codereview.chromium.org/1085843002/diff/340001/LayoutTests/fast/dom/C... LayoutTests/fast/dom/ChildNode/after.html:107: }, nodeName + '.after() when pre-insert behaves like append.'); Thumbs up! https://codereview.chromium.org/1085843002/diff/340001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append.html (right): https://codereview.chromium.org/1085843002/diff/340001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append.html:72: }, nodeName + '.append() with undefined as an argument, on a parent having a child.'); I don't think two tests for append(undefined) are really needed, this is really to test that bindings does the right this with undefined, after that I wouldn't expect anything special about undefined compared to any string. https://codereview.chromium.org/1085843002/diff/340001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/prepend.html (right): https://codereview.chromium.org/1085843002/diff/340001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/prepend.html:72: }, nodeName + '.prepend() with undefined as an argument, on a parent having a child.'); Ditto.
Thanks Philip Updated append.html and prepend.html, to exclude Document test. Added new files append-on-Document.html and prepend.Document.html containing tests for Document. Updated the other parts of cl as per your suggestion. Please have a look.
LGTM % nits, which also apply to the other new test. https://codereview.chromium.org/1085843002/diff/360001/LayoutTests/fast/dom/P... File LayoutTests/fast/dom/ParentNode/append-on-document.html (right): https://codereview.chromium.org/1085843002/diff/360001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append-on-document.html:20: assert_equals(parent.childNodes.length, 1); Checking childNodes.length is redundant with assert_array_equals, so if you could use just the latter that would be nice. I notice now there's some of this in other tests too. https://codereview.chromium.org/1085843002/diff/360001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append-on-document.html:36: assert_throws('HierarchyRequestError', function() { parent.append('text'); }, 'Nodes of type \'#text\' may not be inserted inside nodes of type \'#document\''); This seems to be the message of the exception. I don't think you need to write anything at all here, or in the other assert_throws() cases. Unlike some kinds of tests, it's at least not necessary to match the message. https://codereview.chromium.org/1085843002/diff/360001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append-on-document.html:39: }, 'Document.append() with text as an argument, on a Document having no child.'); Two spaces in "with text" https://codereview.chromium.org/1085843002/diff/360001/LayoutTests/fast/dom/P... LayoutTests/fast/dom/ParentNode/append-on-document.html:48: }, 'Document.append() with only one element as an argument, on a Document having no child.'); Description is wrong, there are two arguments in this test.
I have no additional comments. Please go ahead.
Patchset #18 (id:380001) has been deleted
The CQ bit was checked by philipj@opera.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/1085843002/#ps400001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1085843002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/69454) (exceeded global retry quota)
The bots are failing because these tests need to be updated: webexposed/global-interface-listing.html webexposed/element-instance-property-listing.html win_blink_rel also so a failure of the new test: fast/dom/ChildNode/before.html That's worrying, please look into the cause.
Thanks Philip for looking into this. I looked into the cause of failure of new tests on win_blink_rel and I identified that, when we are calling parent->insertBefore() in before() and insertBefore() in prepend() method, it passes wrong value of firstChild() beacuse convertNodesIntoNode is executing after this method(Probably on Win only). And this results in wrong value when we are taking child itself as in nodes list. So, for this one I just followed the steps as in specs. Please have a look.
LGTM, C++ is hard: http://stackoverflow.com/questions/621542/compilers-and-argument-order-of-eva... https://codereview.chromium.org/1085843002/diff/480001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1085843002/diff/480001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:548: appendChild(convertNodesIntoNode(nodes, document()), exceptionState); Can you make the same change here for consistency? https://codereview.chromium.org/1085843002/diff/480001/Source/core/dom/Node.c... Source/core/dom/Node.cpp:567: parent->insertBefore(convertNodesIntoNode(nodes, document()), viableNextSibling, exceptionState); And here.
The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/1085843002/#ps500001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1085843002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/07/09 14:48:45, philipj wrote: > LGTM, C++ is hard: > http://stackoverflow.com/questions/621542/compilers-and-argument-order-of-eva... > > https://codereview.chromium.org/1085843002/diff/480001/Source/core/dom/Node.cpp > File Source/core/dom/Node.cpp (right): > > https://codereview.chromium.org/1085843002/diff/480001/Source/core/dom/Node.c... > Source/core/dom/Node.cpp:548: appendChild(convertNodesIntoNode(nodes, > document()), exceptionState); > Can you make the same change here for consistency? > > https://codereview.chromium.org/1085843002/diff/480001/Source/core/dom/Node.c... > Source/core/dom/Node.cpp:567: parent->insertBefore(convertNodesIntoNode(nodes, > document()), viableNextSibling, exceptionState); > And here. Thanks Philip Now committing this.
The CQ bit was checked by paritosh.in@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1085843002/500001
Message was sent while issue was closed.
Committed patchset #23 (id:500001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198629
Message was sent while issue was closed.
Yay! If you want to continue to drive this feature to shipping, I think the next step would be to somehow advertise that these methods are now available behind the experimental flag and see what web developers think. After a few months or so, send an Intent to Ship.
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:500001) has been created in https://codereview.chromium.org/1234813003/ by dominicc@chromium.org. The reason for reverting is: I suspect that this caused Issue 509461.. |
