Bug 47702 – Use more specific types for node pointers, especially when calling node insertion and removal functions

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Use more specific types for node pointers, especially when calling node insertion and removal functions

[Darin Adler]

I am preparing to make functions such as insertBefore non-virtual and make sure almost all callers call the versions in ContainerNode. We’ll still need a way to call these from DOM bindings on an arbitrary node.

To get started on this, I commented out the functions in Node, and changed almost every caller so they had a specific enough type that they could call directly to ContainerNode.

[Darin Adler]

Created attachment 70812 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 70812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70812&action=review

Looks great, r=me.

> WebCore/dom/Element.cpp:125
> -            Node* firstChild = node->firstChild();
> +            HTMLElement* element = static_cast<HTMLElement*>(node.get());

It would be nice to keep the name more specific.

> WebCore/dom/Range.cpp:670
> -                    m_start.container()->removeChild(n, ec);
> +                    static_cast<ContainerNode*>(m_start.container())->removeChild(n, ec);

We use "toXXX" functions in some code areas to ASSERT at runtime. But perhaps this is obvious enough as is.

> WebCore/dom/Text.cpp:156
>      RefPtr<Text> protectedThis(this); // Mutation event handlers could cause our last ref to go away
> -    Node* parent = parentNode(); // Protect against mutation handlers moving this node during traversal
> +    ContainerNode* parent = parentNode(); // Protect against mutation handlers moving this node during traversal

This code looks very suspicious. It seems that nothing prevents parent from being destroyed by mutation event handlers.

> WebCore/dom/TreeWalker.cpp:100
>              if (!parent || parent == root() || parent == m_current)
>                  return 0;
>              node = parent;

I'm not sure that this is an improvement. The pointer is never going to be used as a ContainerNode here, so seeing the more specific type makes one wonder a little. Another visual inconvenience is that parentNode() doesn't have "Container" in its name.

There are of course more places like this in this patch. Perhaps this is so minor that it's not worth going over the patch again or even debating.

I wish we could match DOM in not having a ContainerNode class, but that's probably impractical because it would make Text objects larger.

[Darin Adler]

Thanks for the review.

(In reply to comment #3)
> > WebCore/dom/Element.cpp:125
> > -            Node* firstChild = node->firstChild();
> > +            HTMLElement* element = static_cast<HTMLElement*>(node.get());
> 
> It would be nice to keep the name more specific.

This is a misunderstanding. There is still a firstChild variable, I’m not renaming that. I’m just adding an “element”

> > WebCore/dom/Range.cpp:670
> > -                    m_start.container()->removeChild(n, ec);
> > +                    static_cast<ContainerNode*>(m_start.container())->removeChild(n, ec);
> 
> We use "toXXX" functions in some code areas to ASSERT at runtime. But perhaps this is obvious enough as is.

Good idea. I’ll add toContainerNode.

> > WebCore/dom/Text.cpp:156
> >      RefPtr<Text> protectedThis(this); // Mutation event handlers could cause our last ref to go away
> > -    Node* parent = parentNode(); // Protect against mutation handlers moving this node during traversal
> > +    ContainerNode* parent = parentNode(); // Protect against mutation handlers moving this node during traversal
> 
> This code looks very suspicious. It seems that nothing prevents parent from being destroyed by mutation event handlers.

You may be right. I’ll take a second look after landing this.

> > WebCore/dom/TreeWalker.cpp:100
> >              if (!parent || parent == root() || parent == m_current)
> >                  return 0;
> >              node = parent;
> 
> I'm not sure that this is an improvement. The pointer is never going to be used as a ContainerNode here, so seeing the more specific type makes one wonder a little. Another visual inconvenience is that parentNode() doesn't have "Container" in its name.
> 
> There are of course more places like this in this patch. Perhaps this is so minor that it's not worth going over the patch again or even debating.

I’d like these local variables to have the more specific type even if there is no immediate concrete benefit.

I want these local variables to get their type from the result of the parentNode function. If we were requiring newer C++ compilers we could do it with the auto keyword instead of specifying a type directly. I agree that doesn’t seem important to make the type be ContainerNode, but on the other hand it doesn’t seem helpful to broaden the type from ContainerNode to Node for no particular reason either.

[Darin Adler]

Committed r69868: <http://trac.webkit.org/changeset/69868>

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.