Change WebURLLoaderClient::willFollowRedirect() API to return bool

third_party/WebKit/Source/core/fetch/ResourceLoader.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
index 5085721e81f06b478dd8247f3a6191cd55106163..8c9d1a56cf61c579bfc3792589f97c83269ac80d 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
@@ -123,7 +123,7 @@ void ResourceLoader::cancel()
     didFail(nullptr, ResourceError::cancelledError(m_resource->lastResourceRequest().url()));
 }
 
-void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewRequest, const WebURLResponse& passedRedirectResponse, int64_t encodedDataLength)
+bool ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewRequest, const WebURLResponse& passedRedirectResponse, int64_t encodedDataLength)
 {
     ASSERT(!passedNewRequest.isNull());
     ASSERT(!passedRedirectResponse.isNull());
@@ -132,12 +132,17 @@ void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewR
     const ResourceResponse& redirectResponse(passedRedirectResponse.toResourceResponse());
     newRequest.setRedirectStatus(ResourceRequest::RedirectStatus::FollowedRedirect);
 
+    const KURL originalURL = newRequest.url();
     if (m_fetcher->willFollowRedirect(m_resource.get(), newRequest, redirectResponse, encodedDataLength)) {
-        m_resource->willFollowRedirect(newRequest, redirectResponse);
+        if (m_resource->willFollowRedirect(newRequest, redirectResponse))
+            return newRequest.url() == originalURL;

[Nate Chapin, 2016/09/06 16:46:14]

I assume this is handling some old cases that still try to cancel the request by
blanking the url? Can we hunt those down and get rid of them?

[tyoshino (SeeGerritForStatus), 2016/09/07 10:52:28]

Inside m_fetcher->willFollowRedirect(), we may reach
WebFrameTestClient::willSendRequest() and RenderFrameImpl::willSendRequest()
which may rewrite the URL. Yeah, this line is to capture them.

They're about extension URLs and layout test output directories.

Maybe the right thing to do is to honor these rewrites but currently it doesn't
get reflected to net stack. It looks the best thing we can do here is not to
follow redirect.

+
+        return false;
     } else {
         m_resource->willNotFollowRedirect();
         if (m_loader)
             didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(newRequest.url()));

[Nate Chapin, 2016/09/06 16:46:14]

Is the didFail() still needed?

[tyoshino (SeeGerritForStatus), 2016/09/07 10:52:28]

Hmm, ResourceDispatcher::Cancel() will be invoked to clear the corresponding
PendingRequest. But that's it. It's only a part of what happens on
m_loader.reset(). So, we should keep this?

Is there any other path to do what's done in didFail()?

[Nate Chapin, 2016/09/07 17:05:45]

No. I guess ResourceDispatcher::Cancel() could trigger the didFail(), but that's
a bigger, less predictable change, so leaving the didFail() here is fine.

[tyoshino (SeeGerritForStatus), 2016/09/14 14:53:47]

OK. I'll try to do it later when I get chance.

+        return false;
     }
 }