Summary: | The code to create a NodeListsNodeData is duplicated everywhere | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, arv, darin, eric, ojan, sam, tkent, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 73853 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-12-06 16:28:20 PST
Created attachment 118139 [details]
cleanup
Comment on attachment 118139 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=118139&action=review > Source/WebCore/dom/Node.cpp:1003 > + if (data && data->nodeLists() && (!treeScope() || !treeScope()->hasNodeListCaches())) Oops, I obviously don't need to check for data here. Will fix before landing it. Created attachment 118143 [details]
Fixed some bug
Comment on attachment 118143 [details] Fixed some bug View in context: https://bugs.webkit.org/attachment.cgi?id=118143&action=review > Source/WebCore/dom/NodeRareData.h:135 > + setNodeLists(NodeListsNodeData::create()); > + if (TreeScope* treeScope = node->treeScope()) > + treeScope->addNodeListCache(); Do we really want this inlined? I would have put this part into a non-inline helper function. Also, this function is kind of long. Maybe we could put it outside the class definition. Comment on attachment 118143 [details] Fixed some bug View in context: https://bugs.webkit.org/attachment.cgi?id=118143&action=review >> Source/WebCore/dom/NodeRareData.h:135 >> + treeScope->addNodeListCache(); > > Do we really want this inlined? I would have put this part into a non-inline helper function. > > Also, this function is kind of long. Maybe we could put it outside the class definition. Sure, I can put this in Node.cpp. Thanks for the review. Comment on attachment 118143 [details] Fixed some bug View in context: https://bugs.webkit.org/attachment.cgi?id=118143&action=review >>> Source/WebCore/dom/NodeRareData.h:135 >>> + setNodeLists(NodeListsNodeData::create()); >>> + if (TreeScope* treeScope = node->treeScope()) >>> + treeScope->addNodeListCache(); >> >> Do we really want this inlined? I would have put this part into a non-inline helper function. >> >> Also, this function is kind of long. Maybe we could put it outside the class definition. > > Sure, I can put this in Node.cpp. Thanks for the review. I meant to apply that comment to all the code inside if (!m_nodeLists) Comment on attachment 118143 [details] Fixed some bug View in context: https://bugs.webkit.org/attachment.cgi?id=118143&action=review >>>> Source/WebCore/dom/NodeRareData.h:135 >>>> + treeScope->addNodeListCache(); >>> >>> Do we really want this inlined? I would have put this part into a non-inline helper function. >>> >>> Also, this function is kind of long. Maybe we could put it outside the class definition. >> >> Sure, I can put this in Node.cpp. Thanks for the review. > > I meant to apply that comment to all the code inside if (!m_nodeLists) Ok. Will add createNodeLists() and put that in Node.cpp. Committed r102208: <http://trac.webkit.org/changeset/102208> |