Change WebURLLoaderClient::willFollowRedirect() API to return bool

Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

R=japhet@chromium.org,mkwst@chromium.org,bbudge@chromium.org,jochen@chromium.org,dalecurtis@chromium.org
BUG=636297
Committed: https://crrev.com/c42eabb6f25a2349f6c4d579f2ab4be55502302b
Cr-Commit-Position: refs/heads/master@{#423102}

[tyoshino (SeeGerritForStatus), 2016-08-10 09:46:31]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

BUG=none
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

BUG=636297
==========

[tyoshino (SeeGerritForStatus), 2016-08-15 12:23:26]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 12:23:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tyoshino (SeeGerritForStatus), 2016-08-15 12:23:52]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-15 12:25:54]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-15 12:25:56]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-15 12:26:01]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-15 12:26:08]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 12:26:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 12:34:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 12:34:52]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tyoshino (SeeGerritForStatus), 2016-08-16 05:11:41]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

BUG=636297
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL.

RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations.

BUG=636297
==========

[tyoshino (SeeGerritForStatus), 2016-08-16 05:20:38]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL.

RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations.

BUG=636297
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

BUG=636297
==========

[tyoshino (SeeGerritForStatus), 2016-08-16 05:20:48]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 05:21:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 05:39:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 05:39:48]

Dry run: Exceeded global retry quota

[tyoshino (SeeGerritForStatus), 2016-08-16 05:52:12]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 05:52:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 07:13:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 07:13:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tyoshino (SeeGerritForStatus), 2016-08-16 09:08:26]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 09:08:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tyoshino (SeeGerritForStatus), 2016-08-17 05:59:00]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

BUG=636297
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

BUG=636297
==========

[tyoshino (SeeGerritForStatus), 2016-08-17 05:59:00]

tyoshino@chromium.org changed reviewers:
+ japhet@chromium.org

[tyoshino (SeeGerritForStatus), 2016-09-06 07:15:29]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 07:15:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 09:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 09:10:22]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-09-06 10:20:47]

Friendly reminder :)

Rebased.

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

Sorry for the delay!

https://codereview.chromium.org/2230173002/diff/200001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2230173002/diff/200001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:185: url_data_->Fail();
I don't know this code at all, but it smells like it should return false
sometimes.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/RawResource.cpp:129: newRequest =
ResourceRequest();
Why is blanking the ResourceRequest needed?

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:138: return
newRequest.url() == originalURL;
I assume this is handling some old cases that still try to cancel the request by
blanking the url? Can we hunt those down and get rid of them?

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:144: didFail(nullptr,
ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
Is the didFail() still needed?

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: return true;
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]

https://codereview.chromium.org/2230173002/diff/200001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2230173002/diff/200001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:185: url_data_->Fail();
On 2016/09/06 16:46:14, Nate Chapin wrote:
> I don't know this code at all, but it smells like it should return false
> sometimes.

Yeah. I'll ask the owner for review.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/RawResource.cpp:129: newRequest =
ResourceRequest();
On 2016/09/06 16:46:14, Nate Chapin wrote:
> Why is blanking the ResourceRequest needed?

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

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/RawResource.cpp:136: newRequest =
ResourceRequest();
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".

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:138: return
newRequest.url() == originalURL;
On 2016/09/06 16:46:14, Nate Chapin wrote:
> I assume this is handling some old cases that still try to cancel the request
by
> blanking the url? Can we hunt those down and get rid of them?

Inside m_fetcher->willFollowRedirect(), we may reach
WebFrameTestClient::willSendRequest() and RenderFrameImpl::willSendRequest()
which may rewrite the URL. Yeah, this line is to capture them.

They're about extension URLs and layout test output directories.

Maybe the right thing to do is to honor these rewrites but currently it doesn't
get reflected to net stack. It looks the best thing we can do here is not to
follow redirect.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:144: didFail(nullptr,
ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
On 2016/09/06 16:46:14, Nate Chapin wrote:
> Is the didFail() still needed?

Hmm, ResourceDispatcher::Cancel() will be invoked to clear the corresponding
PendingRequest. But that's it. It's only a part of what happens on
m_loader.reset(). So, we should keep this?

Is there any other path to do what's done in didFail()?

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: return true;
On 2016/09/06 16:46:14, Nate Chapin wrote:
> If you return false here, you can probably remove the stopFetching() call and
> get the same result.

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.

[tyoshino (SeeGerritForStatus), 2016-09-07 10:52:36]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 10:52:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 12:31:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 12:31:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

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

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/RawResource.cpp:136: newRequest =
ResourceRequest();
On 2016/09/07 10:52:28, tyoshino wrote:
> 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".

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

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:144: didFail(nullptr,
ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
On 2016/09/07 10:52:28, tyoshino wrote:
> On 2016/09/06 16:46:14, Nate Chapin wrote:
> > Is the didFail() still needed?
> 
> Hmm, ResourceDispatcher::Cancel() will be invoked to clear the corresponding
> PendingRequest. But that's it. It's only a part of what happens on
> m_loader.reset(). So, we should keep this?
> 
> Is there any other path to do what's done in didFail()?

No. I guess ResourceDispatcher::Cancel() could trigger the didFail(), but that's
a bigger, less predictable change, so leaving the didFail() here is fine.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: return true;
On 2016/09/07 10:52:28, tyoshino wrote:
> On 2016/09/06 16:46:14, Nate Chapin wrote:
> > If you return false here, you can probably remove the stopFetching() call
and
> > get the same result.
> 
> 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.

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

https://codereview.chromium.org/2230173002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:136: if
(m_fetcher->willFollowRedirect(m_resource.get(), newRequest, redirectResponse,
encodedDataLength)) {
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]

I'm going home after checking CQ bit. Please take a look again if the bots are
green. Thanks!

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/RawResource.cpp:136: newRequest =
ResourceRequest();
On 2016/09/07 17:05:45, Nate Chapin wrote:
> On 2016/09/07 10:52:28, tyoshino wrote:
> > 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".
> 
> How much trouble would it be to change the behavior in this patch?

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.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:144: didFail(nullptr,
ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
On 2016/09/07 at 17:05:45, Nate Chapin wrote:
> No. I guess ResourceDispatcher::Cancel() could trigger the didFail(), but
that's a bigger, less predictable change, so leaving the didFail() here is fine.

OK. I'll try to do it later when I get chance.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: return true;
On 2016/09/07 17:05:45, Nate Chapin wrote:
> On 2016/09/07 10:52:28, tyoshino wrote:
> > On 2016/09/06 16:46:14, Nate Chapin wrote:
> > > If you return false here, you can probably remove the stopFetching() call
> and
> > > get the same result.
> > 
> > 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.
> 
> Hrm, maybe ResourceLoader::willFollowRedirect() should guarantee that it calls
> didFail() any time it returns false? Requiring ResourceClients to do that work
> seems unreliable.

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.?

https://codereview.chromium.org/2230173002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:136: if
(m_fetcher->willFollowRedirect(m_resource.get(), newRequest, redirectResponse,
encodedDataLength)) {
On 2016/09/07 17:05:45, Nate Chapin wrote:
> 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?

Nice suggestion. Reorganized.

> 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?

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.

[tyoshino (SeeGerritForStatus), 2016-09-14 14:53:55]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 14:54:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 16:25:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 16:25:40]

Dry run: This issue passed the CQ dry run.

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

LGTM except for the stopFetching() calls in DocumentLoader.cpp :)

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/RawResource.cpp:136: newRequest =
ResourceRequest();
On 2016/09/14 14:53:47, tyoshino wrote:
> On 2016/09/07 17:05:45, Nate Chapin wrote:
> > On 2016/09/07 10:52:28, tyoshino wrote:
> > > 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".
> > 
> > How much trouble would it be to change the behavior in this patch?
> 
> 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.

OK.

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: return true;
On 2016/09/14 14:53:47, tyoshino wrote:
> On 2016/09/07 17:05:45, Nate Chapin wrote:
> > On 2016/09/07 10:52:28, tyoshino wrote:
> > > On 2016/09/06 16:46:14, Nate Chapin wrote:
> > > > If you return false here, you can probably remove the stopFetching()
call
> > and
> > > > get the same result.
> > > 
> > > 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.
> > 
> > Hrm, maybe ResourceLoader::willFollowRedirect() should guarantee that it
calls
> > didFail() any time it returns false? Requiring ResourceClients to do that
work
> > seems unreliable.
> 
> 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.?

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.

https://codereview.chromium.org/2230173002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2230173002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1099: 
This file only has whitespace changes?

[tyoshino (SeeGerritForStatus), 2016-09-16 07:19:59]

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2230173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: return true;
On 2016/09/14 22:25:27, Nate Chapin wrote:
> On 2016/09/14 14:53:47, tyoshino wrote:
> > On 2016/09/07 17:05:45, Nate Chapin wrote:
> > > On 2016/09/07 10:52:28, tyoshino wrote:
> > > > On 2016/09/06 16:46:14, Nate Chapin wrote:
> > > > > If you return false here, you can probably remove the stopFetching()
> call
> > > and
> > > > > get the same result.
> > > > 
> > > > 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.
> > > 
> > > Hrm, maybe ResourceLoader::willFollowRedirect() should guarantee that it
> calls
> > > didFail() any time it returns false? Requiring ResourceClients to do that
> work
> > > seems unreliable.
> > 
> > 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.?
> 
> 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.

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

https://codereview.chromium.org/2230173002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2230173002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1099: 
On 2016/09/14 22:25:27, Nate Chapin wrote:
> This file only has whitespace changes?

Oh, right. Reverted.

[tyoshino (SeeGerritForStatus), 2016-09-16 07:28:09]

tyoshino@chromium.org changed reviewers:
+ bbudge@chromium.org, dalecurtis@chromium.org, jochen@chromium.org,
mkwst@chromium.org

[tyoshino (SeeGerritForStatus), 2016-09-16 07:28:10]

+dalecurtis for content/renderer/media/ and media/
+bbudge for content/renderer/pepper/
+mkwst for third_party/WebKit/Source/platform/
+jochen for content/child/

dalecurtis, could you please tell me what do you think about making
ResourceMultiBufferDataProvider::willFollowRedirect() return false for some
paths?

[tyoshino (SeeGerritForStatus), 2016-09-16 07:28:35]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 07:28:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-09-16 07:32:26]

mkwst@chromium.org changed required reviewers:
+ japhet@chromium.org

[Mike West, 2016-09-16 07:32:27]

//Source/platform LGTM as long as japhet@ is fine with the behavior change in
general.

[commit-bot: I haz the power, 2016-09-16 07:33:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 07:33:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tyoshino (SeeGerritForStatus), 2016-09-16 07:36:14]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 07:36:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 07:40:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 07:40:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tyoshino (SeeGerritForStatus), 2016-09-16 11:09:41]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 11:09:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 12:24:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 12:24:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tyoshino (SeeGerritForStatus), 2016-09-16 15:07:24]

tyoshino@chromium.org changed reviewers:
+ boliu@chromium.org

[tyoshino (SeeGerritForStatus), 2016-09-16 15:07:25]

+boliu

All tests except for the Android ones are passing.

boliu, from what I got by reading the code, loadUrlAsync() is connected to
content::NavigationControllerImpl, and therefore it would result in normal
navigation. Right? Is there anything special happening for Android WebView tests
(e.g. interception, rewriting, etc.) compared to other tests?

[boliu, 2016-09-16 17:45:41]

On 2016/09/16 15:07:25, tyoshino wrote:
> +boliu
> 
> All tests except for the Android ones are passing.
> 
> boliu, from what I got by reading the code, loadUrlAsync() is connected to
> content::NavigationControllerImpl, and therefore it would result in normal
> navigation. Right? Is there anything special happening for Android WebView
tests
> (e.g. interception, rewriting, etc.) compared to other tests?

Yeah that's just a normal navigation, more or less equivalent to user typing the
url in the omnibox in chrome.

The error is:
junit.framework.ComparisonFailure:
expected:<[http://localhost:46515/redirect_target.html]> but was:<[about:blank]>

So the load is supposed to redirect to redirect_target.html, but it appears that
what was reported by onPageFinished callback is about:blank.

[DaleCurtis, 2016-09-16 17:49:18]

dalecurtis@chromium.org changed reviewers:
+ hubbe@chromium.org

[DaleCurtis, 2016-09-16 17:49:20]

+hubbe for media/ multibuffer.

https://codereview.chromium.org/2230173002/diff/320001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:186: // "this" may be deleted
now.
I'd keep a return here or this code becomes brittle under any future additions.
I think it should return false? +hubbe

[hubbe1, 2016-09-16 17:52:24]

hubbe@google.com changed reviewers:
+ hubbe@google.com

[hubbe1, 2016-09-16 17:52:25]

https://codereview.chromium.org/2230173002/diff/320001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:186: // "this" may be deleted
now.
On 2016/09/16 17:49:19, DaleCurtis wrote:
> I'd keep a return here or this code becomes brittle under any future
additions.
> I think it should return false? +hubbe

Agreed, return false would be best here.

[bbudge, 2016-09-16 18:13:04]

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/pepper_url_loader_host.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
content/renderer/pepper/pepper_url_loader_host.cc:131: SetDefersLoading(true);
Should you return false here? Perhaps comment the 'return true' line to explain
why if not.

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

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoaderClient.h (right):

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoaderClient.h:54: WebURLLoader*,
WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t
encodedDataLength) {
should newRequest be const now?

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

Thanks boliu for the advice.

I'm still investigating the cause of the failures.

At this point, I've noticed only that reverting removal of stopFetching() call
and insertion of didFail() makes the Android bots happy.

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/pepper_url_loader_host.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
content/renderer/pepper/pepper_url_loader_host.cc:131: SetDefersLoading(true);
On 2016/09/16 18:13:03, bbudge wrote:
> Should you return false here? Perhaps comment the 'return true' line to
explain
> why if not.

This code originates from this patch you authored.

https://chromium.googlesource.com/chromium/src/+/71e5874e3bea5a8adb67310e717b...

According to the comment of the patch and the comment on
PpapiHostMsg_URLLoader_SetDeferLoading IPC definition in current repo, it looks
this is for allowing the plugin to audit redirect. So, here we should only defer
the redirect following.

Added a comment.

https://codereview.chromium.org/2230173002/diff/320001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:186: // "this" may be deleted
now.
On 2016/09/16 17:52:25, hubbe1 wrote:
> On 2016/09/16 17:49:19, DaleCurtis wrote:
> > I'd keep a return here or this code becomes brittle under any future
> additions.
> > I think it should return false? +hubbe
> 
> Agreed, return false would be best here.

OK. Changed.

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoaderClient.h (right):

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoaderClient.h:54: WebURLLoader*,
WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t
encodedDataLength) {
On 2016/09/19 14:26:54, jochen (slow) wrote:
> should newRequest be const now?

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...

[bbudge, 2016-09-26 15:46:31]

LGTM

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/pepper_url_loader_host.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
content/renderer/pepper/pepper_url_loader_host.cc:131: SetDefersLoading(true);
On 2016/09/26 14:48:41, tyoshino wrote:
> On 2016/09/16 18:13:03, bbudge wrote:
> > Should you return false here? Perhaps comment the 'return true' line to
> explain
> > why if not.
> 
> This code originates from this patch you authored.
> 
>
https://chromium.googlesource.com/chromium/src/+/71e5874e3bea5a8adb67310e717b...
> 
> According to the comment of the patch and the comment on
> PpapiHostMsg_URLLoader_SetDeferLoading IPC definition in current repo, it
looks
> this is for allowing the plugin to audit redirect. So, here we should only
defer
> the redirect following.
> 
> Added a comment.

The linked patch is by brettw. But I agree with you, 'true' is appropriate here.
Thanks for adding a comment.

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

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoaderClient.h (right):

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoaderClient.h:54: WebURLLoader*,
WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t
encodedDataLength) {
On 2016/09/26 at 14:48:41, tyoshino wrote:
> On 2016/09/19 14:26:54, jochen (slow) wrote:
> > should newRequest be const now?
> 
> 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...

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]

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/pepper_url_loader_host.cc (right):

https://codereview.chromium.org/2230173002/diff/320001/content/renderer/peppe...
content/renderer/pepper/pepper_url_loader_host.cc:131: SetDefersLoading(true);
On 2016/09/26 15:46:31, bbudge wrote:
> On 2016/09/26 14:48:41, tyoshino wrote:
> > On 2016/09/16 18:13:03, bbudge wrote:
> > > Should you return false here? Perhaps comment the 'return true' line to
> > explain
> > > why if not.
> > 
> > This code originates from this patch you authored.
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/71e5874e3bea5a8adb67310e717b...
> > 
> > According to the comment of the patch and the comment on
> > PpapiHostMsg_URLLoader_SetDeferLoading IPC definition in current repo, it
> looks
> > this is for allowing the plugin to audit redirect. So, here we should only
> defer
> > the redirect following.
> > 
> > Added a comment.
> 
> The linked patch is by brettw. But I agree with you, 'true' is appropriate
here.

Oops!

> Thanks for adding a comment.

Thanks for review!

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoaderClient.h (right):

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoaderClient.h:54: WebURLLoader*,
WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t
encodedDataLength) {
On 2016/09/27 19:17:27, jochen (slow) wrote:
> On 2016/09/26 at 14:48:41, tyoshino wrote:
> > On 2016/09/19 14:26:54, jochen (slow) wrote:
> > > should newRequest be const now?
> > 
> > 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...
> 
> 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?

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.

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

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]

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoaderClient.h (right):

https://codereview.chromium.org/2230173002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoaderClient.h:54: WebURLLoader*,
WebURLRequest& newRequest, const WebURLResponse& redirectResponse, int64_t
encodedDataLength) {
On 2016/09/29 13:40:22, tyoshino wrote:
> To support by loading the AssociatedURLLoader, the DocumentThreadableLoader

s/by loading/loading by/;

[tyoshino (SeeGerritForStatus), 2016-09-29 13:41:30]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 13:41:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tyoshino (SeeGerritForStatus), 2016-09-29 13:49:48]

On 2016/09/26 14:48:41, tyoshino wrote:
> I'm still investigating the cause of the failures.
> 
> At this point, I've noticed only that reverting removal of stopFetching() call
> and insertion of didFail() makes the Android bots happy.

Oh, sorry. I haven't finished this investigation.

Insertion of didFail()s is fine. Just removal of stopFetching() is bad.

[commit-bot: I haz the power, 2016-09-29 15:15:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 15:15:43]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-30 11:11:24]

oh well, lgtm with some comments.

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (left):

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
content/child/web_url_loader_impl.cc:648: if (redirect_info.new_url ==
GURL(new_request.url())) {
can we DCHECK() that redirect_info.new_url == new_request.url() at this point?

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
content/child/web_url_loader_impl.cc:645: if (follow) {
maybe just early exist if !follow here?

https://codereview.chromium.org/2230173002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc (right):

https://codereview.chromium.org/2230173002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc:88: newRequest
= WebURLRequest();
why this?

[DaleCurtis, 2016-09-30 16:42:51]

media/ lgtm

[tyoshino (SeeGerritForStatus), 2016-10-03 11:31:13]

The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 11:31:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 12:45:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 12:45:45]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-10-04 04:02:25]

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (left):

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
content/child/web_url_loader_impl.cc:648: if (redirect_info.new_url ==
GURL(new_request.url())) {
On 2016/09/30 11:11:24, jochen (slow) wrote:
> can we DCHECK() that redirect_info.new_url == new_request.url() at this point?

Added

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2230173002/diff/500001/content/child/web_url_...
content/child/web_url_loader_impl.cc:645: if (follow) {
On 2016/09/30 11:11:24, jochen (slow) wrote:
> maybe just early exist if !follow here?

Yeah. Done

https://codereview.chromium.org/2230173002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc (right):

https://codereview.chromium.org/2230173002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc:88: newRequest
= WebURLRequest();
On 2016/09/30 11:11:24, jochen (slow) wrote:
> why this?

Just preserving the old behavior. Before this change, newRequest was guaranteed
to be cleared when not following.

[tyoshino (SeeGerritForStatus), 2016-10-04 04:10:55]

japhet@, could you please take another look at the changes that:
- changed DocumentThreadableLoader::redirectReceived() to return false for cross
origin successful case (the last line of the method)
- reverted removal of m_fetcher->stopFetching() from
DocumentLoader::redirectReceived() as it makes the Android WebView test fail
- changed RawResourceClient::redirectReceived() to accept a new ResourceRequest
object as const reference in response to jochen@'s comment which you originally
proposed
- change to TextTrackLoader which you've recently introduced

[Nate Chapin, 2016-10-04 20:21:14]

lgtm

[tyoshino (SeeGerritForStatus), 2016-10-05 03:38:27]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

BUG=636297
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

R=japhet@chromium.org,mkwst@chromium.org,bbudge@chromium.org,jochen@chromium....
BUG=636297
==========

[tyoshino (SeeGerritForStatus), 2016-10-05 03:39:32]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-10-05 03:39:32]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
bbudge@chromium.org, jochen@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2230173002/#ps540001
(title: "Addressed #81")

[commit-bot: I haz the power, 2016-10-05 03:39:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 03:42:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 03:42:24]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tyoshino (SeeGerritForStatus), 2016-10-05 06:22:06]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-10-05 06:22:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jochen@chromium.org, mkwst@chromium.org, dalecurtis@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2230173002/#ps560001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-05 06:22:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 07:46:14]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

R=japhet@chromium.org,mkwst@chromium.org,bbudge@chromium.org,jochen@chromium....
BUG=636297
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

R=japhet@chromium.org,mkwst@chromium.org,bbudge@chromium.org,jochen@chromium....
BUG=636297
==========

[commit-bot: I haz the power, 2016-10-05 07:46:16]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2016-10-05 07:47:46]

Description was changed from

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

R=japhet@chromium.org,mkwst@chromium.org,bbudge@chromium.org,jochen@chromium....
BUG=636297
==========

to

==========
Change WebURLLoaderClient::willFollowRedirect() API to return bool

WebURLLoaderImpl has been requiring clients to clear the newRequest
argument when they don't want the redirect to be followed. This is
not intuitive and required strange-looking code snippet to reset the
argument.

The method should just return a bool to indicate whether or not to
follow the redirect.

Changed ResourceLoader::willFollowRedirect() to return false when
m_fetcher->willFollowRedirect() returns false. As didFail() clears
m_loader, it should be safe and it's more natural that the case
returns false.

ResourceFetcher::willFollowRedirect() may change the URL of newRequest
and return true because FrameFetchContext::dispatchWillSendRequest()
calls prepareRequest() which calls
FrameLoaderClient::dispatchWillSendRequest() which calls
WebFrameClient::willSendRequest() whose implementations may change the
URL. RenderFrameImpl::willSendRequest() and
WebFrameTestClient::willSendRequest() are the implementations. This CL
leaves the interface unchanged though it should also be fixed.

R=japhet@chromium.org,mkwst@chromium.org,bbudge@chromium.org,jochen@chromium....
BUG=636297

Committed: https://crrev.com/c42eabb6f25a2349f6c4d579f2ab4be55502302b
Cr-Commit-Position: refs/heads/master@{#423102}
==========

[commit-bot: I haz the power, 2016-10-05 07:47:49]

Patchset 29 (id:??) landed as
https://crrev.com/c42eabb6f25a2349f6c4d579f2ab4be55502302b
Cr-Commit-Position: refs/heads/master@{#423102}