Add support to URLRequest for deferring redirects....

Add support to URLRequest for deferring redirects.

I chose to add an out parameter to OnReceivedRedirect because it allows for the
default behavior to remain the same.

I considered adding a ContinueAfterRedirect method that all OnReceivedRedirect
implementations would need to call, but this caused one annoying problem:  In
the case of a ChromePlugin, it is possible for the URLRequest to get deleted
inside the handler for the redirect.  This would make it hard to subsequently
call a method on the URLRequest since I would need to have a way to determine
if the URLRequest had been deleted.

TEST=covered by unit tests
BUG=16413, 6442
R=eroman,wtc


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21417

[darin (slow to review), 2009-07-22 20:49:59]

sending to eroman for review since wtc is away today...

[eroman, 2009-07-22 21:42:22]

lgtm

<side_comment>
There might be other bugs IDs linked to this. I remember safe-browsing had a
use-case for wanting to defer redirects (http://crbug.com/6442), but I can't
find out what their resolution was...
<side_comment>

http://codereview.chromium.org/155897/diff/22/27
File net/url_request/url_request.h (right):

http://codereview.chromium.org/155897/diff/22/27#newcode138
Line 138: // If the request is not canceled and |*defer_redirect| is false, then
the
Can you mention that writing to *defer_redirect is optional, since it defaults
to some value (false)?

(I think I would have preferred using a return value (enum) to force
implementations to define their behavior, but this is fine too.)

http://codereview.chromium.org/155897/diff/22/26
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/155897/diff/22/26#newcode303
Line 303: DCHECK(location.is_valid());
Can you DCHECK that http_status_code != -1 as well?

[wtc, 2009-07-22 21:53:42]

I found issue 11256 and issue 5131 on the issue of
safebrowsing and redirect.  Paul G. asked Eric and
me about this issue by email, which he mentioned in
issue 11256 comment 2.

I am reviewing this CL now...

[wtc, 2009-07-22 22:44:01]

LGTM, but I have some nits and minor questions below.

My comments on two issues may seem confusing.

1. I didn't know why you provided a no-op default
implementation for OnReceivedRedirect at first, so
some of my comments reflected that.  Later I knew
why.

2. I didn't know you initialize defer_redirect to
false before calling OnReceivedRedirect, so there
are many comments that ask you to set *defer_redirect
to some value.  If you don't want to make that change,
please clarify this issue in the block comment before
OnReceivedRedirect in url_request.h.

Also, the only Delegate in this CL that actually sets
*defer_redirect to trus is TestDelegate.  Does this
mean there will be a follow-up CL that actually uses
this new feature?

http://codereview.chromium.org/155897/diff/22/32
File chrome/browser/chrome_plugin_host.cc (right):

http://codereview.chromium.org/155897/diff/22/32#newcode193
Line 193: bool* defer_redirect) {
Set *defer_redirect to false.

http://codereview.chromium.org/155897/diff/22/36
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/155897/diff/22/36#newcode84
Line 84: virtual void OnReceivedRedirect(URLRequest* request,
You can remove this method to rely on the default
implementation.

http://codereview.chromium.org/155897/diff/22/34
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/155897/diff/22/34#newcode890
Line 890: bool* defer_redirect) {
Set *defer_redirect to false.

http://codereview.chromium.org/155897/diff/22/25
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/155897/diff/22/25#newcode70
Line 70: virtual void OnReceivedRedirect(URLRequest* request, const GURL&
to_url,
You can remove this method to rely on the default implementation.

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/155897/diff/22/30#oldcode423
Line 423: status_ = URLRequestStatus();
It's not clear why you moved this line.  It seems to have
no effect.  Is it because it's more clear if we reset
status_ and is_pending_ before calling Start()?

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/155897/diff/22/30#newcode274
Line 274: job_->Kill();
Is this change related to this CL?

Why not just add
  job_->Kill();
to OrphanJob()?

http://codereview.chromium.org/155897/diff/22/30#newcode340
Line 340: void URLRequest::ReceivedRedirect(const GURL& location, bool*
defer_redirect) {
Should we first set *defer_redirect to some value to
ensure that this function always sets *defer_redirect?
Right now it only sets *defer_redirect if it calls
delegate_->OnReceivedRedirect().

http://codereview.chromium.org/155897/diff/22/27
File net/url_request/url_request.h (left):

http://codereview.chromium.org/155897/diff/22/27#oldcode141
Line 141: const GURL& new_url) = 0;
Why do you change this "= 0" to a default implementation
that does nothing?

If you made this change deliberately, the default
implementation should at least sets *defer_redirect
to a value (false?).

http://codereview.chromium.org/155897/diff/22/27
File net/url_request/url_request.h (right):

http://codereview.chromium.org/155897/diff/22/27#newcode138
Line 138: // If the request is not canceled and |*defer_redirect| is false, then
the
I suggest that instead of
  If the request is not canceled
you say
  If the delegate does not cancel the request
to make it clear who may cancel the request.

http://codereview.chromium.org/155897/diff/22/26
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/155897/diff/22/26#newcode306
Line 306: 
So you removed the Kill() call that used to be here... Why?

http://codereview.chromium.org/155897/diff/22/26#newcode357
Line 357: bool defer_redirect = false;
So you initialize defer_redirect to false here.

I think it's more clear to let ReceivedRedirect always
set defer_redirect on return.

http://codereview.chromium.org/155897/diff/22/26#newcode358
Line 358: request_->ReceivedRedirect(new_location, &defer_redirect);
Are you sure we don't need to set has_handled_response_
to true when the request is canceled or when
request_->status().is_success() is false?

http://codereview.chromium.org/155897/diff/22/28
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/155897/diff/22/28#newcode26
Line 26: class GURL;
Remove this forward declaration since we include the gurl.h
header now.

http://codereview.chromium.org/155897/diff/22/28#newcode268
Line 268: void FollowRedirect(const GURL& location, int http_status_code);
Does this have to be a protected method?  Just curious.

http://codereview.chromium.org/155897/diff/22/28#newcode354
Line 354: // Set when a redirect is pending.
Should we change "pending" to "deferred"?

http://codereview.chromium.org/155897/diff/22/28#newcode356
Line 356: int deferred_redirect_code_;
Change "code" to "status_code"?  It'd be more clear.

http://codereview.chromium.org/155897/diff/22/29
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/155897/diff/22/29#newcode1299
Line 1299: // ("Content-Length: 10" is just a junk value to make sure it gets
stripped).
This stale comment should be updated or removed.  (I don't
know why it says Content-Length will get stripped.)

http://codereview.chromium.org/155897/diff/22/23
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/155897/diff/22/23#newcode285
Line 285: DCHECK(request->status().is_success());
Set *defer_redirect to false.

[darin (slow to review), 2009-07-22 23:35:34]

Thanks for the great feedback!!

> 1. I didn't know why you provided a no-op default
> implementation for OnReceivedRedirect at first, so
> some of my comments reflected that.  Later I knew
> why.
> 
> 2. I didn't know you initialize defer_redirect to
> false before calling OnReceivedRedirect, so there
> are many comments that ask you to set *defer_redirect
> to some value.  If you don't want to make that change,
> please clarify this issue in the block comment before
> OnReceivedRedirect in url_request.h.

I wrote a comment in url_request.h.  I went back-n-forth
on different interfaces for this feature.  In the end,
I was happiest with the pre-initialized out param.  I
think returning an enum value could also work well.  If
you guys have strong opinions about this, please let me
know.  I'm happy to oblige.


> Also, the only Delegate in this CL that actually sets
> *defer_redirect to trus is TestDelegate.  Does this
> mean there will be a follow-up CL that actually uses
> this new feature?

Yes, part 2 will involve Chrome changes to make use of
deferred redirects.

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/155897/diff/22/30#oldcode423
Line 423: status_ = URLRequestStatus();
I moved that line because it needs to happen after the call to Kill, which has
the side-effect of setting status_ to an error value.  Previously, the caller of
URLRequest::Redirect would take care of calling Kill, but I found it cleaner to
make URLRequest::Redirect responsible for doing so.

http://codereview.chromium.org/155897/diff/22/27
File net/url_request/url_request.h (left):

http://codereview.chromium.org/155897/diff/22/27#oldcode141
Line 141: const GURL& new_url) = 0;
I think a default implementation makes sense for the same reason that a default
implementation makes sense for OnAuthRequired.  Many consumers (as I discovered
in this CL) do not really need to override OnReceivedRedirect.

http://codereview.chromium.org/155897/diff/22/26
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/155897/diff/22/26#newcode303
Line 303: DCHECK(location.is_valid());
Yes, done.

http://codereview.chromium.org/155897/diff/22/26#newcode306
Line 306: 
See my comment about moving it to URLRequest::Redirect.

http://codereview.chromium.org/155897/diff/22/26#newcode358
Line 358: request_->ReceivedRedirect(new_location, &defer_redirect);
On 2009/07/22 22:44:01, wtc wrote:
> Are you sure we don't need to set has_handled_response_
> to true when the request is canceled or when
> request_->status().is_success() is false?

I'm pretty sure since has_handled_response_ only impacts whether or not
NotifyHeadersComplete does work, and in the cancelation cases, I don't expect
NotifyHeadersComplete to be called again.

http://codereview.chromium.org/155897/diff/22/28
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/155897/diff/22/28#newcode26
Line 26: class GURL;
On 2009/07/22 22:44:01, wtc wrote:
> Remove this forward declaration since we include the gurl.h
> header now.

Done.

http://codereview.chromium.org/155897/diff/22/28#newcode268
Line 268: void FollowRedirect(const GURL& location, int http_status_code);
On 2009/07/22 22:44:01, wtc wrote:
> Does this have to be a protected method?  Just curious.

I think it can be made private.  Done.

http://codereview.chromium.org/155897/diff/22/28#newcode354
Line 354: // Set when a redirect is pending.
On 2009/07/22 22:44:01, wtc wrote:
> Should we change "pending" to "deferred"?

Yes, that is a typo.  I was previously using the term "pending" before switching
to "deferred", which is hopefully a clearer term for this.

http://codereview.chromium.org/155897/diff/22/28#newcode356
Line 356: int deferred_redirect_code_;
On 2009/07/22 22:44:01, wtc wrote:
> Change "code" to "status_code"?  It'd be more clear.

Done.

http://codereview.chromium.org/155897/diff/22/29
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/155897/diff/22/29#newcode1299
Line 1299: // ("Content-Length: 10" is just a junk value to make sure it gets
stripped).
On 2009/07/22 22:44:01, wtc wrote:
> This stale comment should be updated or removed.

Done.


>  (I don't
> know why it says Content-Length will get stripped.)

When we handle a 302 redirect (actually, any non-307 redirect), we strip various
request headers if the request method was POST.  Content-Length is included in
that set.

[wtc, 2009-07-23 00:30:11]

LGTM.

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/155897/diff/22/30#oldcode423
Line 423: status_ = URLRequestStatus();
On 2009/07/22 23:35:34, darin wrote:
>
> Previously, the caller of
> URLRequest::Redirect would take care of calling Kill, but I found it cleaner
to
> make URLRequest::Redirect responsible for doing so.

OK, so this explains the new job_->Kill() call in
URLRequest::Redirect().  But why did you also add
a job_->Kill() call to URLRequest::RestartWithJob()?

http://codereview.chromium.org/155897/diff/64/1093
File net/url_request/url_request.h (right):

http://codereview.chromium.org/155897/diff/64/1093#newcode144
Line 144: // The default value for |*defer_redirect| is false, so that consumers
do
I suggest rewriting this sentence as follows:

  The caller must set |*defer_redirect| to false, so that
  delegates do not need to set it if they are happy with the
  default behavior of not deferring redirect.

I also considered suggesting that this method returns
bool to indicate whether the redirect is deferred, but
I see that all the Delegate methods return void.  If
it's okay for a Delegate method to return non-void,
I think returning bool or an enum would be more clear.

http://codereview.chromium.org/155897/diff/64/1094
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/155897/diff/64/1094#newcode322
Line 322: void FollowRedirect(const GURL& location, int http_status_code);
Nit: move this method back to its original location in this
header and in the .cc file to minimize the diffs.

[darin (slow to review), 2009-07-23 01:27:29]

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/155897/diff/22/30#oldcode423
Line 423: status_ = URLRequestStatus();
> OK, so this explains the new job_->Kill() call in
> URLRequest::Redirect().  But why did you also add
> a job_->Kill() call to URLRequest::RestartWithJob()?

Sorry, I meant to explain that.  Since I removed the call to Kill in
URLRequestJob::FollowRedirect, it is necessary for it to be called in
RestartWithJob.  Otherwise, the job we are redirecting away from will not be
killed.

http://codereview.chromium.org/155897/diff/64/1093
File net/url_request/url_request.h (right):

http://codereview.chromium.org/155897/diff/64/1093#newcode144
Line 144: // The default value for |*defer_redirect| is false, so that consumers
do
On 2009/07/23 00:30:12, wtc wrote:
> I suggest rewriting this sentence as follows:
> 
>   The caller must set |*defer_redirect| to false, so that
>   delegates do not need to set it if they are happy with the
>   default behavior of not deferring redirect.

Done.


> I also considered suggesting that this method returns
> bool to indicate whether the redirect is deferred, but
> I see that all the Delegate methods return void.  If
> it's okay for a Delegate method to return non-void,
> I think returning bool or an enum would be more clear.

I originally considered returning a bool, but I decided against it since it was
not so clear if returning false meant canceling the redirect or deferring the
redirect.  An enum would also make that clear, but an out parameter also makes
things more clear.

I'll think on the enum idea a bit more.

http://codereview.chromium.org/155897/diff/64/1094
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/155897/diff/64/1094#newcode322
Line 322: void FollowRedirect(const GURL& location, int http_status_code);
On 2009/07/23 00:30:12, wtc wrote:
> Nit: move this method back to its original location in this
> header and in the .cc file to minimize the diffs.

Done.

[wtc, 2009-07-23 02:37:47]

http://codereview.chromium.org/155897/diff/64/1093
File net/url_request/url_request.h (right):

http://codereview.chromium.org/155897/diff/64/1093#newcode144
Line 144: // The default value for |*defer_redirect| is false, so that consumers
do
On 2009/07/23 01:27:29, darin wrote:
>
> I originally considered returning a bool, but I decided against it since it
was
> not so clear if returning false meant canceling the redirect or deferring the
> redirect.

I agree.

[wtc, 2009-07-23 02:47:42]

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/155897/diff/22/30#oldcode423
Line 423: status_ = URLRequestStatus();
On 2009/07/23 01:27:29, darin wrote:
>
> Since I removed the call to Kill in
> URLRequestJob::FollowRedirect, it is necessary for it to be called in
> RestartWithJob.  Otherwise, the job we are redirecting away from will not be
> killed.

But you already added a job_->Kill() call to
Redirect (on line 449), which replaces the Kill() call
in URLRequestJob::FollowRedirect that you removed.

So I still don't understand what RestartWithJob has to
do with redirect...

[darin (slow to review), 2009-07-23 05:26:27]

http://codereview.chromium.org/155897/diff/22/30
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/155897/diff/22/30#oldcode423
Line 423: status_ = URLRequestStatus();
> But you already added a job_->Kill() call to
> Redirect (on line 449), which replaces the Kill() call
> in URLRequestJob::FollowRedirect that you removed.
> 
> So I still don't understand what RestartWithJob has to
> do with redirect...

The problem is that RestartWithJob calls OrphanJob, so the original
job no longer has a valid request_ on which to call Redirect.  I
needed to call Kill before OrphanJob so that the original job does
not call FollowRedirect.

I think this is cleaner than having the original job follow the
redirect, and then get killed.  The killing used to happen in
FollowRedirect, and then there was an early return if request_
was null.  That just seemed more convoluted to me.  By having
RestartWithJob call Kill, it is more inline with the way that
consumers may call Cancel in response to OnReceivedRedirect.

[darin (slow to review), 2009-07-23 16:50:18]

On the subject of returning an enum, I think I prefer the out parameter because
it makes the effect more optional.  I struggle to come up with a good name for
the default action since the default action is either to follow the redirect or
to do nothing if the request was cancelled.  If I were to name the enums
FOLLOW_REDIRECT and DEFER_REDIRECT, then the FOLLOW_REDIRECT value would be a
bit misleading.  Imagine code like this:

  RedirectPolicy Foo::OnReceivedRedirect(URLRequest* request,...) {
    request->Cancel();
    return FOLLOW_REDIRECT;
  }

I think that reads poorly since clearly the redirect won't be followed since the
request was cancelled.  Alternatively, we could use an enum value like
DEFAULT_ACTION, but that doesn't seem very nice either.  The point of using
enums is to be more descriptive about what the choices are to the consumer.

For these reasons, I think the optional out parameter is nicer.  The default
action is clearly expressed by just doing nothing, and the consumer can opt-in
to having the normal redirect processing deferred.

Here's how the two use cases read:

  void Foo::OnReceivedRedirect(URLRequest* request, bool* defer) {
    request->Cancel();
  }

  void Foo::OnReceivedRedirect(URLRequest* request, bool* defer) {
    *defer = true;
  }

[eroman, 2009-07-23 17:55:51]

lgtm