Bug 74648 - DOM: prepend, append, before, after & replace
Summary: DOM: prepend, append, before, after & replace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL: https://dom.spec.whatwg.org
Keywords: InRadar, WebExposed
: 97234 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-12-15 14:04 PST by Erik Arvidsson
Modified: 2015-07-14 09:32 PDT (History)
23 users (show)

See Also:


Attachments
WIP (15.07 KB, patch)
2012-09-28 11:47 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (15.69 KB, patch)
2012-11-06 14:37 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
WIP. Fully working V8 variadic union type (44.09 KB, patch)
2012-11-08 06:35 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
WIP patch (106.75 KB, patch)
2015-07-05 20:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (111.19 KB, patch)
2015-07-11 07:26 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2011-12-15 14:04:48 PST
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?
Comment 1 Ojan Vafai 2011-12-15 14:23:11 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.
Comment 2 Cameron McCormack (:heycam) 2011-12-15 16:02:07 PST
Just to point out that union types haven't been defined in Web IDL yet (Anne is using them speculatively).
Comment 3 Ojan Vafai 2012-09-20 10:45:46 PDT
*** Bug 97234 has been marked as a duplicate of this bug. ***
Comment 5 Erik Arvidsson 2012-09-28 11:47:49 PDT
Created attachment 166287 [details]
WIP
Comment 6 Erik Arvidsson 2012-09-28 11:49:05 PDT
(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)
Comment 7 Emil A Eklund 2012-09-28 11:50:54 PDT
Exciting times!

Wouldn't it make more sense to add the append and prepend methods to ContainerNode instead of Node though?
Comment 8 Erik Arvidsson 2012-09-28 12:07:06 PDT
(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
Comment 9 Erik Arvidsson 2012-11-06 14:37:50 PST
Created attachment 172653 [details]
Patch
Comment 10 Erik Arvidsson 2012-11-06 14:41:06 PST
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 11 Early Warning System Bot 2012-11-06 14:51:52 PST
Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14755244
Comment 12 Early Warning System Bot 2012-11-06 14:54:20 PST
Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14745473
Comment 13 Build Bot 2012-11-06 15:34:32 PST
Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14744539
Comment 14 Build Bot 2012-11-06 16:24:48 PST
Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14733881
Comment 15 EFL EWS Bot 2012-11-06 17:02:57 PST
Comment on attachment 172653 [details]
Patch

Attachment 172653 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14744576
Comment 16 Erik Arvidsson 2012-11-08 06:35:42 PST
Created attachment 173033 [details]
WIP. Fully working V8 variadic union type
Comment 17 Sam Weinig 2015-07-05 20:18:07 PDT
Created attachment 256188 [details]
WIP patch
Comment 18 Sam Weinig 2015-07-05 20:19:35 PDT
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.
Comment 19 yisibl 2015-07-10 00:51:12 PDT
Blink has supported: https://code.google.com/p/chromium/issues/detail?id=255482#c11
Comment 20 Sam Weinig 2015-07-11 07:26:52 PDT
Created attachment 256654 [details]
Patch
Comment 21 Sam Weinig 2015-07-11 07:28:25 PDT
(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".
Comment 22 Sam Weinig 2015-07-11 07:30:10 PDT
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 23 Simon Fraser (smfr) 2015-07-11 13:52:03 PDT
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 24 Darin Adler 2015-07-11 15:46:34 PDT
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?
Comment 25 Sam Weinig 2015-07-11 17:53:15 PDT
(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.
Comment 26 Sam Weinig 2015-07-14 08:59:45 PDT
Committed revision 186803.
Comment 27 Radar WebKit Bug Importer 2015-07-14 09:32:31 PDT
<rdar://problem/21814964>