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..6396070d6eaf4cb2a98aa23d1ecb4248f0487200 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,24 @@ 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)) {

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

This nesting is a bit difficult to reason about, in my opinion. Could we invert
this if() statement and have a line of early-exits? Handle the
!m_fetcher->willFollowRedirect() case, then the
!m_resource->willFollowRedirect(newRequest, redirectResponse), then the changed
url case?

Alternatively, given my comment in DocumentLoader about stopFetching(), maybe
all the non-follow cases should fall through and do a single didFail() and
return false at the end?

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

Nice suggestion. Reorganized.
Done adding didFail() and willNotFollowRedirect() calls. I've changed the order
of URL comparison and Resource::willFollowRedirect() as it's useless to invoke
willFollowRedirect() for fetch for which we've decided not to follow.

I kept the reorganized structure as it's more readable.

-        m_resource->willFollowRedirect(newRequest, redirectResponse);
+        if (m_resource->willFollowRedirect(newRequest, redirectResponse)) {
+            // ResourceFetcher::willFollowRedirect() may rewrite the URL to
+            // something else not for rejecting redirect but for other reasons.
+            // E.g. WebFrameTestClient::willSendRequest() and
+            // RenderFrameImpl::willSendRequest(). We should reflect the
+            // rewriting but currently we cannot. So, return false to make the
+            // redirect fail.
+            return newRequest.url() == originalURL;
+        }
+
+        return false;
     } else {
         m_resource->willNotFollowRedirect();
         if (m_loader)
             didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
+        return false;
     }
 }