Change WebURLLoaderClient::willFollowRedirect() API to return bool

third_party/WebKit/public/platform/WebURLLoaderClient.h

Index: third_party/WebKit/public/platform/WebURLLoaderClient.h
diff --git a/third_party/WebKit/public/platform/WebURLLoaderClient.h b/third_party/WebKit/public/platform/WebURLLoaderClient.h
index 9f93cbcc6957dc7248204a69f181227ec86583e2..ea034c81acf59e6a21ebc49a0ef31b0c65210b6e 100644
--- a/third_party/WebKit/public/platform/WebURLLoaderClient.h
+++ b/third_party/WebKit/public/platform/WebURLLoaderClient.h
@@ -47,8 +47,13 @@ public:
     // generated by the redirect. The client may modify |newRequest|.
     // |encodedDataLength| is the size of the data that came from the network
     // for this redirect, or zero if the redirect was served from cache.
-    virtual void willFollowRedirect(
-        WebURLLoader*, WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t encodedDataLength) {}
+    //
+    // Implementations should return true to instruct the loader to follow the,
+    // redirect, or false otherwise.
+    virtual bool willFollowRedirect(
+        WebURLLoader*, WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t encodedDataLength) {

[jochen (gone - plz use gerrit), 2016/09/19 14:26:54]

should newRequest be const now?

[tyoshino (SeeGerritForStatus), 2016/09/26 14:48:41]

Please see this. I'd like to give it try later as there're some non-trivial
investigation needed to clean up remaining code paths which are mutating
newRequest.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...

[jochen (gone - plz use gerrit), 2016/09/27 19:17:27]

I'm worried that not doing this in one will put us in an inconsistent state. I'm
also not sure why this is non-trivial? Shouldn't there only be places that kill
the new request, and we'd replace them all with return false, and that's it?

If not, maybe we should first clean up all places where the resource request is
modified, so changing willFollowRedirect then is trivial?

[tyoshino (SeeGerritForStatus), 2016/09/29 13:40:22]

To support by loading the AssociatedURLLoader, the DocumentThreadableLoader
forwards willFollowRedirect event to m_client via the
DocumentThreadableLoaderClient interface. The AssociatedURLLoader takes
WebURLLoaderClient implementation as its client. So, ideally we should make
WebURLLoaderClient::willFollowRedirect() to accept "const WebURLLoader&". But
there're several places including the ResourceLoader where request is modified.
Some of them are meaningless and I'm trying to remove lines modifying
WebURLLoader.

But given your comment, I just more carefully investigated all places and
noticed that the ResourceLoader does need to modify it. So, changing all
WebURLLoaderClient::willFollowRedirect() impl to accept "const WebURLLoader&" is
bad idea.

I think we should have the AssociatedURLLoader stop inheriting the WebURLLoader.
This design has been causing several pains in refactoring the loader chain.
Does the AssociatedURLLoader/WebURLLoader separation make sense to you? If so,
do you suggest that I do it before landing this? I've checked that no one is
passing WebURLLoaderClient that modifies ResourceRequest to an
AssociatedURLLoader instance. So, we can land this as is (the latest patch set)
by saying thanks to the const_cast in WrappedResourceRequest constructor used in
DocumentThreadableLoaderClient.

[tyoshino (SeeGerritForStatus), 2016/09/29 13:41:17]

s/by loading/loading by/;

+        return true;
+    }
 
     // Called to report upload progress. The bytes reported correspond to
     // the HTTP message body.