Bug 52963 – Enable O(1) access to root from any node in shadow DOM subtree

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Hang an extra ptr off NodeRareData (simple) and ensure that its value is always accurate (tedious)

Later on, we'll probably need to expand this to be a shadow dom context to store rules/settings.

[Dimitri Glazkov (Google)]

(In reply to comment #0)
> Hang an extra ptr off NodeRareData (simple) and ensure that its value is always accurate (tedious)
> 
> Later on, we'll probably need to expand this to be a shadow dom context to store rules/settings.

A ptr to the context, clearly.

[Hajime Morrita]

Hi, It might be a dull question, but what feature requires this?
I'm afraid that it is no longer "rare" data if every shadow nodes have it...
If the use-case allows, some temporal static variables or flags might be enough,
or lazy-allocation might be possible for non-hot codepath.

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Hi, It might be a dull question, but what feature requires this?
> I'm afraid that it is no longer "rare" data if every shadow nodes have it...
> If the use-case allows, some temporal static variables or flags might be enough,
> or lazy-allocation might be possible for non-hot codepath.

For the immediate needs, we need something like this to fix bug 52917. For the future needs, you'd want to be able to jump to shadow root or read style-cascading settings for any node in the shadow subtree.

As for using RareNodeData, you're more than welcome to explore different solutions!

[Dimitri Glazkov (Google)]

One idea is to replace Document* in Node with a ptr to some other class that represent a "tree scope", including ptr to document, list of ids, names, and coincidentally shadow DOM settings. This might be a better path forward, considering that shadow DOM subtree would have to have its own scope of ids/names and other fun data that is normally stored on the Document. WDYT?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> One idea is to replace Document* in Node with a ptr to some other class that represent a "tree scope", including ptr to document, list of ids, names, and coincidentally shadow DOM settings. This might be a better path forward, considering that shadow DOM subtree would have to have its own scope of ids/names and other fun data that is normally stored on the Document. WDYT?

As a side benefit, we can build scoped stylesheets off of this.

[Dimitri Glazkov (Google)]

Adding the usual suspects for the discussion. Darin, Dave, what do you think?

[Darin Adler]

Standard space vs. speed tradeoff. Just need to find a way to measure both.

[Roland Steiner]

Will have a go at the "tree scope" version.

[Hajime Morrita]

(In reply to comment #8)
> Will have a go at the "tree scope" version.

BTW, I found this required for transforming media shadows into new style.
They have m_mediaElement, which should be replaced with this API + Element::shadowHost().

[Roland Steiner]

Implementing the tree scope version (replacing the Document* with a pointer to a new class NodeTreeScope) I wonder if this added indirection between Node and Document won't raise speed concerns (?).

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> Implementing the tree scope version (replacing the Document* with a pointer to a new class NodeTreeScope) I wonder if this added indirection between Node and Document won't raise speed concerns (?).

In the past, I measured performance impact using either/both well-established DOM benchmarks and microbenchmarks I wrote myself -- and made determination based on that. Darin's comment implied just that. See bug 50184 as one example.

[Roland Steiner]

A "simple and hacky" version didn't seem much easier than a clean and forward-compatible version, so I'm going for that. Might have been a mistaken assessment in hidsight, though... :p

I any case, a (larger than anticipated) work-in-progress patch is forthcoming shortly.

[Roland Steiner]

Created attachment 80329 [details]
Work in Progress (shadow part not done yet)

[Dimitri Glazkov (Google)]

Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?

[Dimitri Glazkov (Google)]

I would really like Darin's and Dave's comment on the approach. Making ShadowRoot as an extra order in shadow DOM hierarchy seems complex, but there might be good reasons for it.

Roland, can you outline them here?

[Dimitri Glazkov (Google)]

Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?

[Darin Adler]

(In reply to comment #16)
> Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?

It should. I wrote all the code for that class; I don’t think any non-Apple copyrights need to be moved into the separate file with it.

[Darin Adler]

(In reply to comment #15)
> I would really like Darin's and Dave's comment on the approach. Making ShadowRoot as an extra order in shadow DOM hierarchy seems complex, but there might be good reasons for it.
> 
> Roland, can you outline them here?

I’m a little lost. I need a brief explanation of the goal an technique.

[Roland Steiner]

Thanks for the comments! You're of course right that the patch needs to be broken up once it goes for a review, but I wanted to first solicit some feedback regarding the general direction (quite a bit of it is inter-connected, others like ScopeContainerNode may not make sense if the whole approach is discarded).

The idea is that documents and shadow roots form a 'scope' that governs, for example, ID hashes. It also contains a pointer to the document, as well as taking over the selfOnlyRef logic. The new class ScopeContainerNode extracts this functionality into a super-class of Document and the new ShadowRoot class. 

Scopes can be nested: ShadowRoots can be within the scope of yet another ShadowRoot and so on. Document, of course, cannot have a parent scope (unless we perhaps extend that to IFrames later on, but I'm not sure how good of an idea that is).

Node is then changed from having a pointer to the document to a pointer to ScopeContainerClass. To get at the document the code takes the document pointer there. The new structure also allows us to remove the IsShadowRoot flag.

Node::setDocument and Node::setDocumentRecursively are replaced by Node::setScope, ScopeContainerNode::setScope and ScopeContainerNode::changeDocument. The logic here is that if a node changes documents (or scope in general) the scope pointers of nodes in nested scopes don't need to be updated, neither need selfOnyRef. However, document pointers still need to be changed and associated messages for affected nodes fired.

[Roland Steiner]

As for the shadow part:

ElemRareData would now point to a ShadowRoot that governs the scope of all nodes within the shadow. This way nodes in shadow can have O(1) access to their root, host, etc.

However, this DOES introduce a new, additional node in the shadow tree that wasn't present so far. Consequently, the logic of parentNode(), parentNodeOrHost(), etc., need to be changed. But in the long run I hope that the new node will actually make things easier rather than harder (looking forward to CSS scoping, for example).

ShadowRoot contains an additional pointer to the host element. Now, currently (and probably in the first version of the patch) the parent pointer will also continue to point to same. I believe that in a final solution it'd be cleaner to have the parent pointer be 0, and go back to the host via that additional pointer. This way nothing has a parent pointer that isn't also a child, and switching between scopes must happen explicitly. If that turns out to be not such a good idea, that additional pointer can be removed.

[Roland Steiner]

Addendum: the setScope functions are buggy ATM, because they introduce unwanted recursion. Fixing that.

[Roland Steiner]

Further addendum: I created a quick shared document with some captures of a rough overview.

https://docs.google.com/document/d/1bk46RIlpy8rmjSQZe_hZVv3g8oN_ygCegB9mw_6iWiw/edit?hl=en&authkey=CPGy750I

[Roland Steiner]

Some more easily readable drawings for the "advanced" version:

Class diagram:

https://docs.google.com/drawings/edit?id=1YBlsCpd8b2AgCBlOix-nrzIAgVnogjCiauPZt4b6Af8&hl=en&authkey=CLzgxuMG

Tree layout:

https://docs.google.com/drawings/edit?id=1XwnE6LhV4Omsc61bU8QzKYGEzdJ6Ifby-b2_QN6JEYA&hl=en&authkey=COa9xZYF

[Roland Steiner]

Created attachment 81056 [details]
Patch, Simple Version

Full version became too unwieldy, posting a 'simple' version instead as a first step. No r? yet as I haven't done any performance checks yet, but passes (almost) all layout tests - the failures there are on my local machine seem to be unrelated or flakiness, but I this also still needs to be verified.

[Dimitri Glazkov (Google)]

Comment on attachment 81056 [details]
Patch, Simple Version

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

> Source/WebCore/dom/DOMScope.h:39
> +class DOMScope {

I like DOMScope as the name.

> Source/WebCore/dom/DOMScope.h:56
> +    Document* documentPtr() const { return m_document; }

Why not just document()?

> Source/WebCore/dom/Element.cpp:1133
> +        delete shadowRoot;

Whenever I see a "delete" like this, I can't help but wonder if it's going to bite us in the ass one day. I think we should flesh out the ownership of ShadowRoot* better and use existing primitives, such as OwnPtr and PassOwnPtr.

> Source/WebCore/dom/ShadowRoot.h:60
> +    void setDocumentRecursively(Document*);

Darin pointed out to me once that "recursively" should be the rule, not the exception. So just use setDocument.

[Dimitri Glazkov (Google)]

Comment on attachment 81056 [details]
Patch, Simple Version

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

> Source/WebCore/dom/Node.h:598
> +        // allows us to do away with this dual-use of the parent pointer.

What kinds of ASSERTs? This smells fishy.

[Roland Steiner]

(In reply to comment #25)
> (From update of attachment 81056 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81056&action=review
> > Source/WebCore/dom/DOMScope.h:56
> > +    Document* documentPtr() const { return m_document; }
> 
> Why not just document()?

This was meant to avoid a name clash in Document, but using just document() is probably better in the long run, to avoid further refactoring and confusion. Changed the code in Document to use a 'using' declaration instead.
 
> > Source/WebCore/dom/Element.cpp:1133
> > +        delete shadowRoot;
> 
> Whenever I see a "delete" like this, I can't help but wonder if it's going to bite us in the ass one day. I think we should flesh out the ownership of ShadowRoot* better and use existing primitives, such as OwnPtr and PassOwnPtr.

That's a good point - have changed the code to use OwnPtr and RefPtr.

> > Source/WebCore/dom/ShadowRoot.h:60
> > +    void setDocumentRecursively(Document*);
> 
> Darin pointed out to me once that "recursively" should be the rule, not the exception. So just use setDocument.

This is meant to mirror the existing method in Node of the same name (where both setDocument and setDocumentRecursively exist).

But in the long run I want to replace this with a generic setScope anyway.

(In reply to comment #26)
> > Source/WebCore/dom/Node.h:598
> > +        // allows us to do away with this dual-use of the parent pointer.
> 
> What kinds of ASSERTs? This smells fishy.

The TextControlInnerElement constructor sets the ShadowHost pointer. RenderTextControl::createSubtreeIfNeeded() (from where it was constructed) calls attachInnerElement(). This calls deprecatedParserAddChild(), which calls parserAddChild(), which ASSERTs that the new child doesn't have a parent node.

[Roland Steiner]

Created attachment 81201 [details]
work-in-progress, see comments

[Roland Steiner]

Uploaded another work-in-progress (warning: this one again crashes ATM). Amongst other changes and corretions in this patch I renamed the "ShadowRoot" class into "ShadowScope" to avoid naming confusion within Element.

One more question that came up was how to handle the creation of new HTML elements: currently, all are created with a Document pointer, i.e., in the Document's scope with the new code. This could be thought of the default for elements that are not attached yet (and again, when they are detached). However, this requires the scope pointers of all nodes to be updated when attached into a shadow scope.

OTOH we could create shadow nodes directly into a shadow scope, but this would require changing the signature on all HTML... element constructors (and may run into as-of-yet-unknown hurdles).

[Dimitri Glazkov (Google)]

(In reply to comment #29)
> Uploaded another work-in-progress (warning: this one again crashes ATM). Amongst other changes and corretions in this patch I renamed the "ShadowRoot" class into "ShadowScope" to avoid naming confusion within Element.
> 
> One more question that came up was how to handle the creation of new HTML elements: currently, all are created with a Document pointer, i.e., in the Document's scope with the new code. This could be thought of the default for elements that are not attached yet (and again, when they are detached). However, this requires the scope pointers of all nodes to be updated when attached into a shadow scope.
> 
> OTOH we could create shadow nodes directly into a shadow scope, but this would require changing the signature on all HTML... element constructors (and may run into as-of-yet-unknown hurdles).

I have another question. Can we somehow use it for DocumentFragments?

[Roland Steiner]

(In reply to comment #30)
> I have another question. Can we somehow use it for DocumentFragments?

DOMScope should be ideally suited for DocumentFragment (in theory at least). Speaking of theory: We could also make it a container for nodes that are assigned to a Document, but not in the tree (i.e., where currently the inDocument flag is false).

[Dimitri Glazkov (Google)]

Roland, you stopped working on this because we first need to convert all existing instances of old shadow DOM to the new shadow DOM, right?

[Roland Steiner]

(In reply to comment #32)
> Roland, you stopped working on this because we first need to convert all existing instances of old shadow DOM to the new shadow DOM, right?

Yes - sorry, I should have made a note about this here sooner. The reason is that proper life-time handling between render-tree based shadow-DOM and node-based shadow DOM is rather tricky, so I would prefer to wait until conversion is done.

However, if this is blocking people, I can try to find a workable solution in the interim (which is likely going to be somewhat ugly, though).

[Dimitri Glazkov (Google)]

(In reply to comment #33)
> (In reply to comment #32)
> > Roland, you stopped working on this because we first need to convert all existing instances of old shadow DOM to the new shadow DOM, right?
> 
> Yes - sorry, I should have made a note about this here sooner. The reason is that proper life-time handling between render-tree based shadow-DOM and node-based shadow DOM is rather tricky, so I would prefer to wait until conversion is done.
> 
> However, if this is blocking people, I can try to find a workable solution in the interim (which is likely going to be somewhat ugly, though).

No, no worries -- I was just making sure I understood where this was :)

[Roland Steiner]

Created attachment 88311 [details]
new patch

New patch following the discussed approach. Document::destroyScope() only has moved to a more appropriate place, while Node::setDocumentRecursively() has moved, but also slightly changed.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1
Source/WebCore/dom/TreeScope.cpp:103:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 8 files


Source/WebCore/dom/TreeScope.cpp:103:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 8 files


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

[Roland Steiner]

Created attachment 88341 [details]
new, smaller patch

Moved functions back to original place to make review easier.

[Roland Steiner]

Note that in Node.h the functions setDocument and setDocumentRecursively are now protected (were public). They are superseded by setTreeScope and setTreeScopeRecursively.

[Dimitri Glazkov (Google)]

Comment on attachment 88341 [details]
new, smaller patch

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.

[Roland Steiner]

Created attachment 88564 [details]
new patch, using NodeRareData

[Dimitri Glazkov (Google)]

Comment on attachment 88564 [details]
new patch, using NodeRareData

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

> Source/WebCore/dom/Node.cpp:493
> +        ensureRareData()->setTreeScope(newTreeScope);

Should this not try to check if newTreeScope == newDocument and then not set anything? Otherwise, this would always initialize rare data when we move a node between documents.

> Source/WebCore/dom/Node.cpp:524
> +        if (!node->isElementNode())
> +            continue;
> +        /* FIXME: adapt to ShadowRoot (likely code below):
> +        if (ShadowRoot* shadowRoot = toElement(node)->shadowRoot()) {
> +            shadowRoot->setParentTreeScope(newTreeScope);
> +            if (currentDocument != newDocument)
> +                shadowRoot->setDocumentRecursively(newDocument);
> +        }
> +        */

We can remove this for now. No need to land commented-out code.

> Source/WebCore/dom/Node.h:641
> +    void setDocument(Document*);
> +    void setDocumentRecursively(Document*);

Can these be private?

[Dimitri Glazkov (Google)]

Comment on attachment 88564 [details]
new patch, using NodeRareData

Also, make sure to run all layout tests both in Debug and Release for this.

[Dimitri Glazkov (Google)]

Comment on attachment 88564 [details]
new patch, using NodeRareData

Also, this patch only introduces the concept of a TreeScope that's exactly equivalent to Document at this point. It is not aware of the shadow DOM yet.

Since this really doesn't fix bug 52963 yet, we should move this patch to a separate, blocking bug.

[Roland Steiner]

(In reply to comment #41)
> (From update of attachment 88564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88564&action=review
> 
> > Source/WebCore/dom/Node.cpp:493
> > +        ensureRareData()->setTreeScope(newTreeScope);
> 
> Should this not try to check if newTreeScope == newDocument and then not set anything? Otherwise, this would always initialize rare data when we move a node between documents.

This branch is for the case whether newTreeScope is not a Document (whether old or new). Switching documents is done in the line below (setDocument) that in turn checks whether the document actually changed.

It is true, however, that quite a bit of the code in this patch is not yet actually used (!) until we have bona fide shadow scopes.


> > Source/WebCore/dom/Node.cpp:524
> > +        if (!node->isElementNode())
> > +            continue;
> > +        /* FIXME: adapt to ShadowRoot (likely code below):
> > +        if (ShadowRoot* shadowRoot = toElement(node)->shadowRoot()) {
> > +            shadowRoot->setParentTreeScope(newTreeScope);
> > +            if (currentDocument != newDocument)
> > +                shadowRoot->setDocumentRecursively(newDocument);
> > +        }
> > +        */
> 
> We can remove this for now. No need to land commented-out code.

Ok, I'll just leave in a single-line comment then (?).


> > Source/WebCore/dom/Node.h:641
> > +    void setDocument(Document*);
> > +    void setDocumentRecursively(Document*);
> 
> Can these be private?

I left these protected, since I'm a bit torn whether, e.g., attaching a DocumentType should use setDocument or setTreeScope. ATM it doesn't really matter, but I thought that TreeScope, ShadowScope and/or Document might legitimately make use of these.

[Roland Steiner]

(In reply to comment #44)
> This branch is for the case whether newTreeScope is not a Document (whether old or new).

should be: ... case where newTreeScope...

[Roland Steiner]

(In reply to comment #43)
> (From update of attachment 88564 [details])
> Also, this patch only introduces the concept of a TreeScope that's exactly equivalent to Document at this point. It is not aware of the shadow DOM yet.
> 
> Since this really doesn't fix bug 52963 yet, we should move this patch to a separate, blocking bug.

Opened new bug 58060

[Roland Steiner]

Created attachment 89504 [details]
work-in-progress

Work-in-progress patch. Crashes during JS GC where Document::removedLastRef seems to be called twice - the first time there must be some Nodes left assigned to the Document that are not in the tree, that prevent destruction of the document.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/Node.cpp:511:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/dom/ShadowRoot.cpp:72:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/dom/Document.cpp:525:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 12 files


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

[Roland Steiner]

Work left after this: update data structures contained in TreeScope (node ID hash, element-by-ID, image map, access keys, node list cache counter) on the containing scope of a node rather than the document

[Dimitri Glazkov (Google)]

Comment on attachment 89504 [details]
work-in-progress

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

Let's discuss this on the whiteboard today.

> Source/WebCore/dom/ContainerNode.cpp:77
> +    // Note that we do NOT set the tree scope to document here, because this function is usu. called
> +    // when the children should be destroyed anyway. Call setTreeScopeRecursively where this is not the case.

Probably won't need this comment here when landing.

> Source/WebCore/dom/ContainerNode.cpp:660
> +    newChild->setTreeScopeRecursively(treeScope());

This will have performance implications. Can we instead piggyback on insertedIntoDocument?

> Source/WebCore/dom/Node.cpp:503
> +    for (Node* node = includeRoot ? this : traverseNextNode(this); node; node = node->traverseNextNode(this)) {

in the case of includeRoot == false,  you'll traverse next node twice.

[Roland Steiner]

Created attachment 89861 [details]
patch, setTreeScope version

Dromaeo times coming shortly

[Dimitri Glazkov (Google)]

Comment on attachment 89861 [details]
patch, setTreeScope version

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

looks good with nits. Let's see the numbers before landing though.

> Source/WebCore/dom/Element.h:233
> +    // FIXME: Make these return a proper ShadowRoot* (requires changes wrt. WebDOMOperations.h).

File a bug and put it in the fixme. No need to mention WebDOMOperations here.

> Source/WebCore/dom/Node.cpp:516
> +        // FIXME: Remove toShadowRoot() once shadowRoot() returns a proper ShadowRoot*.

Ditto.

> Source/WebCore/dom/ShadowRoot.cpp:58
> +    // FIXME: Decide correct node type.

Ditto.

> Source/WebCore/dom/ShadowRoot.cpp:62
> +PassRefPtr<Node> ShadowRoot::cloneNode(bool /*deep*/)

No need to have commented-out param here.

> Source/WebCore/dom/ShadowRoot.h:68
> +inline const ShadowRoot* toShadowRoot(const Node* node)
> +{
> +    ASSERT(!node || node->isShadowBoundary());
> +    return static_cast<const ShadowRoot*>(node);
> +}

Remove this since we don't use it.

> Source/WebCore/dom/TreeScope.h:39
> +    friend class Node;

Why does it need to be a friend here?

[Roland Steiner]

Not sure what I should think of the Dromaeo times - they are quite variable, even when run again with the same settings. Unexpectedly, it also shows a rather general plus in the number of runs/s with my patch...

with patch, all tests: http://dromaeo.com/?id=137913
with patch, DOM tests: http://dromaeo.com/?id=137922

original, all tests: http://dromaeo.com/?id=137931
original, DOM tests: http://dromaeo.com/?id=137941

Spreadsheet: https://spreadsheets.google.com/ccc?key=0AlmQQ415jP-_dHFHZ3dGcE5XTWdza2pGLVd4dGdzWFE&hl=en&authkey=CKaqu7UH

[Dimitri Glazkov (Google)]

Comment on attachment 89861 [details]
patch, setTreeScope version

let's give this puppy a whirl.

[Roland Steiner]

Created attachment 89870 [details]
updated patch

made setParentTreeScope() public, to avoid 'friend class Node'.

[Roland Steiner]

Committed r84050: <http://trac.webkit.org/changeset/84050>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84050 might have broken WinCairo Debug (Build)

[Dimitri Glazkov (Google)]

This was rolled on in http://trac.webkit.org/changeset/84251. Roland, can you take a look?

[Roland Steiner]

Created attachment 90292 [details]
patch, correct JS wrappers

With 58957 out of the way, let's try our luck once more. It's largely the same as previous patch, plus checking for ShadowRoot when creating JS wrappers.

[Roland Steiner]

Committed r84394: <http://trac.webkit.org/changeset/84394>