Morph the APIs for Node, ParentNode, and Element closer to the specs

Morph the APIs for Node, ParentNode, and Element closer to the specs

These still don't match the specs exactly, but they're much closer.

R=ojan@chromium.org, eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/275a60b6fc1ed5774dade875484e59683038d5f6

[esprehn, 2015-02-13 23:53:47]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-02-13 23:53:48]

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element.h
File sky/engine/core/dom/Element.h (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element....
sky/engine/core/dom/Element.h:79: void setAttribute(const AtomicString& name,
ExceptionState& es) {
default this to null in the idl instead?

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element.idl
File sky/engine/core/dom/Element.idl (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element....
sky/engine/core/dom/Element.idl:18: readonly attribute CSSStyleDeclaration
style;
Only elements have style, why move this to node?

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:415: refNode = node.release();
This doesn't look right, why are you reffing and then dropping each new node as
you go? You need to ref yourself, not each child as you insert them.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:430: parent->append(nodes, es);
You don't need this branch, we can just call insertBefore with null for the next
sibling.

[abarth-chromium, 2015-02-13 23:59:47]

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element.h
File sky/engine/core/dom/Element.h (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element....
sky/engine/core/dom/Element.h:79: void setAttribute(const AtomicString& name,
ExceptionState& es) {
On 2015/02/13 at 23:53:48, esprehn wrote:
> default this to null in the idl instead?

I don't think this function is ever called but the code generator is dumb and
tries to link against this symbol.  I'm happy to fix the code generator in a
future CL.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element.idl
File sky/engine/core/dom/Element.idl (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Element....
sky/engine/core/dom/Element.idl:18: readonly attribute CSSStyleDeclaration
style;
On 2015/02/13 at 23:53:48, esprehn wrote:
> Only elements have style, why move this to node?

That's what Ian's spec says.  Maybe we should change the spec to not say that?

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:415: refNode = node.release();
On 2015/02/13 at 23:53:48, esprehn wrote:
> This doesn't look right, why are you reffing and then dropping each new node
as you go? You need to ref yourself, not each child as you insert them.

The question is how should this code behave if there are tree mutations inside
this loop.  There are a couple of different semantics you could choose.  This
one always inserts the next element in the list after the previous one, even if
you move the previous one to some other crazy place.  Another approach would be
to walk the list backwards and always insert after |this| even if |this| moved
somewhere else, but we thought it was more sensible to insert the elements in
the order given.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:430: parent->append(nodes, es);
On 2015/02/13 at 23:53:48, esprehn wrote:
> You don't need this branch, we can just call insertBefore with null for the
next sibling.

I don't follow.  Which function would you like me to call if |m_next| is null?

[eseidel, 2015-02-14 00:12:23]

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Containe...
File sky/engine/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Containe...
sky/engine/core/dom/ContainerNode.cpp:887: return result;
This copies the vector unless c++ is smarter now...

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Containe...
File sky/engine/core/dom/ContainerNode.h (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Containe...
sky/engine/core/dom/ContainerNode.h:54: Element* firstElementChild() const;
So now ContainerNode knows about its subclass Element? :/

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Containe...
sky/engine/core/dom/ContainerNode.h:57: Vector<RefPtr<Node>> getChildNodes()
const;
Are you sure you want to copy the vector? :)

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Containe...
sky/engine/core/dom/ContainerNode.h:61: void append(Vector<RefPtr<Node>>& nodes,
ExceptionState&);
Another way to do this would be to use a PassOwnPtr<Vector>.  We could also use
an ImplmentedAs=takeFromAndAppend or similar if we wanted to make the c++ naming
more clear.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:408: for (auto& node : nodes) {
Living the dream.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.idl
File sky/engine/core/dom/Node.idl (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.idl...
sky/engine/core/dom/Node.idl:6: // TODO(abarth): This should actually be a named
argument.
Can't it be today?  [Named] should work?

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/editing/Remo...
File sky/engine/core/editing/RemoveNodePreservingChildrenCommand.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/editing/Remo...
sky/engine/core/editing/RemoveNodePreservingChildrenCommand.cpp:46:
appendChildNodes(toContainerNode(*m_node), children);
I think this is going to be very confusing to read.  I think we're going to need
some sort of takeNodesAndAppend or similar.

https://codereview.chromium.org/924203002/diff/1/sky/tests/dom/document-child...
File sky/tests/dom/document-child-mutations.sky (right):

https://codereview.chromium.org/924203002/diff/1/sky/tests/dom/document-child...
sky/tests/dom/document-child-mutations.sky:25: oldChild.replaceWith([newChild]);
Feels like we're going to want a single vs. list version of these.  Presumably
ian has one in teh works?

replaceWithNode(node)
replaceWith(list)?

[abarth-chromium, 2015-02-14 00:27:19]

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.idl
File sky/engine/core/dom/Node.idl (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.idl...
sky/engine/core/dom/Node.idl:6: // TODO(abarth): This should actually be a named
argument.
On 2015/02/14 at 00:12:23, eseidel wrote:
> Can't it be today?  [Named] should work?

I wrote it before you landed your change.  :)

Can I fix it in the next CL?  I'll probably have to update some more tests.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/editing/Remo...
File sky/engine/core/editing/RemoveNodePreservingChildrenCommand.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/editing/Remo...
sky/engine/core/editing/RemoveNodePreservingChildrenCommand.cpp:46:
appendChildNodes(toContainerNode(*m_node), children);
On 2015/02/14 at 00:12:23, eseidel wrote:
> I think this is going to be very confusing to read.  I think we're going to
need some sort of takeNodesAndAppend or similar.

I can change this back to getChildNodes.  The problem was that there's now a
getChildNodes function in Node that's part of the API.  This does almost the
same thing, except NodeVector has some inline capacity.

https://codereview.chromium.org/924203002/diff/1/sky/tests/dom/document-child...
File sky/tests/dom/document-child-mutations.sky (right):

https://codereview.chromium.org/924203002/diff/1/sky/tests/dom/document-child...
sky/tests/dom/document-child-mutations.sky:25: oldChild.replaceWith([newChild]);
On 2015/02/14 at 00:12:23, eseidel wrote:
> Feels like we're going to want a single vs. list version of these.  Presumably
ian has one in teh works?
> 
> replaceWithNode(node)
> replaceWith(list)?

There's some discussion about this in the Sky chatroom.  There's a question of
how much sugar the API versus how much it should just give you the metal.  Maybe
file a bug against the spec to discuss?

[esprehn, 2015-02-14 01:09:25]

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:415: refNode = node.release();
On 2015/02/13 at 23:59:45, abarth wrote:
> On 2015/02/13 at 23:53:48, esprehn wrote:
> > This doesn't look right, why are you reffing and then dropping each new node
as you go? You need to ref yourself, not each child as you insert them.
> 
> The question is how should this code behave if there are tree mutations inside
this loop.  There are a couple of different semantics you could choose.  This
one always inserts the next element in the list after the previous one, even if
you move the previous one to some other crazy place.  Another approach would be
to walk the list backwards and always insert after |this| even if |this| moved
somewhere else, but we thought it was more sensible to insert the elements in
the order given.

There can't be mutations inside this loop.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:430: parent->append(nodes, es);
On 2015/02/13 at 23:59:45, abarth wrote:
> On 2015/02/13 at 23:53:48, esprehn wrote:
> > You don't need this branch, we can just call insertBefore with null for the
next sibling.
> 
> I don't follow.  Which function would you like me to call if |m_next| is null?

Just always call

RefPtr<Node> next = m_next;
remove();
parent()->insertBefore(this, m_next);

[abarth-chromium, 2015-02-14 01:28:27]

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:415: refNode = node.release();
On 2015/02/14 at 01:09:25, esprehn wrote:
> There can't be mutations inside this loop.

Ok, I'll change this to ASSERT that.

https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
sky/engine/core/dom/Node.cpp:430: parent->append(nodes, es);
On 2015/02/14 at 01:09:24, esprehn wrote:
> Just always call
> 
> RefPtr<Node> next = m_next;
> remove();
> parent()->insertBefore(this, m_next);

That doesn't work because |this| isn't a child of |parent| anymore because
|this| is removed on the previous line.

[abarth-chromium, 2015-02-14 01:34:37]

PTAL

[esprehn, 2015-02-14 01:37:12]

On 2015/02/14 at 01:28:27, abarth wrote:
> https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp
> File sky/engine/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
> sky/engine/core/dom/Node.cpp:415: refNode = node.release();
> On 2015/02/14 at 01:09:25, esprehn wrote:
> > There can't be mutations inside this loop.
> 
> Ok, I'll change this to ASSERT that.
> 
>
https://codereview.chromium.org/924203002/diff/1/sky/engine/core/dom/Node.cpp...
> sky/engine/core/dom/Node.cpp:430: parent->append(nodes, es);
> On 2015/02/14 at 01:09:24, esprehn wrote:
> > Just always call
> > 
> > RefPtr<Node> next = m_next;
> > remove();
> > parent()->insertBefore(this, m_next);
> 
> That doesn't work because |this| isn't a child of |parent| anymore because
|this| is removed on the previous line.

So save the parent first?

[esprehn, 2015-02-14 01:50:08]

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
sky/engine/core/dom/Node.cpp:411: ContainerNode* parent = refNode->parentNode();
You don't need to keep checking parent node like this, script can't run in here.

RefPtr<Node> reference = m_next;
for (auto& node : nodes) {
  parent()->insertBefore(node, reference);
}

I think is enough?

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
sky/engine/core/dom/Node.cpp:434: }
RefPtr<Node> parent = this->parent();
RefPtr<Node> next = m_next;
if (!parent)
  return;
remove();
parent->insertBefore(this, m_next);

should be the same as this function. replaceWith makes no sense if there's no
parent, you can't have siblings if there's no parent.

https://codereview.chromium.org/924203002/diff/20001/sky/tests/lowlevel/style...
File sky/tests/lowlevel/style-basic.sky (right):

https://codereview.chromium.org/924203002/diff/20001/sky/tests/lowlevel/style...
sky/tests/lowlevel/style-basic.sky:37: + " id=" +
element.getAttribute("id").toString()
Why toString() ?

[esprehn, 2015-02-14 01:51:06]

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
sky/engine/core/dom/Node.cpp:434: }
On 2015/02/14 at 01:50:08, esprehn wrote:
> RefPtr<Node> parent = this->parent();
> RefPtr<Node> next = m_next;
> if (!parent)
>   return;
> remove();
> parent->insertBefore(this, m_next);
> 

Sorry that's parent->insertBefore(this, next);

[abarth-chromium, 2015-02-14 02:30:53]

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
sky/engine/core/dom/Node.cpp:434: }
On 2015/02/14 at 01:51:06, esprehn wrote:
> On 2015/02/14 at 01:50:08, esprehn wrote:
> > RefPtr<Node> parent = this->parent();
> > RefPtr<Node> next = m_next;
> > if (!parent)
> >   return;
> > remove();
> > parent->insertBefore(this, m_next);
> > 
> 
> Sorry that's parent->insertBefore(this, next);

We're trying to insert |nodes|, not |this|.  Also, |nodes| is a Vector, not a
single Node.  If I don't want to write a loop here, my choices are:

1) Call append or prepend on the parent.
2) Call insertBefore or insertAfter on a sibling.

The branch is necessary because |this| might be the only child of |parent| (in
which case I need to call append or prepend) or because |this| might be in the
middle of |parent| child list (which which case I need to call insertBefore or
insertAfter).

https://codereview.chromium.org/924203002/diff/20001/sky/tests/lowlevel/style...
File sky/tests/lowlevel/style-basic.sky (right):

https://codereview.chromium.org/924203002/diff/20001/sky/tests/lowlevel/style...
sky/tests/lowlevel/style-basic.sky:37: + " id=" +
element.getAttribute("id").toString()
On 2015/02/14 at 01:50:08, esprehn wrote:
> Why toString() ?

Because getAttribute returns null when the attribute isn't there.

[esprehn, 2015-02-14 02:37:43]

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
File sky/engine/core/dom/Node.cpp (right):

https://codereview.chromium.org/924203002/diff/20001/sky/engine/core/dom/Node...
sky/engine/core/dom/Node.cpp:434: }
On 2015/02/14 at 02:30:53, abarth wrote:
> ...
> The branch is necessary because |this| might be the only child of |parent| (in
which case I need to call append or prepend) or because |this| might be in the
middle of |parent| child list (which which case I need to call insertBefore or
insertAfter).

Why not a loop then?

RefPtr<Node> parent = this->parent();
RefPtr<Node> next = m_next;
remove();
for (auto& node : nodes) {
  parent->insertBefore(node, next);
}

that replaces us with all those nodes right?

[abarth-chromium, 2015-02-14 04:22:32]

PTAL

[ojan, 2015-02-14 05:11:22]

ojan@chromium.org changed reviewers:
+ ojan@chromium.org

[ojan, 2015-02-14 05:11:23]

lgtm

[abarth-chromium, 2015-02-14 05:40:31]

Committed patchset #5 (id:80001) manually as
275a60b6fc1ed5774dade875484e59683038d5f6 (presubmit successful).