Bug 58060

Summary: Prepare access to TreeScope from Node, using NodeRareData
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: DOMAssignee: Roland Steiner <rolandsteiner>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, morrita
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52963    
Attachments:
Description Flags
Patch
none
new patch, added ASSERTs dglazkov: review+

Roland Steiner
Reported 2011-04-07 10:41:58 PDT
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.
Attachments
Patch (14.80 KB, patch)
2011-04-07 13:16 PDT, Roland Steiner
no flags
new patch, added ASSERTs (15.37 KB, patch)
2011-04-08 13:48 PDT, Roland Steiner
dglazkov: review+
Roland Steiner
Comment 1 2011-04-07 13:16:30 PDT
Created attachment 88677 [details] Patch All Layout tests ran successvully, both in Debug and Release.
Roland Steiner
Comment 2 2011-04-07 13:17:16 PDT
"successvully"? @_@;;
Dimitri Glazkov (Google)
Comment 3 2011-04-07 13:23:10 PDT
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 4 2011-04-07 14:16:01 PDT
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)
Comment 5 2011-04-08 07:03:21 PDT
(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
Comment 6 2011-04-08 13:48:01 PDT
Created attachment 88869 [details] new patch, added ASSERTs
Roland Steiner
Comment 7 2011-04-08 13:49:05 PDT
Re-ran all layout tests in DEBUG, successfully. Re-running on RELEASE as well atm.
Dimitri Glazkov (Google)
Comment 8 2011-04-08 13:55:39 PDT
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
Comment 9 2011-04-08 15:37:52 PDT
Note You need to log in before you can comment on or make changes to this bug.