Bug 52963 - Enable O(1) access to root from any node in shadow DOM subtree
: Enable O(1) access to root from any node in shadow DOM subtree
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 52788 53289 57689 57994 58060 58892 58957
: 52917 52962 53001 54025
  Show dependency treegraph
 
Reported: 2011-01-22 10:43 PST by
Modified: 2011-09-20 13:07 PST (History)


Attachments
Work in Progress (shadow part not done yet) (72.92 KB, patch)
2011-01-27 06:32 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
Patch, Simple Version (52.66 KB, patch)
2011-02-03 06:24 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
work-in-progress, see comments (59.47 KB, patch)
2011-02-04 02:33 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
new patch (17.89 KB, patch)
2011-04-05 14:53 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
new, smaller patch (15.22 KB, patch)
2011-04-05 17:19 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
new patch, using NodeRareData (15.14 KB, patch)
2011-04-06 20:28 PST, Roland Steiner
dglazkov: review-
Review Patch | Details | Formatted Diff | Diff
work-in-progress (17.63 KB, patch)
2011-04-13 17:47 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
patch, setTreeScope version (15.63 KB, patch)
2011-04-15 15:06 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (15.66 KB, patch)
2011-04-15 16:09 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
patch, correct JS wrappers (18.05 KB, patch)
2011-04-19 19:07 PST, Roland Steiner
dglazkov: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-22 10:43:12 PST
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.
------- Comment #1 From 2011-01-22 10:58:32 PST -------
(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.
------- Comment #2 From 2011-01-23 15:43:26 PST -------
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.
------- Comment #3 From 2011-01-23 16:00:23 PST -------
(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!
------- Comment #4 From 2011-01-23 16:50:53 PST -------
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?
------- Comment #5 From 2011-01-23 16:51:20 PST -------
(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.
------- Comment #6 From 2011-01-23 16:52:44 PST -------
Adding the usual suspects for the discussion. Darin, Dave, what do you think?
------- Comment #7 From 2011-01-23 17:34:10 PST -------
Standard space vs. speed tradeoff. Just need to find a way to measure both.
------- Comment #8 From 2011-01-24 00:43:50 PST -------
Will have a go at the "tree scope" version.
------- Comment #9 From 2011-01-24 03:20:16 PST -------
(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().
------- Comment #10 From 2011-01-24 21:17:24 PST -------
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 (?).
------- Comment #11 From 2011-01-25 09:19:07 PST -------
(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.
------- Comment #12 From 2011-01-27 04:45:03 PST -------
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.
------- Comment #13 From 2011-01-27 06:32:15 PST -------
Created an attachment (id=80329) [details]
Work in Progress (shadow part not done yet)
------- Comment #14 From 2011-01-27 09:05:31 PST -------
Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?
------- Comment #15 From 2011-01-27 09:09:14 PST -------
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?
------- Comment #16 From 2011-01-27 10:41:59 PST -------
Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?
------- Comment #17 From 2011-01-27 11:40:17 PST -------
(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.
------- Comment #18 From 2011-01-27 11:40:45 PST -------
(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.
------- Comment #19 From 2011-01-27 18:21:03 PST -------
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.
------- Comment #20 From 2011-01-27 18:32:23 PST -------
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.
------- Comment #21 From 2011-01-27 18:34:27 PST -------
Addendum: the setScope functions are buggy ATM, because they introduce unwanted recursion. Fixing that.
------- Comment #22 From 2011-02-03 00:59:45 PST -------
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
------- Comment #24 From 2011-02-03 06:24:05 PST -------
Created an attachment (id=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.
------- Comment #25 From 2011-02-03 13:33:47 PST -------
(From update of attachment 81056 [details])
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.
------- Comment #26 From 2011-02-03 16:44:22 PST -------
(From update of attachment 81056 [details])
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.
------- Comment #27 From 2011-02-04 02:16:33 PST -------
(In reply to comment #25)
> (From update of attachment 81056 [details] [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.
------- Comment #28 From 2011-02-04 02:33:26 PST -------
Created an attachment (id=81201) [details]
work-in-progress, see comments
------- Comment #29 From 2011-02-04 02:40:16 PST -------
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).
------- Comment #30 From 2011-02-04 07:47:29 PST -------
(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?
------- Comment #31 From 2011-02-06 18:56:21 PST -------
(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).
------- Comment #32 From 2011-03-06 08:29:16 PST -------
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?
------- Comment #33 From 2011-03-07 01:50:04 PST -------
(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).
------- Comment #34 From 2011-03-07 09:06:56 PST -------
(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 :)
------- Comment #35 From 2011-04-05 14:53:28 PST -------
Created an attachment (id=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.
------- Comment #36 From 2011-04-05 14:56:30 PST -------
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.
------- Comment #37 From 2011-04-05 17:19:00 PST -------
Created an attachment (id=88341) [details]
new, smaller patch

Moved functions back to original place to make review easier.
------- Comment #38 From 2011-04-05 17:20:27 PST -------
Note that in Node.h the functions setDocument and setDocumentRecursively are now protected (were public). They are superseded by setTreeScope and setTreeScopeRecursively.
------- Comment #39 From 2011-04-05 18:26:32 PST -------
(From update of attachment 88341 [details])
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.
------- Comment #40 From 2011-04-06 20:28:41 PST -------
Created an attachment (id=88564) [details]
new patch, using NodeRareData
------- Comment #41 From 2011-04-06 20:54:29 PST -------
(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.

> 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?
------- Comment #42 From 2011-04-06 21:29:26 PST -------
(From update of attachment 88564 [details])
Also, make sure to run all layout tests both in Debug and Release for this.
------- Comment #43 From 2011-04-07 09:23:06 PST -------
(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.
------- Comment #44 From 2011-04-07 10:36:11 PST -------
(In reply to comment #41)
> (From update of attachment 88564 [details] [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.
------- Comment #45 From 2011-04-07 10:48:40 PST -------
(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...
------- Comment #46 From 2011-04-07 10:49:56 PST -------
(In reply to comment #43)
> (From update of attachment 88564 [details] [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
------- Comment #47 From 2011-04-13 17:47:11 PST -------
Created an attachment (id=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.
------- Comment #48 From 2011-04-13 17:51:01 PST -------
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.
------- Comment #49 From 2011-04-13 17:59:26 PST -------
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
------- Comment #50 From 2011-04-14 09:23:45 PST -------
(From update of attachment 89504 [details])
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.
------- Comment #51 From 2011-04-15 15:06:18 PST -------
Created an attachment (id=89861) [details]
patch, setTreeScope version

Dromaeo times coming shortly
------- Comment #52 From 2011-04-15 15:19:06 PST -------
(From update of attachment 89861 [details])
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?
------- Comment #53 From 2011-04-15 15:22:14 PST -------
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
------- Comment #54 From 2011-04-15 15:29:02 PST -------
(From update of attachment 89861 [details])
let's give this puppy a whirl.
------- Comment #55 From 2011-04-15 16:09:17 PST -------
Created an attachment (id=89870) [details]
updated patch

made setParentTreeScope() public, to avoid 'friend class Node'.
------- Comment #56 From 2011-04-15 16:47:34 PST -------
Committed r84050: <http://trac.webkit.org/changeset/84050>
------- Comment #57 From 2011-04-15 18:21:56 PST -------
http://trac.webkit.org/changeset/84050 might have broken WinCairo Debug (Build)
------- Comment #58 From 2011-04-19 07:06:11 PST -------
This was rolled on in http://trac.webkit.org/changeset/84251. Roland, can you take a look?
------- Comment #59 From 2011-04-19 19:07:13 PST -------
Created an attachment (id=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.
------- Comment #60 From 2011-04-20 14:40:41 PST -------
Committed r84394: <http://trac.webkit.org/changeset/84394>