Bug 74648 – DOM: prepend, append, before, after & replace

• Component: DOM
• Status: RESOLVED FIXED

[Erik Arvidsson]

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.

Does this seem reasonable?

Is it worth adding support for this in the code generators at this point?

[Ojan Vafai]

(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.

[Cameron McCormack (:heycam)]

Just to point out that union types haven't been defined in Web IDL yet (Anne is using them speculatively).

[Ojan Vafai]

*** Bug 97234 has been marked as a duplicate of this bug. ***

[Ojan Vafai]

FYI: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-methods

[Erik Arvidsson]

Created attachment 166287 [details]
WIP

[Erik Arvidsson]

(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)

[Emil A Eklund]

Exciting times!

Wouldn't it make more sense to add the append and prepend methods to ContainerNode instead of Node though?

[Erik Arvidsson]

(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

[Erik Arvidsson]

Created attachment 172653 [details]
Patch

[Erik Arvidsson]

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.

[Early Warning System Bot]

Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14755244

[Early Warning System Bot]

Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14745473

[Build Bot]

Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14744539

[Build Bot]

Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14733881

[EFL EWS Bot]

Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14744576

[Erik Arvidsson]

Created attachment 173033 [details]
WIP. Fully working V8 variadic union type

[Sam Weinig]

Created attachment 256188 [details]
WIP patch

[Sam Weinig]

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.

[yisibl]

Blink has supported: https://code.google.com/p/chromium/issues/detail?id=255482#c11

[Sam Weinig]

Created attachment 256654 [details]
Patch

[Sam Weinig]

(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".

[Sam Weinig]

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.

[Simon Fraser (smfr)]

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?

[Darin Adler]

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?

[Sam Weinig]

(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.

[Sam Weinig]

Committed revision 186803.

[Radar WebKit Bug Importer]

<rdar://problem/21814964>