[Cocoa] Add redirect support to CustomProtocolManager
This is needed to write a test for https://bugs.webkit.org/show_bug.cgi?id=147872
Created attachment 258709 [details] Patch
Created attachment 258750 [details] Patch
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?
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.
(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!
(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.
Committed r188517: <http://trac.webkit.org/changeset/188517>