Bug 52963 - Enable O(1) access to root from any node in shadow DOM subtree
Summary: Enable O(1) access to root from any node in shadow DOM subtree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Roland Steiner
URL:
Keywords:
Depends on: 52788 53289 57689 57994 58060 58892 58957
Blocks: 54025 52917 52962 53001
  Show dependency treegraph
 
Reported: 2011-01-22 10:43 PST by Dimitri Glazkov (Google)
Modified: 2011-09-20 13:07 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 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 Dimitri Glazkov (Google) 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 Hajime Morrita 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 Dimitri Glazkov (Google) 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 Dimitri Glazkov (Google) 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 Dimitri Glazkov (Google) 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 Dimitri Glazkov (Google) 2011-01-23 16:52:44 PST
Adding the usual suspects for the discussion. Darin, Dave, what do you think?
Comment 7 Darin Adler 2011-01-23 17:34:10 PST
Standard space vs. speed tradeoff. Just need to find a way to measure both.
Comment 8 Roland Steiner 2011-01-24 00:43:50 PST
Will have a go at the "tree scope" version.
Comment 9 Hajime Morrita 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 Roland Steiner 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 Dimitri Glazkov (Google) 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 Roland Steiner 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 Roland Steiner 2011-01-27 06:32:15 PST
Created attachment 80329 [details]
Work in Progress (shadow part not done yet)
Comment 14 Dimitri Glazkov (Google) 2011-01-27 09:05:31 PST
Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?
Comment 15 Dimitri Glazkov (Google) 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 Dimitri Glazkov (Google) 2011-01-27 10:41:59 PST
Sounds like DocumentOrderedMap is a no-brainer extraction that could be done first?
Comment 17 Darin Adler 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 Darin Adler 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 Roland Steiner 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 Roland Steiner 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 Roland Steiner 2011-01-27 18:34:27 PST
Addendum: the setScope functions are buggy ATM, because they introduce unwanted recursion. Fixing that.
Comment 22 Roland Steiner 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 Roland Steiner 2011-02-03 06:24:05 PST
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.
Comment 25 Dimitri Glazkov (Google) 2011-02-03 13:33:47 PST
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.
Comment 26 Dimitri Glazkov (Google) 2011-02-03 16:44:22 PST
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.
Comment 27 Roland Steiner 2011-02-04 02:16:33 PST
(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.
Comment 28 Roland Steiner 2011-02-04 02:33:26 PST
Created attachment 81201 [details]
work-in-progress, see comments
Comment 29 Roland Steiner 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 Dimitri Glazkov (Google) 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 Roland Steiner 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 Dimitri Glazkov (Google) 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 Roland Steiner 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 Dimitri Glazkov (Google) 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 Roland Steiner 2011-04-05 14:53:28 PDT
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.
Comment 36 WebKit Review Bot 2011-04-05 14:56:30 PDT
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 Roland Steiner 2011-04-05 17:19:00 PDT
Created attachment 88341 [details]
new, smaller patch

Moved functions back to original place to make review easier.
Comment 38 Roland Steiner 2011-04-05 17:20:27 PDT
Note that in Node.h the functions setDocument and setDocumentRecursively are now protected (were public). They are superseded by setTreeScope and setTreeScopeRecursively.
Comment 39 Dimitri Glazkov (Google) 2011-04-05 18:26:32 PDT
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.
Comment 40 Roland Steiner 2011-04-06 20:28:41 PDT
Created attachment 88564 [details]
new patch, using NodeRareData
Comment 41 Dimitri Glazkov (Google) 2011-04-06 20:54:29 PDT
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?
Comment 42 Dimitri Glazkov (Google) 2011-04-06 21:29:26 PDT
Comment on attachment 88564 [details]
new patch, using NodeRareData

Also, make sure to run all layout tests both in Debug and Release for this.
Comment 43 Dimitri Glazkov (Google) 2011-04-07 09:23:06 PDT
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.
Comment 44 Roland Steiner 2011-04-07 10:36:11 PDT
(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.
Comment 45 Roland Steiner 2011-04-07 10:48:40 PDT
(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 Roland Steiner 2011-04-07 10:49:56 PDT
(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
Comment 47 Roland Steiner 2011-04-13 17:47:11 PDT
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.
Comment 48 WebKit Review Bot 2011-04-13 17:51:01 PDT
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 Roland Steiner 2011-04-13 17:59:26 PDT
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 Dimitri Glazkov (Google) 2011-04-14 09:23:45 PDT
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.
Comment 51 Roland Steiner 2011-04-15 15:06:18 PDT
Created attachment 89861 [details]
patch, setTreeScope version

Dromaeo times coming shortly
Comment 52 Dimitri Glazkov (Google) 2011-04-15 15:19:06 PDT
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?
Comment 53 Roland Steiner 2011-04-15 15:22:14 PDT
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 Dimitri Glazkov (Google) 2011-04-15 15:29:02 PDT
Comment on attachment 89861 [details]
patch, setTreeScope version

let's give this puppy a whirl.
Comment 55 Roland Steiner 2011-04-15 16:09:17 PDT
Created attachment 89870 [details]
updated patch

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