Bug 73961

Summary: The code to create a NodeListsNodeData is duplicated everywhere
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: 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 Flags
cleanup
none
Fixed some bug darin: review+

Ryosuke Niwa
Reported 2011-12-06 16:28:20 PST
So much code duplication!
Attachments
cleanup (9.51 KB, patch)
2011-12-06 16:44 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed some bug (9.55 KB, patch)
2011-12-06 16:56 PST, Ryosuke Niwa 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-12-06 16:44:26 PST
Created attachment 118139 [details] cleanup
Ryosuke Niwa
Comment 2 2011-12-06 16:45:19 PST
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.
Ryosuke Niwa
Comment 3 2011-12-06 16:56:12 PST
Created attachment 118143 [details] Fixed some bug
Darin Adler
Comment 4 2011-12-06 17:17:48 PST
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.
Ryosuke Niwa
Comment 5 2011-12-06 17:20:50 PST
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.
Darin Adler
Comment 6 2011-12-06 17:22:13 PST
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)
Ryosuke Niwa
Comment 7 2011-12-06 17:26:53 PST
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.
Ryosuke Niwa
Comment 8 2011-12-06 19:17:11 PST
Committed r102208: <http://trac.webkit.org/changeset/102208>
Note You need to log in before you can comment on or make changes to this bug.