Bug 147871 – [Cocoa] Add redirect support to CustomProtocolManager

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

[Andy Estes]

[Cocoa] Add redirect support to CustomProtocolManager

[Andy Estes]

This is needed to write a test for https://bugs.webkit.org/show_bug.cgi?id=147872

[Andy Estes]

Created attachment 258709 [details]
Patch

[Andy Estes]

Created attachment 258750 [details]
Patch

[Andy Estes]

Created attachment 258751 [details]
Patch

Comment on attachment 258751 [details]
Patch

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

> Source/WebKit2/Shared/Network/CustomProtocols/Cocoa/CustomProtocolManagerCocoa.mm:54
> +using namespace WebCore;

I think recently in WebKit2 we have taken to avoiding using namespace WebCore. It sure looks like existing functions in this file already use WebCore::-qualified types.

> Source/WebKit2/Shared/Network/CustomProtocols/soup/CustomProtocolManagerSoup.cpp:110
> +{

Do we need a notImplemented() here?

[Alexey Proskuryakov]

Comment on attachment 258751 [details]
Patch

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

> Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:116
> +    if (redirectResponse) {
> +        _connection->send(Messages::CustomProtocolManager::WasRedirectedToRequest(_customProtocolID, request, redirectResponse), 0);
> +        return nil;
> +    }

For normal loading, -connection:willSendRequest:redirectResponse: needs to return "request" if the delegate agrees to take the redirect, nil if the request is canceled, and some other request if the delegate decides to modify it.

Is it a bug to return nil here, or do custom protocols work differently? If it's a bug, please add a test case that would have caught it.

[Andy Estes]

(In reply to comment #5)
> Comment on attachment 258751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258751&action=review
> 
> > Source/WebKit2/Shared/Network/CustomProtocols/Cocoa/CustomProtocolManagerCocoa.mm:54
> > +using namespace WebCore;
> 
> I think recently in WebKit2 we have taken to avoiding using namespace
> WebCore. It sure looks like existing functions in this file already use
> WebCore::-qualified types.

Oh, ok. I was actually going to remove all the WebCore::s in another patch, so that's good to know!

> 
> > Source/WebKit2/Shared/Network/CustomProtocols/soup/CustomProtocolManagerSoup.cpp:110
> > +{
> 
> Do we need a notImplemented() here?

I can add it.

Thanks for the review!

[Andy Estes]

(In reply to comment #6)
> Comment on attachment 258751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258751&action=review
> 
> > Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:116
> > +    if (redirectResponse) {
> > +        _connection->send(Messages::CustomProtocolManager::WasRedirectedToRequest(_customProtocolID, request, redirectResponse), 0);
> > +        return nil;
> > +    }
> 
> For normal loading, -connection:willSendRequest:redirectResponse: needs to
> return "request" if the delegate agrees to take the redirect, nil if the
> request is canceled, and some other request if the delegate decides to
> modify it.
> 
> Is it a bug to return nil here, or do custom protocols work differently? If
> it's a bug, please add a test case that would have caught it.

Custom protocols do work differently in this case. If I were to return the request here, then the connection would tell the embedder's NSURLProtocol to start loading the redirect request.

Then, when the Networking process receives WasRedirectedToRequest, it starts a redirect for the underlying connection, which triggers our normal willSendRequest() logic. If this results in a new request, then the Networking process's NSURLProtocol receives -startLoading, which starts a new connection in the UI process.

So returning nil here prevents duplicate loads from starting in the embedder's NSURLProtocol, and ensures that the request that is started is the one that WebKit has had a chance to modify.

[Andy Estes]

Committed r188517: <http://trac.webkit.org/changeset/188517>