Change WebURLLoaderClient::willFollowRedirect() API to return bool

third_party/WebKit/Source/core/loader/DocumentLoader.cpp

Index: third_party/WebKit/Source/core/loader/DocumentLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
index c29564fccd737f49b3a7022ed98cd0f706a2937c..7b77a1fd68554312e93635e7a2d6158de8e67c93 100644
--- a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
@@ -321,7 +321,7 @@ void DocumentLoader::finishedLoading(double finishTime)
     clearMainResourceHandle();
 }
 
-void DocumentLoader::redirectReceived(Resource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse)
+bool DocumentLoader::redirectReceived(Resource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse)
 {
     ASSERT_UNUSED(resource, resource == m_mainResource);
     ASSERT(!redirectResponse.isNull());
@@ -334,17 +334,19 @@ void DocumentLoader::redirectReceived(Resource* resource, ResourceRequest& reque
     if (!redirectingOrigin->canDisplay(requestURL)) {
         FrameLoader::reportLocalLoadFailed(m_frame, requestURL.getString());
         m_fetcher->stopFetching();
-        return;
+        return true;

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

If you return false here, you can probably remove the stopFetching() call and
get the same result.

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

I agree that we can change the return value here and at L341 to false.

For the same reason as I wrote for the comment on didFail(), I'm not sure if
it's fine to omit it.

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

Hrm, maybe ResourceLoader::willFollowRedirect() should guarantee that it calls
didFail() any time it returns false? Requiring ResourceClients to do that work
seems unreliable.

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

Any issue with only failing this RawResource? At this point, the main resource
is not yet loaded, but isn't it possible that m_fetcher is already used for
loading some subresources given e.g. some pre-load/fetch-ing directives, etc.?

[Nate Chapin, 2016/09/14 22:25:27]

Subresources can only be loaded once this DocumentLoader is committed. Redirects
are guaranteed to happen before commit, so you should have a strong guarantee
that the main resource is the only fetch in progress.

[tyoshino (SeeGerritForStatus), 2016/09/16 07:19:58]

OK. Removing stopFetching() calls here and at L340.

     }
     if (!frameLoader()->shouldContinueForNavigationPolicy(m_request, SubstituteData(), this, CheckContentSecurityPolicy, m_navigationType, NavigationPolicyCurrentTab, replacesCurrentHistoryItem(), isClientRedirect())) {
         m_fetcher->stopFetching();
-        return;
+        return true;
     }
 
     ASSERT(timing().fetchStart());
     appendRedirect(requestURL);
     didRedirect(redirectResponse.url(), requestURL);
     frameLoader()->client()->dispatchDidReceiveServerRedirectForProvisionalLoad();
+
+    return true;
 }
 
 static bool canShowMIMEType(const String& mimeType, LocalFrame* frame)