Summary: | Prepare access to TreeScope from Node, using NodeRareData | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roland Steiner <rolandsteiner> | ||||||
Component: | DOM | Assignee: | 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
Roland Steiner
2011-04-07 10:41:58 PDT
Created attachment 88677 [details]
Patch
All Layout tests ran successvully, both in Debug and Release.
"successvully"? @_@;; 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? 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! (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! Created attachment 88869 [details]
new patch, added ASSERTs
Re-ran all layout tests in DEBUG, successfully. Re-running on RELEASE as well atm. 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. :) Committed r83349: <http://trac.webkit.org/changeset/83349> |