[webkit-reviews] review denied: [Bug 57689] Extract scoping functionality from Document : [Attachment 88100] 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 57689: Extract scoping functionality from Document
https://bugs.webkit.org/show_bug.cgi?id=57689

Attachment 88100: Patch
https://bugs.webkit.org/attachment.cgi?id=88100&action=review

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

I think it should be called TreeScope. DOM is already plenty qualified based on
the directory of the file. dom/DOMTreeScope seems a bit repetitive. Comments
below.

> Source/WebCore/dom/DOMTreeScope.cpp:60
> +void DOMTreeScope::removedLastRef()
> +{
> +    ASSERT(!m_deletionHasBegun);
> +    if (m_guardRefCount) {
> +	   // If removing a child removes the last self-only ref, we don't
> +	   // want the scope to be destructed until after
> +	   // removeAllChildren returns, so we guard ourselves with an
> +	   // extra self-only ref.
> +	   guardRef();
> +	   destroyScope();
> +	   guardDeref();
> +    } else {
> +	   ContainerNode::removedLastRef();
> +    }
> +}

Why do we need this in tree scope?

> Source/WebCore/dom/DOMTreeScope.cpp:158
> +Element* DOMTreeScope::findAnchor(const String& name)
> +{
> +    if (name.isEmpty())
> +	   return 0;
> +    if (Element* element = getElementById(name))
> +	   return element;
> +    for (Node* node = this; node; node = node->traverseNextNode()) {
> +	   if (node->hasTagName(aTag)) {
> +	       HTMLAnchorElement* anchor =
static_cast<HTMLAnchorElement*>(node);
> +	       if (document()->inQuirksMode()) {
> +		   // Quirks mode, case insensitive comparison of names.
> +		   if (equalIgnoringCase(anchor->name(), name))
> +		       return anchor;
> +	       } else {
> +		   // Strict mode, names need to match exactly.
> +		   if (anchor->name() == name)
> +		       return anchor;
> +	       }
> +	   }
> +    }
> +    return 0;
> +}

Do we need this in the treescope?

> Source/WebCore/dom/Document.h:210
> +    typedef DOMTreeScope DocumentBaseClass;

What is this for?

> Source/WebCore/dom/Document.h:1406
> +	   m_document->guardRef();

I like guardRef as a name.

> Source/WebCore/dom/Node.cpp:-817
> -void Node::setDocumentRecursively(Document* document)
> -{
> -    if (this->document() == document)
> -	   return;
> -
> -    // If an element is moved from a document and then eventually back again
the collection cache for
> -    // that element may contain stale data as changes made to it will have
updated the DOMTreeVersion
> -    // of the document it was moved to. By increasing the DOMTreeVersion of
the donating document here
> -    // we ensure that the collection cache will be invalidated as needed
when the element is moved back.
> -    if (this->document())
> -	   this->document()->incDOMTreeVersion();
> -
> -    for (Node* node = this; node; node = node->traverseNextNode(this)) {
> -	   node->setDocument(document);
> -	   if (!node->isElementNode())
> -	       continue;
> -	   if (Node* shadow = toElement(node)->shadowRoot())
> -	       shadow->setDocumentRecursively(document);
> -    }
> -}
> -

Why did this move? Can we not do this to minimize the diff?

> Source/WebCore/platform/TreeShared.h:-124
> -    virtual void removedLastRef()
> -    {
> -#ifndef NDEBUG
> -	   m_deletionHasBegun = true;
> -#endif
> -	   delete this;
> -    }
> -

Ditto.