Bug 173682 – Make FrameLoadRequest a move-only type

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

We should look to make FrameLoadRequest a move-only type as it is largely used as a convenience object for passing information around that is needed to perform a load.

[Daniel Bates]

Created attachment 313573 [details]
Patch

[Daniel Bates]

Comment on attachment 313573 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:926
> +    childFrame->loader().loadURL(WTFMove(frameLoadRequest), referer, FrameLoadType::RedirectWithLockedBackForwardList, 0, 0);

Will change 0 to nullptr for the last two arguments before landing.

> 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.)

> 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&.

[Alex Christensen]

Comment on attachment 313573 [details]
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&&?

[Darin Adler]

Comment on attachment 313573 [details]
Patch

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

>> Source/WebKit/win/Plugins/PluginView.h:84
>> +            : m_frameLoadRequest { WTFMove(frameLoadRequest) }
> 
> Did WebKit style change to encourage using initializer lists in the member initializers?

This came up in Daniel’s last patch too.

We are always defining WebKit style live as a team. Some of us are preferring the brace syntax rather than the parentheses, specifically because the parentheses syntax silently allows certain type conversions that must be made explicit in the brace syntax. Because of that, I prefer braces almost any place we can use them, including here.

[Darin Adler]

Comment on attachment 313573 [details]
Patch

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

>>> 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&&?

I think in general we wouldn’t want functions to take ownership of an object when they simply need to examine the object. Just as if something can take a const*, we want to use that instead of a non-const *.

[Daniel Bates]

Comment on attachment 313573 [details]
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.

Will remove it to make consistent with the code change in WebCore::createWindow().

>> Source/WebKit/win/Plugins/PluginView.h:84
>> +            : m_frameLoadRequest { WTFMove(frameLoadRequest) }
> 
> Did WebKit style change to encourage using initializer lists in the member initializers?

See bug #173564, comment 14. We should bring this up on webkit-dev for discussion.

>>> 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&&?

I was wavering on having Chrome::createWindow() take a FrameLoadRequest&& (and hence inadvertently kept the WTFMove() I added in this line). I chose not to change the prototype of Chrome::createWindow() and have it continue to take a cont FrameLoadRequest& because this function does not perform an actual load and hence I do not see the need for it to take ownership of the FrameLoadRequest. Chrome::createWindow() passes the FrameLoadRequest, which represents the frame that requested the new window, to the embedding client should the embedding client wish to make use of this info when deciding whether to actually create the new window. It seems reasonable that the caller keeps ownership of the FrameLoadRequest in this case to do as they wish. For instance, when you use the context menu to open a hyperlink in a new window we ultimately call into <https://trac.webkit.org/browser/trunk/Source/WebCore/page/ContextMenuController.cpp?rev=218649#L186> which uses the same FrameLoadRequest to ask the embedding client to create a new window and initiate a load in the new window if the embedding client created it.

[Daniel Bates]

Committed r218713: <http://trac.webkit.org/changeset/218713>

[Radar WebKit Bug Importer]

<rdar://problem/32930446>

[Myles C. Maxfield]

(In reply to Darin Adler from comment #4)
> Comment on attachment 313573 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313573&action=review
> 
> >> Source/WebKit/win/Plugins/PluginView.h:84
> >> +            : m_frameLoadRequest { WTFMove(frameLoadRequest) }
> > 
> > Did WebKit style change to encourage using initializer lists in the member initializers?
> 
> This came up in Daniel’s last patch too.
> 
> We are always defining WebKit style live as a team. Some of us are
> preferring the brace syntax rather than the parentheses, specifically
> because the parentheses syntax silently allows certain type conversions that
> must be made explicit in the brace syntax. Because of that, I prefer braces
> almost any place we can use them, including here.

If this is the policy, can we put it in the style guide?

[Darin Adler]

I’m not sure that “some of us” and “I prefer” justifies saying this is “the policy”, but I would support making it official if there is some consensus we should move this way.