Bug 57689 – Extract scoping functionality from Document

• Component: DOM
• Status: RESOLVED FIXED

[Roland Steiner]

For the new shadow DOM we will need to have separate scopes of hash IDs, etc., for each shadow tree. Therefore it's necessary to extract this functionality from Document into its own class.

[Roland Steiner]

Q: does this include m_numNodeListCaches?

[Dimitri Glazkov (Google)]

Yeah.

[Roland Steiner]

Created attachment 88100 [details]
Patch

[WebKit Review Bot]

Attachment 88100 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/dom/DOMTreeScope.cpp:59:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 88100 [details]
Patch

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.

[WebKit Review Bot]

Attachment 88100 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8329319

[Roland Steiner]

(In reply to comment #5)
> (From update of attachment 88100 [details])
> 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.

Ok, will change.

> > Source/WebCore/dom/DOMTreeScope.cpp:60
> > +void DOMTreeScope::removedLastRef()
> > +{
> >    ...
> > +}
> 
> Why do we need this in tree scope?

The first version of the change in Node will probably have 2 pointers: to document and to the containing scope. Like with Document, a Scope should stay around till the last child has been destroyed, so I figured it's better to move this into TreeScope. Note that the Document will in turn stay around, because it'll be guardRef'd by the nested TreeScopes. 

Once we remove the (superfluous) additional Document pointer from Node, it'll be faster to just guardRef on the containing scope, rather than accessing the Document indirectly.

> > Source/WebCore/dom/DOMTreeScope.cpp:158
> > +Element* DOMTreeScope::findAnchor(const String& name)
> > +{
> > +    if (name.isEmpty())
> > +        return 0;
> > +    if (Element* element = getElementById(name))
> >          ...
> > +    }
> > +    return 0;
> > +}
> 
> Do we need this in the treescope?

Dunno, but it references getElementById, so I thought it'll fit here more naturally.

> > Source/WebCore/dom/Document.h:210
> > +    typedef DOMTreeScope DocumentBaseClass;
> 
> What is this for?

This was meant to make subsequent changes to the class hierarchy easier (just 2 local changes rather than in multiple places), but since this habit doesn't exist in WebKit in other place, I guess I'll better remove this.

> > Source/WebCore/dom/Node.cpp:-817
> > -void Node::setDocumentRecursively(Document* document)
> > -{
> >     ...
> > -}
> > -
> 
> Why did this move? Can we not do this to minimize the diff?

Yes, that was already part of the next patch for Node, and shouldn't be included here.

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

This has to move, since removedLastRef() needs to be public for the guardRef change (and IMHO really should have been public from the start).

[Roland Steiner]

Created attachment 88124 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 88124 [details]
Patch

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

This is actually quite cool.

> Source/WebCore/ChangeLog:55
> +        (WebCore::TreeScope::DOMTreeScope):
> +        (WebCore::TreeScope::removedLastRef):
> +        (WebCore::TreeScope::destroyScope):
> +        (WebCore::TreeScope::getElementById):
> +        (WebCore::TreeScope::addElementById):
> +        (WebCore::TreeScope::removeElementById):
> +        (WebCore::TreeScope::getElementByAccessKey):
> +        (WebCore::TreeScope::invalidateAccessKeyMap):
> +        (WebCore::TreeScope::addImageMap):
> +        (WebCore::TreeScope::removeImageMap):
> +        (WebCore::TreeScope::getImageMap):
> +        (WebCore::TreeScope::findAnchor):

You can remove these lines from the ChangeLog for brevity.

> Source/WebCore/ChangeLog:60
> +        (WebCore::TreeScope::guardRef):
> +        (WebCore::TreeScope::guardDeref):
> +        (WebCore::TreeScope::hasElementWithId):
> +        (WebCore::TreeScope::containsMultipleElementsWithId):

Ditto.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:47224
> +				RelativePath="..\dom\TreeScope.cpp"
> +				>
> +			</File>
> +			<File
> +				RelativePath="..\dom\TreeScope.h"
> +				>
> +			</File>
> +			<File

This is incorrect. The .cpp file should be set to not build (see nearby .cpp in ..\dom\ for an example) and then it should be added to DOMAllInOne.cpp file.

> Source/WebCore/dom/Node.h:368
> +    

Extra spaces here?

> Source/WebCore/dom/TreeScope.h:64
> +    virtual void removedLastRef();
> +    virtual void destroyScope();

Can we make these protected?

[WebKit Review Bot]

http://trac.webkit.org/changeset/82882 might have broken Windows XP Debug (Tests)

[Roland Steiner]

Committed as WK r82882.