Bug 73961 - The code to create a NodeListsNodeData is duplicated everywhere
Summary: The code to create a NodeListsNodeData is duplicated everywhere
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 73853
  Show dependency treegraph
 
Reported: 2011-12-06 16:28 PST by Ryosuke Niwa
Modified: 2011-12-06 19:17 PST (History)
8 users (show)

See Also:


Attachments
cleanup (9.51 KB, patch)
2011-12-06 16:44 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed some bug (9.55 KB, patch)
2011-12-06 16:56 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-12-06 16:28:20 PST
So much code duplication!
Comment 1 Ryosuke Niwa 2011-12-06 16:44:26 PST
Created attachment 118139 [details]
cleanup
Comment 2 Ryosuke Niwa 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.
Comment 3 Ryosuke Niwa 2011-12-06 16:56:12 PST
Created attachment 118143 [details]
Fixed some bug
Comment 4 Darin Adler 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Darin Adler 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)
Comment 7 Ryosuke Niwa 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.
Comment 8 Ryosuke Niwa 2011-12-06 19:17:11 PST
Committed r102208: <http://trac.webkit.org/changeset/102208>