Bug 174945 – Add SPI for returning a modified request in decidePolicyForNavigationAction's decisionHandler

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Alex Christensen]

Add SPI for returning a modified request in decidePolicyForNavigationAction's decisionHandler

[Alex Christensen]

Created attachment 316650 [details]
Patch

[Geoffrey Garen]

/Volumes/Data/EWS/WebKit/Source/WebCore/loader/PolicyChecker.cpp:130:48: error: too few arguments to function call, expected 2, have 1
        continueAfterNavigationPolicy(PolicyUse);

[Alex Christensen]

Created attachment 316673 [details]
Patch

[Build Bot]

Comment on attachment 316673 [details]
Patch

Attachment 316673 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4204404

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316674 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 316673 [details]
Patch

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

I did not review the concept but I did review the copying of ResourceRequest objects.

> Source/WebCore/loader/PolicyCallback.h:56
> +    void setRequest(ResourceRequest& newRequest) { m_request = newRequest; }

I think this should either take a ResourceRequest&& and use WTFMove, or it should take a const ResourceRequest& and always copy. Taking a non-const reference and copying does not seem right.

> Source/WebCore/loader/PolicyChecker.cpp:151
> +        continueAfterNavigationPolicy(action, newRequest);

I think that WTFMove here would avoid copying the ResourceRequest.

> Source/WebCore/loader/PolicyChecker.cpp:222
> +                callback.setRequest(*newRequest);

I think that WTFMove here would allow us to avoid copying the entire ResourceRequest.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:834
> +        newRequest = navigationDecision.newRequest;

Using WTFMove here we could avoid copying the ResourceRequest.

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:256
> +    function(action, newRequest);

Adding WTFMove here would avoid copying the ResourceRequest.

[Alex Christensen]

Comment on attachment 316673 [details]
Patch

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

>> Source/WebCore/loader/PolicyCallback.h:56
>> +    void setRequest(ResourceRequest& newRequest) { m_request = newRequest; }
> 
> I think this should either take a ResourceRequest&& and use WTFMove, or it should take a const ResourceRequest& and always copy. Taking a non-const reference and copying does not seem right.

I think PolicyCallback should be removed and replaced by WTF::Function and lambdas, but not in this patch.

[Alex Christensen]

We're not needing this any more