Change WebURLLoaderClient::willFollowRedirect() API to return bool

third_party/WebKit/public/platform/WebURLLoaderClient.h

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 29 matching lines @@
 class WebURLRequest;
 class WebURLResponse;
 struct WebURLError;
 
 class BLINK_PLATFORM_EXPORT WebURLLoaderClient {
 public:
     // Called when following a redirect. |newRequest| contains the request
     // 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.
     virtual void didSendData(
         WebURLLoader*, unsigned long long bytesSent, unsigned long long totalBytesToBeSent) { }
 
     // Called when response headers are received.
     virtual void didReceiveResponse(WebURLLoader*, const WebURLResponse&) { }
 
     // Called when response headers are received.
@@ skipping 33 matching lines @@
     // Value passed to didFinishLoading when total encoded data length isn't known.
     static const int64_t kUnknownEncodedDataLength = -1;
 
 protected:
     virtual ~WebURLLoaderClient() { }
 };
 
 } // namespace blink
 
 #endif