Bug 199535 – WebBackForwardListItem::setPageState should receive pageState by reference

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Michael Catanzaro]

Coverity is complaining here about copying PageState by value in the parameter list. It's sort of a false positive, in that the PageState really does need to be copied here, so this is the best we can do. But pass by value and then WTFMove() is a pretty strange way to write it. Just passing by reference would be better. Then it will be copied into m_itemState.pageState.

[Michael Catanzaro]

Created attachment 373542 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 373542 [details]
Patch

Looks good to me.
I think you need to CC some reviewers or this will be unnoticed

[youenn fablet]

Comment on attachment 373542 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        m_itemState.pageState..

I would tend to go with setPageState(PageState&&), except if it requires too much explicit PageState { } at call sites.
Doing setPageState(PageState) is somehow better than setPageState(const&) since it probably accepts both const& and && as parameters.

[Michael Catanzaro]

Created attachment 373971 [details]
Patch

[Michael Catanzaro]

(In reply to youenn fablet from comment #3)
> > Source/WebKit/ChangeLog:12
> > +        m_itemState.pageState..
> 
> I would tend to go with setPageState(PageState&&), except if it requires too
> much explicit PageState { } at call sites.

Sure. There's only one callsite.

[WebKit Commit Bot]

Comment on attachment 373971 [details]
Patch

Clearing flags on attachment: 373971

Committed r247396: <https://trac.webkit.org/changeset/247396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53025386>