| Summary: | DOM: prepend, append, before, after & replace | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||||||||
| Component: | DOM | Assignee: | Sam Weinig <sam> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | 50167214, abarth, adamk, bjonesbe, bronislav.klucka, cmarcelo, eae, ericbidelman, esprehn, gustavo, haraken, heycam, japhet, jsbell, mathias, ojan, philn, sam, skyul, syoichi, webkit-bug-importer, webkit.review.bot, xan.lopez | ||||||||||||
| Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| URL: | https://dom.spec.whatwg.org | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Erik Arvidsson
2011-12-15 14:04:48 PST
(In reply to comment #0) > DOM4 has these methods which all takes var args of union types > > // NEW (using experimental IDL) > void prepend(Node | DOMString... nodes); > void append(Node | DOMString... nodes); > void before(Node | DOMString... nodes); > void after(Node | DOMString... nodes); > void replace(Node | DOMString... nodes); > > The way I was envisioning implementing these was to add methods to Node.h that takes a WTF::Vector<WebCore::NodeOrString>. The class NodeOrString would have isNode/toNode and isString/toString methods and there would be one constructor per type. The binding code would check the type and call the right constructor. I would have isNode/node and isString/string. > Does this seem reasonable? Yes. > Is it worth adding support for this in the code generators at this point? Yes. Just to point out that union types haven't been defined in Web IDL yet (Anne is using them speculatively). *** Bug 97234 has been marked as a duplicate of this bug. *** Created attachment 166287 [details]
WIP
(In reply to comment #5) > Created an attachment (id=166287) [details] > WIP This is a work in progress. It currently only does append(Node... nodes) Next step is to see what it takes to do append((Node or String) nodes) Exciting times! Wouldn't it make more sense to add the append and prepend methods to ContainerNode instead of Node though? (In reply to comment #7) > Wouldn't it make more sense to add the append and prepend methods to ContainerNode instead of Node though? Yes. That would be better Created attachment 172653 [details]
Patch
Comment on attachment 172653 [details]
Patch
This is another WIP patch. This time it does union types
prepend((Node or DOMString) value)
Next step is to combine these two patches to do
prepend((Node or DOMString)... values)
Feedback is wanted on the approach.
This is V8 only at this point.
Comment on attachment 172653 [details] Patch Attachment 172653 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14755244 Comment on attachment 172653 [details] Patch Attachment 172653 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14745473 Comment on attachment 172653 [details] Patch Attachment 172653 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14744539 Comment on attachment 172653 [details] Patch Attachment 172653 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14733881 Comment on attachment 172653 [details] Patch Attachment 172653 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14744576 Created attachment 173033 [details]
WIP. Fully working V8 variadic union type
Created attachment 256188 [details]
WIP patch
Comment on attachment 256188 [details]
WIP patch
WIP patch. There are still some questions I have for the spec editor about replaceWith. Putting this up here to EWS test.
Blink has supported: https://code.google.com/p/chromium/issues/detail?id=255482#c11 Created attachment 256654 [details]
Patch
(In reply to comment #18) > Comment on attachment 256188 [details] > WIP patch > > WIP patch. There are still some questions I have for the spec editor about > replaceWith. Putting this up here to EWS test. I got my question, which was how can parentNode change between steps 1 and 5 min replaceWith, answered. Answer being, "If the context object is one of the nodes passed in, that will no longer be true due to it being inserted into the DocumentFragment". I did not add support for general variadic unions in IDL as it does not seem like something that is used too often, and the custom code was easy enough to write. If there are more uses of variadic unions (or unions at all really), we should add better support to the generator and remove the custom code. Comment on attachment 256654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256654&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Tests: fast/dom/ChildNode-after.html What are you doing? Adding new features? Removing things? Fixing things? Comment on attachment 256654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256654&action=review > Source/WebCore/bindings/js/JSCharacterDataCustom.cpp:36 > +JSValue JSCharacterData::before(ExecState* exec) I’ve been using the word “state” instead of ”exec” in new code. Maybe you would consider doing the same? > Source/WebCore/bindings/js/JSCharacterDataCustom.cpp:38 > + auto nodes = toNodeOrStringVector(exec); Seems like something we could teach the code generator to do pretty easily. We just need to come up with some magic type name and/or IDL keyword that means this. > Source/WebCore/bindings/js/JSCharacterDataCustom.cpp:41 > + impl().before(WTF::move(nodes), ec); If the call to toNodeOrStringVector(exec) was in the expression here, we wouldn’t need the WTF::move. I think that would be better. > Source/WebCore/bindings/js/JSNodeOrString.cpp:38 > + Vector<NodeOrString> nodeOrStringVector; Should use a shorter name. No need to reiterate the type. Maybe vector or result. > Source/WebCore/bindings/js/JSNodeOrString.cpp:40 > + size_t argumentCount = exec->argumentCount(); Needs: nodeOrStringVector.reserveInitialCapacity(argumentCount); > Source/WebCore/bindings/js/JSNodeOrString.cpp:42 > + JSValue value = exec->argument(i); Should be uncheckedArgument(i) since we already looked at argumentCount. > Source/WebCore/bindings/js/JSNodeOrString.cpp:44 > + nodeOrStringVector.append(NodeOrString(&node->impl())); Could be uncheckedAppend if we do reserveInitialCapacity. I don’t think the typecast to NodeOrString is needed here. If so, I am surprised that it is. > Source/WebCore/bindings/js/JSNodeOrString.cpp:48 > + return Vector<NodeOrString>(); Another way to write that is return { } > Source/WebCore/bindings/js/JSNodeOrString.cpp:49 > + nodeOrStringVector.append(NodeOrString(string)); Could be uncheckedAppend if we do reserveInitialCapacity. I don’t think the typecast to NodeOrString is needed here. If so, I am surprised that it is. > Source/WebCore/bindings/js/JSNodeOrString.h:38 > +Vector<NodeOrString> toNodeOrStringVector(JSC::ExecState*); I suggest ExecState& state instead of ExecState* exec. > Source/WebCore/dom/ContainerNode.cpp:928 > + if (ec || !node) > + return; I suggest omitting the !node check here. Since this code is checking ec, it needs to initialize it to zero. Caller is not required to do that. > Source/WebCore/dom/ContainerNode.cpp:937 > + if (ec || !node) > + return; I suggest omitting the !node check here. Since this code is checking ec, it needs to initialize it to zero. Caller is not required to do that. > Source/WebCore/dom/Node.cpp:494 > +static RefPtr<Node> firstPrecedingSiblingNotInNodeSet(Node& context, HashSet<RefPtr<Node>> nodeSet) Please use const HashSet&; no need to copy the set every time this is called. > Source/WebCore/dom/Node.cpp:503 > +static RefPtr<Node> firstFollowingSiblingNotInNodeSet(Node& context, HashSet<RefPtr<Node>> nodeSet) Ditto. > Source/WebCore/dom/Node.cpp:516 > + if (!parent) > + return; Should we be setting ec here? Do we have test coverage for this? > Source/WebCore/dom/Node.cpp:518 > + HashSet<RefPtr<Node>> nodeSet = nodeSetPreTransformedFromNodeOrStringVector(nodeOrStringVector); auto? > Source/WebCore/dom/Node.cpp:523 > + if (ec || !node) > + return; I suggest omitting the !node check here. Since this code is checking ec, it needs to initialize it to zero. Caller is not required to do that. > Source/WebCore/dom/Node.cpp:537 > + if (!parent) > + return; Should we be setting ec here? Do we have test coverage for this? > Source/WebCore/dom/Node.cpp:539 > + HashSet<RefPtr<Node>> nodeSet = nodeSetPreTransformedFromNodeOrStringVector(nodeOrStringVector); auto? > Source/WebCore/dom/Node.cpp:544 > + if (ec || !node) > + return; I suggest omitting the !node check here. Since this code is checking ec, it needs to initialize it to zero. Caller is not required to do that. > Source/WebCore/dom/Node.cpp:546 > + parent->insertBefore(node, viableNextSibling.get(), ec); node.release() here too > Source/WebCore/dom/Node.cpp:555 > + HashSet<RefPtr<Node>> nodeSet = nodeSetPreTransformedFromNodeOrStringVector(nodeOrStringVector); auto? > Source/WebCore/dom/Node.cpp:565 > + parent->insertBefore(node, viableNextSibling.get(), ec); node.release() here too > Source/WebCore/dom/NodeOrString.cpp:39 > + Vector<RefPtr<Node>> nodes; reserveInitialCapacity(nodeOrStringVector.size()); > Source/WebCore/dom/NodeOrString.cpp:43 > + nodes.append(Text::create(context.document(), nodeOrString.string())); uncheckedAppend > Source/WebCore/dom/NodeOrString.cpp:46 > + nodes.append(nodeOrString.node()); uncheckedAppend > Source/WebCore/dom/NodeOrString.cpp:59 > + if (ec) > + return nullptr; Since we are checking ec here, we need to initialize it to zero. Caller is not required to do that. Or maybe the caller should be required to do that in this case? > Source/WebCore/dom/NodeOrString.h:56 > + NodeOrString(const NodeOrString& other) Consider also writing a move constructor. Please either write the copy and move assignment operators or delete them. > Source/WebCore/dom/NodeOrString.h:86 > + Node* node() const { ASSERT(m_type == Type::Node); return m_data.node; } > + StringImpl* string() const { ASSERT(m_type == Type::String); return m_data.string; } I suppose we support null for both of these, so these can’t return references. > Source/WebCore/dom/NodeOrString.h:96 > +RefPtr<Node> convertNodesOrStringsIntoNode(Node& context, Vector<NodeOrString>&&, ExceptionCode&); Name seems oblique. How can I convert a vector of nodes or strings into a single node? I think the point is that it converts them into a little DOM tree, not always a single node. Maybe it should be named something like makeDOMTree. What would be a colloquial way to refer to it? (In reply to comment #24) > Comment on attachment 256654 [details] > Patch Thanks for the review. It probably wasn't quite ready, I accidentally left on the r? when I put it up for the bots to chew through. I don't even have a changelog filled out yet. > View in context: > https://bugs.webkit.org/attachment.cgi?id=256654&action=review > > > Source/WebCore/bindings/js/JSCharacterDataCustom.cpp:36 > > +JSValue JSCharacterData::before(ExecState* exec) > > I’ve been using the word “state” instead of ”exec” in new code. Maybe you > would consider doing the same? Will change. > > Source/WebCore/bindings/js/JSCharacterDataCustom.cpp:38 > > + auto nodes = toNodeOrStringVector(exec); > > Seems like something we could teach the code generator to do pretty easily. > We just need to come up with some magic type name and/or IDL keyword that > means this. I went down this path and decided against it due to how messy it got for the small cost of the custom code. > > Source/WebCore/bindings/js/JSCharacterDataCustom.cpp:41 > > + impl().before(WTF::move(nodes), ec); > > If the call to toNodeOrStringVector(exec) was in the expression here, we > wouldn’t need the WTF::move. I think that would be better. Will change. > > > Source/WebCore/bindings/js/JSNodeOrString.cpp:38 > > + Vector<NodeOrString> nodeOrStringVector; > > Should use a shorter name. No need to reiterate the type. Maybe vector or > result. > Changed to result. > > Source/WebCore/bindings/js/JSNodeOrString.cpp:40 > > + size_t argumentCount = exec->argumentCount(); > > Needs: > > nodeOrStringVector.reserveInitialCapacity(argumentCount); > Done. > > Source/WebCore/bindings/js/JSNodeOrString.cpp:42 > > + JSValue value = exec->argument(i); > > Should be uncheckedArgument(i) since we already looked at argumentCount. > Done. > > Source/WebCore/bindings/js/JSNodeOrString.cpp:44 > > + nodeOrStringVector.append(NodeOrString(&node->impl())); > > Could be uncheckedAppend if we do reserveInitialCapacity. I don’t think the > typecast to NodeOrString is needed here. If so, I am surprised that it is. > Done. > > Source/WebCore/bindings/js/JSNodeOrString.cpp:48 > > + return Vector<NodeOrString>(); > > Another way to write that is return { } Done. > > > Source/WebCore/bindings/js/JSNodeOrString.cpp:49 > > + nodeOrStringVector.append(NodeOrString(string)); > > Could be uncheckedAppend if we do reserveInitialCapacity. I don’t think the > typecast to NodeOrString is needed here. If so, I am surprised that it is. Done. > > > Source/WebCore/bindings/js/JSNodeOrString.h:38 > > +Vector<NodeOrString> toNodeOrStringVector(JSC::ExecState*); > > I suggest ExecState& state instead of ExecState* exec. Done. > > > Source/WebCore/dom/ContainerNode.cpp:928 > > + if (ec || !node) > > + return; > > I suggest omitting the !node check here. I don't think I can. convertNodesOrStringsIntoNode can return null in a non-exceptional case and we should still return. > > Since this code is checking ec, it needs to initialize it to zero. Caller is > not required to do that. Are you sure? This is a pretty common pattern in the DOM, and I believe the expectation is that the top caller zeros out the ec. > > > Source/WebCore/dom/Node.cpp:494 > > +static RefPtr<Node> firstPrecedingSiblingNotInNodeSet(Node& context, HashSet<RefPtr<Node>> nodeSet) > > Please use const HashSet&; no need to copy the set every time this is called. Done. > > > Source/WebCore/dom/Node.cpp:503 > > +static RefPtr<Node> firstFollowingSiblingNotInNodeSet(Node& context, HashSet<RefPtr<Node>> nodeSet) > > Ditto. Done. > > > Source/WebCore/dom/Node.cpp:516 > > + if (!parent) > > + return; > > Should we be setting ec here? Do we have test coverage for this? No. The spec (https://dom.spec.whatwg.org/#dom-childnode-before) says to terminate the steps cleanly in this case. I believe I do have a test for this. > > > Source/WebCore/dom/Node.cpp:518 > > + HashSet<RefPtr<Node>> nodeSet = nodeSetPreTransformedFromNodeOrStringVector(nodeOrStringVector); > > auto? Done. > > > Source/WebCore/dom/Node.cpp:546 > > + parent->insertBefore(node, viableNextSibling.get(), ec); > > node.release() here too Done. > > Source/WebCore/dom/NodeOrString.cpp:39 > > + Vector<RefPtr<Node>> nodes; > > reserveInitialCapacity(nodeOrStringVector.size()); Done. > > > Source/WebCore/dom/NodeOrString.cpp:43 > > + nodes.append(Text::create(context.document(), nodeOrString.string())); > > uncheckedAppend Done. > > > Source/WebCore/dom/NodeOrString.cpp:46 > > + nodes.append(nodeOrString.node()); > > uncheckedAppend Done. > > Source/WebCore/dom/NodeOrString.h:56 > > + NodeOrString(const NodeOrString& other) > > Consider also writing a move constructor. > > Please either write the copy and move assignment operators or delete them. Will do. > > > Source/WebCore/dom/NodeOrString.h:86 > > + Node* node() const { ASSERT(m_type == Type::Node); return m_data.node; } > > + StringImpl* string() const { ASSERT(m_type == Type::String); return m_data.string; } > > I suppose we support null for both of these, so these can’t return > references. I don't think they can be null. I made them references. > > > Source/WebCore/dom/NodeOrString.h:96 > > +RefPtr<Node> convertNodesOrStringsIntoNode(Node& context, Vector<NodeOrString>&&, ExceptionCode&); > > Name seems oblique. How can I convert a vector of nodes or strings into a > single node? I think the point is that it converts them into a little DOM > tree, not always a single node. Maybe it should be named something like > makeDOMTree. What would be a colloquial way to refer to it? The name comes from the spec - https://dom.spec.whatwg.org/#converting-nodes-into-a-node. I will try to make it clearer. Committed revision 186803. |