Bug 58060 – Prepare access to TreeScope from Node, using NodeRareData

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Roland Steiner]

Add a way to directly access a containing TreeScope from a Node, as a prerequisite step for issue 52963.

In the first version, this will be done via NodeRareData to avoid bloating Node.

[Roland Steiner]

Created attachment 88677 [details]
Patch

All Layout tests ran successvully, both in Debug and Release.

[Roland Steiner]

"successvully"? @_@;;

[Dimitri Glazkov (Google)]

Comment on attachment 88677 [details]
Patch

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

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

Can you explain this a bit more? For example, if I am moving from document A to document B, it seems that in this case _all_ nodes will run this code path and thus gain a bit of weight by RareNodeData being instantiated.

> Source/WebCore/dom/Node.cpp:517
> +        if (!node->isElementNode())
> +            continue;

This statement is useless?

[Roland Steiner]

Comment on attachment 88677 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:493
>> +        ensureRareData()->setTreeScope(newTreeScope);
> 
> Can you explain this a bit more? For example, if I am moving from document A to document B, it seems that in this case _all_ nodes will run this code path and thus gain a bit of weight by RareNodeData being instantiated.

If the new tree scope is a document, the if-branch should be taken - no extra rare data added (or the tree scope pointer therein reset to 0 in case it's already present).

The else-branch is strictly for the case where the new tree scope is NOT a document. It should never be taken currently, as we don't yet have a shadow scope. (Perhaps I should add a ASSERT_NOT_REACHED() with a FIXME to remove it once we land shadow scopes?)

>> Source/WebCore/dom/Node.cpp:517
>> +            continue;
> 
> This statement is useless?

D'oh - holy forgotten cleanup, batman!

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (From update of attachment 88677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88677&action=review
> 
> >> Source/WebCore/dom/Node.cpp:493
> >> +        ensureRareData()->setTreeScope(newTreeScope);
> > 
> > Can you explain this a bit more? For example, if I am moving from document A to document B, it seems that in this case _all_ nodes will run this code path and thus gain a bit of weight by RareNodeData being instantiated.
> 
> If the new tree scope is a document, the if-branch should be taken - no extra rare data added (or the tree scope pointer therein reset to 0 in case it's already present).
> 
> The else-branch is strictly for the case where the new tree scope is NOT a document. It should never be taken currently, as we don't yet have a shadow scope. (Perhaps I should add a ASSERT_NOT_REACHED() with a FIXME to remove it once we land shadow scopes?)

This sounds great!

> 
> >> Source/WebCore/dom/Node.cpp:517
> >> +            continue;
> > 
> > This statement is useless?
> 
> D'oh - holy forgotten cleanup, batman!

[Roland Steiner]

Created attachment 88869 [details]
new patch, added ASSERTs

[Roland Steiner]

Re-ran all layout tests in DEBUG, successfully. Re-running on RELEASE as well atm.

[Dimitri Glazkov (Google)]

Comment on attachment 88869 [details]
new patch, added ASSERTs

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

pretty cool.

> Source/WebCore/ChangeLog:28
> +        (WebCore::Document::setDocType): change DOM manipulation methods to update scope of inserted nodes

You can just say "Ditto." here.

> Source/WebCore/ChangeLog:29
> +        (WebCore::Document::adoptNode): change DOM manipulation methods to update scope of inserted nodes

Ditto.

> Source/WebCore/dom/Node.cpp:478
> +    // FIXME: until we land shadow scopes, there should be no non-document scopes.

Capitalize U.

> Source/WebCore/dom/Node.cpp:521
> +        // FIXME: once shadow scopes are landed, update parent scope, etc.

Capitalize O.

> Source/WebCore/dom/TreeScope.cpp:49
> +        // FIXME: this branch should be inert until shadow scopes are landed.

Capitalize T.

> Source/WebCore/dom/TreeScope.cpp:69
> +    // Cannot reparent a document!

Take out the exclamation point, turn into a sentence. :)

[Roland Steiner]

Committed r83349: <http://trac.webkit.org/changeset/83349>