[webkit-reviews] review denied: [Bug 52963] Enable O(1) access to root from any node in shadow DOM subtree : [Attachment 88341] new, smaller patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 52963: Enable O(1) access to root from any node in shadow DOM subtree
https://bugs.webkit.org/show_bug.cgi?id=52963

Attachment 88341: new, smaller patch
https://bugs.webkit.org/attachment.cgi?id=88341&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88341&action=review

Can you remind me why we need to have this guardRef logic on the tree scopes?
Isn't having this done on Document enough? The code looks very awkward with
these refs/derefs and I am not sure we need it.

> Source/WebCore/ChangeLog:9
> +	   Document pointer) to allow direct access to the containing tree
scope.

I am not convinced we have a solution for O(1) document pointer access at the
moment. Perhaps the document ptr will be on the TreeScope? If so, it may make
sense to consider including it into this change. Growing a pointer on each node
is quite expensive, and this could be shipping code.

> Source/WebCore/ChangeLog:39
> +	   * dom/ContainerNode.cpp:
> +	   (WebCore::ContainerNode::insertBefore):
> +	   (WebCore::ContainerNode::replaceChild):
> +	   (WebCore::ContainerNode::appendChild):
> +	   * dom/Document.cpp:
> +	   (WebCore::Document::Document):
> +	   (WebCore::Document::setDocType):
> +	   * dom/Document.h:
> +	   (WebCore::Node::Node):
> +	   * dom/Node.cpp:
> +	   (WebCore::Node::setTreeScope):
> +	   (WebCore::Node::setTreeScopeRecursively):
> +	   * dom/Node.h:
> +	   (WebCore::Node::treeScope):
> +	   * dom/TreeScope.cpp:
> +	   (WebCore::TreeScope::TreeScope):
> +	   (WebCore::TreeScope::~TreeScope):
> +	   (WebCore::TreeScope::removedLastRef):
> +	   (WebCore::TreeScope::setParentTreeScope):
> +	   * dom/TreeScope.h:
> +	   (WebCore::TreeScope::parentTreeScope):

For a change as fundamental as this one, I would definitely provide detailed
explanation of each modification.

> Source/WebCore/dom/Document.h:1389
> +// === Node inline methods ===
> +

This is unnecessary.

> Source/WebCore/dom/Document.h:1390
> +// here because these methods require the Document definition, but we really
want to inline them.

Please make it into a sentence.

> Source/WebCore/dom/Node.cpp:482
> +    newTreeScope->guardRef();
> +    m_treeScope = newTreeScope;
> +    setDocument(newTreeScope->document());
> +    if (oldTreeScope)
> +	   oldTreeScope->guardDeref();

This ref/deref code looks an awful lot like a typical RefPtr assignment. Having
these manual refs/derefs reeks of maintenance pain further down the road.
Usually, we try to actively get rid of them. Can we somehow avoid them here?

> Source/WebCore/dom/Node.cpp:507
> +	       // FIXME: adapt to ShadowFragment
> +	       // ...->setParentTreeScope(newTreeScope);

This comment doesn't make sense yet. So let's yank it.

> Source/WebCore/dom/Node.cpp:509
> +	       // But: need to traverse shadow tree if document has changed.

This comment is unnecessary.

> Source/WebCore/dom/Node.h:696
> +    TreeScope* m_treeScope;

We must measure weight impact of this before thinking of landing. This is why I
was suggesting using RareNodeData.

> Source/WebCore/dom/TreeScope.cpp:113
> +    TreeScope* oldParentScope = m_parentTreeScope;
> +    newParentScope->guardRef();
> +    m_parentTreeScope = newParentScope;
> +    oldParentScope->guardDeref();

This is a classic RefPtr assignment, just with different names. I loathe
landing code like this.