Bug 100879 – ScrollingStateNode::cloneAndResetNode() should not be virtual

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

ScrollingStateNode::cloneAndResetNode() is currently pure virtual, and its implementation is in ScrollingStateScrollingNode. However, all of the work that it does at this time is stuff that a generic ScrollingStateNode could do. We should move this implementation to the base class so that it does not need to be duplicated for future node types.

[Beth Dakin]

Created attachment 171706 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 171706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171706&action=review

> Source/WebCore/page/scrolling/ScrollingStateNode.h:55
> +    PassOwnPtr<ScrollingStateNode> cloneAndResetNode();

Maybe we should also change the name; the trailing 'node' is redundant, and suggests that it should take a node parameter.

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:44
> +    ScrollingStateScrollingNode(ScrollingStateScrollingNode*);

Is this a copy constructor? Those normally take const Foo&

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 171706 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171706&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingStateNode.h:55
> > +    PassOwnPtr<ScrollingStateNode> cloneAndResetNode();
> 
> Maybe we should also change the name; the trailing 'node' is redundant, and suggests that it should take a node parameter.
> 

Good idea. Changed.

> > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:44
> > +    ScrollingStateScrollingNode(ScrollingStateScrollingNode*);
> 
> Is this a copy constructor? Those normally take const Foo&

It is a copy constructor. I will change this too.

[Beth Dakin]

http://trac.webkit.org/changeset/133067