For prerendering, keep track of all the intermediate redirects, and hook into...

For prerendering, keep track of all the intermediate redirects, and hook into
the provisional load messages to catch an earlier time to substitute in the
prerendered page.
BUG=none
TEST=search for techcrunch and notice how the preloaded techcrunch.com is used despite the redirect

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

[tburkard, 2011-01-12 17:40:02]

I created the change to keep track of all the redirects for prerendered URLs,
both when the prerendering happens as well as when a page is loaded in tab
contents, and to do the matching of URLs accordingly.

This will fix cases like techcrunch.com, where google search results send the
user to www.techcrunch.com, which redirects to techcrunch.com.

Thanks.

[cbentzel, 2011-01-12 18:04:14]

I'll do a longer look later today.

Please add unit tests.

Also, you'll want to accumulate all potential URLs in the
PrerenderResourceHandler before creating a PrerenderContents [via the
PrerenderManager].

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:251: printf("\n\n*** new matching
URL: %s\n\n", url.spec().c_str());
Remove printf

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:256: for
(std::vector<GURL>::iterator it = matching_urls_.begin();
Just use std::find instead of writing out the loop?

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:43: printf("\n\n\n\n**** PRERENDER
REQUEST FOR %s\n\n\n\n", url.spec().c_str());
Remove printf, or use VLOG if really necessary.

[tburkard, 2011-01-12 21:32:10]

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:251: printf("\n\n*** new matching
URL: %s\n\n", url.spec().c_str());
On 2011/01/12 18:04:14, cbentzel wrote:
> Remove printf

Done.

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:256: for
(std::vector<GURL>::iterator it = matching_urls_.begin();
On 2011/01/12 18:04:14, cbentzel wrote:
> Just use std::find instead of writing out the loop?

had a more complex check in there at first.

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6171007/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:43: printf("\n\n\n\n**** PRERENDER
REQUEST FOR %s\n\n\n\n", url.spec().c_str());
On 2011/01/12 18:04:14, cbentzel wrote:
> Remove printf, or use VLOG if really necessary.

Done.

[tburkard, 2011-01-12 22:18:04]

Chris, i just added the change we discussed to keep a list of all the redirected
URLs.  please re-review.  Thanks.

[cbentzel, 2011-01-12 23:49:53]

Quick question: who calls OnReceivedMessage?

I'd like to get a browser test going for this to test pretty soon.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:255: for
(std::vector<GURL>::iterator it = matching_urls_.begin();
i'd just do std::find here.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:255: for
(std::vector<GURL>::iterator it = matching_urls_.begin();
const_iterator to be safe [and this method should be const]

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:265: for
(std::vector<GURL>::const_iterator it = alias_urls.begin();
I think this can go away in favor of constructor (which would initialize
matching_urls_)

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:43: void AddAliasURLs(const
std::vector<GURL>& alias_urls);
Why not just pass in the alias_urls to the PrerenderContents constructor? 

If you keep this method, please document it. Maybe move MatchesUrl closer.

Also, most of the other references are "matching_url" rather than "alias_url".
Be consistent with the name: I actual prefer alias, but matching is OK as well.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:58: bool MatchesUrl(const GURL&
url);
Nit: MatchesURL instead of MatchesUrl to be consistent.

Also, this should probably be const.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:143: virtual bool
OnMessageReceived(const IPC::Message& message);
I think it's OK to make this private as well.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:154: void AddMatchingUrl(const
GURL& url);
Nit: AddMatchingURL

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:48: it != prerender_list_.end();
Do you want to do MatchesUrl() here? 

url should match in most cases because it's the final redirect location. So,
something like 

A->B
C->B

would both provide B as |url| and only the alias |A| would be included for
matching against requests (and the C->B path would not cause a new prerender). 

I think this is OK, but just wanted your opinion.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:10: #include <vector>
Should a set be used instead of a vector? It seems like a vector is fine even if
there is a finite redirect loop, but curious about what you think.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:173:
prerender_manager_->AddPreload(url, alias_urls);
It looks like url is going to be part of alias_urls. Is this intended?

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:75: PrerenderCallback;
Nit: indent PrerenderCallback in by 4 spaces [I don't think emacs google-c-style
does this by default]

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:85: void
RunCallbackFromUIThread(const GURL& url,
You are going to need to update prerender_resource_handler_unittest to match.

[cbentzel, 2011-01-12 23:50:35]

Oh, and definitely make sure to run against trybots: I think the current version
won't compile unit_tests due to the change in the callback signature.

[tburkard, 2011-01-13 19:14:16]

OnReceivedMessage is called for every IPC message received from the renderer.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:255: for
(std::vector<GURL>::iterator it = matching_urls_.begin();
On 2011/01/12 23:49:53, cbentzel wrote:
> i'd just do std::find here.

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:255: for
(std::vector<GURL>::iterator it = matching_urls_.begin();
On 2011/01/12 23:49:53, cbentzel wrote:
> const_iterator to be safe [and this method should be const]

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:265: for
(std::vector<GURL>::const_iterator it = alias_urls.begin();
On 2011/01/12 23:49:53, cbentzel wrote:
> I think this can go away in favor of constructor (which would initialize
> matching_urls_)

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:43: void AddAliasURLs(const
std::vector<GURL>& alias_urls);
On 2011/01/12 23:49:53, cbentzel wrote:
> Why not just pass in the alias_urls to the PrerenderContents constructor? 
> 
> If you keep this method, please document it. Maybe move MatchesUrl closer.
> 
> Also, most of the other references are "matching_url" rather than "alias_url".
> Be consistent with the name: I actual prefer alias, but matching is OK as
well. 

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:58: bool MatchesUrl(const GURL&
url);
On 2011/01/12 23:49:53, cbentzel wrote:
> Nit: MatchesURL instead of MatchesUrl to be consistent.
> 
> Also, this should probably be const.

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:143: virtual bool
OnMessageReceived(const IPC::Message& message);
On 2011/01/12 23:49:53, cbentzel wrote:
> I think it's OK to make this private as well. 

TabContents has it protected, too, so i figured protected makes more sense.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:154: void AddMatchingUrl(const
GURL& url);
On 2011/01/12 23:49:53, cbentzel wrote:
> Nit: AddMatchingURL

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:10: #include <vector>
I don't think it makes a difference..
formally set would do but since it's so small vector might be more efficient.
On 2011/01/12 23:49:53, cbentzel wrote:
> Should a set be used instead of a vector? It seems like a vector is fine even
if
> there is a finite redirect loop, but curious about what you think.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:173:
prerender_manager_->AddPreload(url, alias_urls);
Yes, that's fine.  Doesn't harm to have it in there one extra time.
On 2011/01/12 23:49:53, cbentzel wrote:
> It looks like url is going to be part of alias_urls. Is this intended?

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:75: PrerenderCallback;
On 2011/01/12 23:49:53, cbentzel wrote:
> Nit: indent PrerenderCallback in by 4 spaces [I don't think emacs
google-c-style
> does this by default]

Done.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:85: void
RunCallbackFromUIThread(const GURL& url,
On 2011/01/12 23:49:53, cbentzel wrote:
> You are going to need to update prerender_resource_handler_unittest to match.

Done.

[tburkard, 2011-01-13 19:17:42]

Running trybots now..

On Thu, Jan 13, 2011 at 11:14 AM, <tburkard@chromium.org> wrote:

> OnReceivedMessage is called for every IPC message received from the
> renderer.
>
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_contents.cc (right):
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.cc:255: for
> (std::vector<GURL>::iterator it = matching_urls_.begin();
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> i'd just do std::find here.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.cc:255: for
> (std::vector<GURL>::iterator it = matching_urls_.begin();
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> const_iterator to be safe [and this method should be const]
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.cc:265: for
> (std::vector<GURL>::const_iterator it = alias_urls.begin();
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> I think this can go away in favor of constructor (which would
>>
> initialize
>
>> matching_urls_)
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_contents.h (right):
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.h:43: void
> AddAliasURLs(const std::vector<GURL>& alias_urls);
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> Why not just pass in the alias_urls to the PrerenderContents
>>
> constructor?
>
>  If you keep this method, please document it. Maybe move MatchesUrl
>>
> closer.
>
>  Also, most of the other references are "matching_url" rather than
>>
> "alias_url".
>
>> Be consistent with the name: I actual prefer alias, but matching is OK
>>
> as well.
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.h:58: bool MatchesUrl(const
> GURL& url);
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> Nit: MatchesURL instead of MatchesUrl to be consistent.
>>
>
>  Also, this should probably be const.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.h:143: virtual bool
> OnMessageReceived(const IPC::Message& message);
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> I think it's OK to make this private as well.
>>
>
> TabContents has it protected, too, so i figured protected makes more
> sense.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.h:154: void
> AddMatchingUrl(const GURL& url);
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> Nit: AddMatchingURL
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager.h (right):
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:10: #include <vector>
> I don't think it makes a difference..
> formally set would do but since it's so small vector might be more
> efficient.
>
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> Should a set be used instead of a vector? It seems like a vector is
>>
> fine even if
>
>> there is a finite redirect loop, but curious about what you think.
>>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_resource_handler.cc (right):
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.cc:173:
> prerender_manager_->AddPreload(url, alias_urls);
> Yes, that's fine.  Doesn't harm to have it in there one extra time.
>
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> It looks like url is going to be part of alias_urls. Is this intended?
>>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_resource_handler.h (right):
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.h:75:
> PrerenderCallback;
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> Nit: indent PrerenderCallback in by 4 spaces [I don't think emacs
>>
> google-c-style
>
>> does this by default]
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.h:85: void
> RunCallbackFromUIThread(const GURL& url,
> On 2011/01/12 23:49:53, cbentzel wrote:
>
>> You are going to need to update prerender_resource_handler_unittest to
>>
> match.
>
> Done.
>
>
> http://codereview.chromium.org/6171007/
>

[brettw, 2011-01-14 01:34:08]

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:257: alias_urls_.push_back(url);
Would alos_urls_ be better as a std::set?

[cbentzel, 2011-01-14 02:01:13]

LGTM, assuming you add the unit test cases described below.

Also, my question about OnMessageReceived was essentially what the next layer in
the callstack would be. Since this is a RenderViewHostDelegate, it's probably
the RenderViewHost which calls the PrerenderContents' version.

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6171007/diff/21001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:10: #include <vector>
On 2011/01/13 19:14:16, tburkard wrote:
> I don't think it makes a difference..
> formally set would do but since it's so small vector might be more efficient.
> On 2011/01/12 23:49:53, cbentzel wrote:
> > Should a set be used instead of a vector? It seems like a vector is fine
even
> if
> > there is a finite redirect loop, but curious about what you think.
> 

Seems reasonable.

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:141: // from
renderViewHostDelegate.
Nit: RenderViewHostDelegate [capitalized]

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:48: void
AddSimplePreload(const GURL& url) {
Should you add a unit test for the multiple alias_urls case, and make sure that
the find mechanism works correctly?

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler_unittest.cc (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler_unittest.cc:103: void
SetLastHandledURL(const GURL& url, const std::vector<GURL>& alias_url) {
I recommend copying alias_urls into a member variable here, and validating.
Particularly in the PrerenderRedirect case.

[brettw, 2011-01-14 02:09:33]

I didn't really check this in detail, but I didn't see any obvious problem, so
I'll go with cbentzel's review.

[tburkard, 2011-01-18 22:29:18]

Addressed all comments, and did a gclient sync.  I will run trybots again and
then land.

Thanks.

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:257: alias_urls_.push_back(url);
On 2011/01/14 01:34:13, brettw wrote:
> Would alos_urls_ be better as a std::set?

Since it's just 1-3 elements, I thought a vector would be sufficient rather than
using a set with hashing.

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:141: // from
renderViewHostDelegate.
On 2011/01/14 02:01:13, cbentzel wrote:
> Nit: RenderViewHostDelegate [capitalized]

Done.

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:48: void
AddSimplePreload(const GURL& url) {
On 2011/01/14 02:01:13, cbentzel wrote:
> Should you add a unit test for the multiple alias_urls case, and make sure
that
> the find mechanism works correctly?

Done.

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler_unittest.cc (right):

http://codereview.chromium.org/6171007/diff/52001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler_unittest.cc:103: void
SetLastHandledURL(const GURL& url, const std::vector<GURL>& alias_url) {
On 2011/01/14 02:01:13, cbentzel wrote:
> I recommend copying alias_urls into a member variable here, and validating.
> Particularly in the PrerenderRedirect case.

Done.