[webkit-reviews] review granted: [Bug 173682] Make FrameLoadRequest a move-only type : [Attachment 313573] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 173682: Make FrameLoadRequest a move-only type
https://bugs.webkit.org/show_bug.cgi?id=173682

Attachment 313573: Patch

https://bugs.webkit.org/attachment.cgi?id=313573&action=review




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 313573
  --> https://bugs.webkit.org/attachment.cgi?id=313573
Patch

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

>> Source/WebCore/loader/FrameLoader.cpp:1252
>> +	    FrameLoadRequest newFrameLoadRequest = WTFMove(frameLoadRequest);
> 
> Will change this to use the move constructor before landing:
> 
> FrameLoadRequest newFrameLoadRequest { WTFMove(frameLoadRequest) };
> 
> (Note we do not need to explicitly define this local. I kept this local as I
thought that it improves the readability of the code by making it more explicit
that we are repurposes the FrameLoadRequest. Let me know if it is preferred to
remove this local.)

Either way seems ok to me.  You removed a local variable in createWindow and
removing it here seems consistent.

> Source/WebKit/win/Plugins/PluginView.h:84
> +	       : m_frameLoadRequest { WTFMove(frameLoadRequest) }

Did WebKit style change to encourage using initializer lists in the member
initializers?

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:661
>> +	Page* newPage =
webPage->corePage()->chrome().createWindow(*m_frame->coreFrame(),
WTFMove(frameLoadRequest), { }, navigationAction);
> 
> Will revert this change before landing. It is unnecessary to WTFMove() the
FrameLoadRequest as Chrome::createWindow() takes a const FrameLoadRequest&.

Why not just make Chrome::createWindow take a FrameLoadRequest&&?