Change WebURLLoaderClient::willFollowRedirect() API to return bool

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

 /*
  * Copyright (C) 2011 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:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 104 matching lines @@
         client->responseReceived(this, response(), nullptr);
     if (!hasClient(c))
         return;
     if (data())
         client->dataReceived(this, data()->data(), data()->size());
     if (!hasClient(c))
         return;
     Resource::didAddClient(client);
 }
 
-void RawResource::willFollowRedirect(ResourceRequest& newRequest, const ResourceResponse& redirectResponse)
+bool RawResource::willFollowRedirect(ResourceRequest& newRequest, const ResourceResponse& redirectResponse)
 {
-    Resource::willFollowRedirect(newRequest, redirectResponse);
+    bool follow = Resource::willFollowRedirect(newRequest, redirectResponse);
+    if (!follow)
+        newRequest = ResourceRequest();

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

Why is blanking the ResourceRequest needed?

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

Ah, yeah. It's unnecessary. I just tried to make things consistent. Changed to
DCHECK() on the result of Resource::willFollowRedirect().

 
     ASSERT(!redirectResponse.isNull());
     ResourceClientWalker<RawResourceClient> w(clients());
-    while (RawResourceClient* c = w.next())
-        c->redirectReceived(this, newRequest, redirectResponse);
+    while (RawResourceClient* c = w.next()) {
+        if (!c->redirectReceived(this, newRequest, redirectResponse)) {
+            follow = false;
+            newRequest = ResourceRequest();

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

This is to keep the clients see the same (possibly already nullified) request as
before.

This logic should be fixed later to consult all the clients without changing
newRequest and return false only when none of the clients said "proceed".

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

How much trouble would it be to change the behavior in this patch?

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

We need some clean
- one is https://codereview.chromium.org/2336273003/.
- another is ... DocumentThreadableLoader is used by AssociatedURLLoader, and
therefore making redirectReceived()'s newRequest argument const needs making
DocumentThreadableLoaderClient::willFollowRedirect()'s newRequest argument
const, and it needs making WebURLLoaderClient::willFollowRedirect() const.
CrossOriginAccessControl::handleRedirect() still mutates newRequest. So, we need
to change this first.

As I have to these non-trivial work, I preferred to do them later.

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

OK.

+        }
+    }
+
+    return follow;
 }
 
 void RawResource::willNotFollowRedirect()
 {
     ResourceClientWalker<RawResourceClient> w(clients());
     while (RawResourceClient* c = w.next())
         c->redirectBlocked();
 }
 
 void RawResource::responseReceived(const ResourceResponse& response, std::unique_ptr<WebDataConsumerHandle> handle)
@@ skipping 182 matching lines @@
 
 void RawResourceClientStateChecker::notifyFinished(Resource* resource)
 {
     SECURITY_CHECK(m_state != NotAddedAsClient);
     SECURITY_CHECK(m_state != NotifyFinished);
     SECURITY_CHECK(resource->errorOccurred() || (m_state == ResponseReceived || m_state == SetSerializedCachedMetadata || m_state == DataReceived || m_state == DataDownloaded));
     m_state = NotifyFinished;
 }
 
 } // namespace blink