Speculative resource prefetching - fixing missing navigations due to server side redirects.

Speculative resource prefetching - fixing missing navigations due to server side redirects.

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

[Shishir, 2012-07-31 19:40:15]

Please take a look:

willchan for chrome/browser/net/...

[dominich, 2012-07-31 19:49:36]

just one nittish comment

http://codereview.chromium.org/10827103/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:362: if
(!response.redirect_url.is_valid())
This should probably be is_empty(), and you probably want to check is_valid()
when observing the redirect.

[Shishir, 2012-07-31 19:59:44]

http://codereview.chromium.org/10827103/diff/1/chrome/browser/predictors/reso...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/1/chrome/browser/predictors/reso...
chrome/browser/predictors/resource_prefetch_predictor.cc:362: if
(!response.redirect_url.is_valid())
On 2012/07/31 19:49:36, dominich wrote:
> This should probably be is_empty(), and you probably want to check is_valid()
> when observing the redirect.

Done.

[dominich, 2012-07-31 20:04:06]

LGTM

http://codereview.chromium.org/10827103/diff/5002/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10827103/diff/5002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:102: GURL redirect_url; 
// Only when the request was redirected.
nit: 'Empty unless the request was redirected.'

[Shishir, 2012-07-31 20:06:13]

http://codereview.chromium.org/10827103/diff/5002/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

http://codereview.chromium.org/10827103/diff/5002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:102: GURL redirect_url; 
// Only when the request was redirected.
On 2012/07/31 20:04:06, dominich wrote:
> nit: 'Empty unless the request was redirected.'

Done.

[willchan no longer on Chromium, 2012-08-01 01:50:24]

I defer to Matt since I'm at IETF and then traveling on vacation, so I
don't have time to review this.


On Tue, Jul 31, 2012 at 1:06 PM, <shishir@chromium.org> wrote:

>
> http://codereview.chromium.**org/10827103/diff/5002/chrome/**
>
browser/predictors/resource_**prefetch_predictor.h<http://codereview.chromium.org/10827103/diff/5002/chrome/browser/predictors/resource_prefetch_predictor.h>
> File chrome/browser/predictors/**resource_prefetch_predictor.h (right):
>
> http://codereview.chromium.**org/10827103/diff/5002/chrome/**
>
browser/predictors/resource_**prefetch_predictor.h#**newcode102<http://codereview.chromium.org/10827103/diff/5002/chrome/browser/predictors/resource_prefetch_predictor.h#newcode102>
> chrome/browser/predictors/**resource_prefetch_predictor.h:**102: GURL
> redirect_url;  // Only when the request was redirected.
> On 2012/07/31 20:04:06, dominich wrote:
>
>> nit: 'Empty unless the request was redirected.'
>>
>
> Done.
>
>
http://codereview.chromium.**org/10827103/<http://codereview.chromium.org/108...
>

[mmenke, 2012-08-01 10:25:37]

Could you please file a bug and link to it?  I have no idea what a "missing
navigation" is.

I'll need to read over the prefetch predictor, but I'll try and get back to you
today.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:361: // redirect url as
a new navigation.
This comment is phrased/placed really oddly, since the code it's just above
exits the function, conditioned on an if statement the comment doesn't mention
at all.  Maybe just add a blank line, to it's clearer it applies to more than
just the code immediately following it?  Or move it below the if statement.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:362: if
(response.redirect_url.is_empty())
When does this happen?  Think a comment would be reasonable.

[mmenke, 2012-08-01 15:39:53]

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:45: if (headers) {
Rather than muck with the full set of headers, can't we just have
ChromeResourceDispatcherHostDelegate::OnRequestRedirected pass along
response->head.final_url?  Not sure if we care about HSTS, but I'm not sure if
net::URLRequestRedirectJob generates any fake response headers.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:92:
redirect_url(other.redirect_url) {
Completely unrelated, but while you're here, mind fixing the indentation here? 
Thanks.

[mmenke, 2012-08-01 15:59:00]

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:45: if (headers) {
On 2012/08/01 15:39:53, Matt Menke wrote:
> Rather than muck with the full set of headers, can't we just have
> ChromeResourceDispatcherHostDelegate::OnRequestRedirected pass along
> response->head.final_url?  Not sure if we care about HSTS, but I'm not sure if
> net::URLRequestRedirectJob generates any fake response headers.

Err...  That's not quite right.  I'd suggest modifying the delegate's
OnDidLoadResourceFromMemoryCache to take in new_url as an argument, and just
pass that along.  Seems weird to get it from the headers again, when our caller,
a function of the same name, already takes it as an argument.

[Shishir, 2012-08-01 18:07:49]

Addressed Matts comments.

Adding joi for: content/public/... 
Adding  jhawkins for chrome/browser/...

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:45: if (headers) {
On 2012/08/01 15:59:00, Matt Menke wrote:
> On 2012/08/01 15:39:53, Matt Menke wrote:
> > Rather than muck with the full set of headers, can't we just have
> > ChromeResourceDispatcherHostDelegate::OnRequestRedirected pass along
> > response->head.final_url?  Not sure if we care about HSTS, but I'm not sure
if
> > net::URLRequestRedirectJob generates any fake response headers.
> 
> Err...  That's not quite right.  I'd suggest modifying the delegate's
> OnDidLoadResourceFromMemoryCache to take in new_url as an argument, and just
> pass that along.  Seems weird to get it from the headers again, when our
caller,
> a function of the same name, already takes it as an argument.

Done.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:45: if (headers) {
On 2012/08/01 15:39:53, Matt Menke wrote:
> Rather than muck with the full set of headers, can't we just have
> ChromeResourceDispatcherHostDelegate::OnRequestRedirected pass along
> response->head.final_url?  Not sure if we care about HSTS, but I'm not sure if
> net::URLRequestRedirectJob generates any fake response headers.

Passing on the redirect url.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:92:
redirect_url(other.redirect_url) {
On 2012/08/01 15:39:53, Matt Menke wrote:
> Completely unrelated, but while you're here, mind fixing the indentation here?

> Thanks.

Done.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:361: // redirect url as
a new navigation.
On 2012/08/01 10:25:37, Matt Menke wrote:
> This comment is phrased/placed really oddly, since the code it's just above
> exits the function, conditioned on an if statement the comment doesn't mention
> at all.  Maybe just add a blank line, to it's clearer it applies to more than
> just the code immediately following it?  Or move it below the if statement.

Done.

http://codereview.chromium.org/10827103/diff/7002/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:362: if
(response.redirect_url.is_empty())
On 2012/08/01 10:25:37, Matt Menke wrote:
> When does this happen?  Think a comment would be reasonable.

Done.

[mmenke, 2012-08-01 18:24:05]

LGTM

[mmenke, 2012-08-01 18:35:49]

Also, you should have a bug for this feature, and link to it in the CL
description.

[Jói, 2012-08-01 19:28:24]

LGTM for content/public.

[James Hawkins, 2012-08-02 18:17:50]

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.h:11: #include
"googleurl/src/gurl.h"
Forward declare GURL instead.

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:352: // We do not see
another navigation start for a redirect, so record the
nit: Don't use pronouns in comments (we); pronouns are ambiguous.

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/async_resource_handler.cc:147:
rdh_->delegate()->OnRequestRedirected(new_url,
Optional nit: Condense parameters to save lines.

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/sync_resource_handler.cc (right):

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/sync_resource_handler.cc:51:
rdh_->delegate()->OnRequestRedirected(new_url,
Optional nit: Condense parameters to save lines.

[Shishir, 2012-08-02 18:25:43]

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.h:11: #include
"googleurl/src/gurl.h"
On 2012/08/02 18:17:50, James Hawkins wrote:
> Forward declare GURL instead.

Done.

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

http://codereview.chromium.org/10827103/diff/6005/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:352: // We do not see
another navigation start for a redirect, so record the
On 2012/08/02 18:17:50, James Hawkins wrote:
> nit: Don't use pronouns in comments (we); pronouns are ambiguous.

Done.

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/async_resource_handler.cc:147:
rdh_->delegate()->OnRequestRedirected(new_url,
On 2012/08/02 18:17:50, James Hawkins wrote:
> Optional nit: Condense parameters to save lines.

Done.

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/sync_resource_handler.cc (right):

http://codereview.chromium.org/10827103/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/sync_resource_handler.cc:51:
rdh_->delegate()->OnRequestRedirected(new_url,
On 2012/08/02 18:17:50, James Hawkins wrote:
> Optional nit: Condense parameters to save lines.

Done.

[James Hawkins, 2012-08-02 18:32:58]

LGTM

[commit-bot: I haz the power, 2012-08-02 18:39:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10827103/1011

[commit-bot: I haz the power, 2012-08-02 18:40:10]

Presubmit check for 10827103-1011 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host

Presubmit checks took 2.1s to calculate.

[Shishir, 2012-08-02 18:44:57]

Adding creis for chrome/browser/renderer_host/...

[Charlie Reis, 2012-08-02 20:56:16]

On 2012/08/02 18:44:57, Shishir wrote:
> Adding creis for chrome/browser/renderer_host/...

I presume you mean content/browser/renderer_host.

You still need a BUG=# line for your CL, as Matt requested.  I don't understand
what this is trying to solve, so I'd like to see the bug report.

[Shishir, 2012-08-02 21:01:05]

Added a bug number.

[Charlie Reis, 2012-08-02 21:17:59]

I see, thanks.  LGTM.

[commit-bot: I haz the power, 2012-08-02 21:20:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10827103/1011

[commit-bot: I haz the power, 2012-08-02 22:39:59]

Change committed as 149727