Bug 73961 – The code to create a NodeListsNodeData is duplicated everywhere

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

So much code duplication!

[Ryosuke Niwa]

Created attachment 118139 [details]
cleanup

[Ryosuke Niwa]

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]

Created attachment 118143 [details]
Fixed some bug

[Darin Adler]

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 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 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 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]

Committed r102208: <http://trac.webkit.org/changeset/102208>