Prerendered pages are swapped in at browser::Navigate time.

Prerendered pages are swapped in at browser::Navigate time.

They used to get swapped in by observing provisional loads. This is being changed to Navigate so renderer-issued navigations into a prerender can be handled with the same mechanism that is used to handle renderer-issued navigations which need to cross process boundaries, such as clicking into a hosted app.

For browser issued navigations, this means that we will immediately swap in the prerender, instead of sending a Navigate message to the render view and swapping in on the provisional load statement.

For renderer issued navigations, decidePolicyForNavigation will ultimately cancel the navigation in the renderer and send an OpenURL up to the browser process.

In order to make the renderer know that a navigation could be prerendered, a set of prerendered URLs need to be sent to a render process.

BUG=104493
TEST=Existing browser tests, manual tests that omnibox prerenders work.

[cbentzel, 2012-02-24 22:59:13]

This isn't fully ready to go - I need a tiny bit more nit cleanup, and still
need to filter which render processes receive the Add/Remove Prerender IPCs. I
also haven't spent much time looking at the impact on the myriad number of
histograms.

But this is the general approach I am taking. All of the browsertests passed
locally for me and I manually tested omnibox prerenders.

[dominich, 2012-02-28 16:21:00]

Generally looks good.

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:175: // synchronously on a
Navigation.
Is it worth splitting the tests into browser- and render-initiated tests? it
might simplify the message loop handling with your new change as we could test
the majority of the code under the browser and then test some core functionality
under render.

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

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:251: // TODO(cbentzel): Need to
filter to the current profile only.
Should you also filter to the RPH that was the source of the prerender? That
should be the only one that needs to know about it.

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:252: IPC::Message* message =
NULL;
IPC::Message* message = is_add ? new PrerenderMsg_AddPrerenderURL(url) :
                                 new PrerenderMsg_RemovePrerenderURL(url);

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

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:334: if (web_contents &&
I'm unclear what it means to have a NULL web_contents here. Should there be a
DCHECK for that?

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:422: false));
nit: indent

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

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:130: // page can't be swapped in,
NULL is returned.
NULL -> false

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:768: virtual content::WebContents* OpenURLFromTab(
indent

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:549: if
(SwapInPrerender(params->target_contents, url)) {
nit: you don't need the {} here.

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.cc:44: if (it->second > 1) {
this could also be

it->second -= 1;
if (it->second == 0)
  prerender_urls_.erase(it);

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:18: // PrerenderDispatcher pays
attention to which URLs are prerendered.
nit: 'pays attention to' is a bit vague. What does it actually do with them? Who
owns this? Is there only one instance, or one per render process?

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:31: std::map<GURL, int>
prerender_urls_;
nit: use PrerenderMap in this declaration.

http://codereview.chromium.org/9416031/diff/11022/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_view_helper.cc (right):

http://codereview.chromium.org/9416031/diff/11022/content/browser/tab_content...
content/browser/tab_contents/tab_contents_view_helper.cc:68: // We could
potentially add an opener here. That way we would know not to open. We also
nit: line length

[cbentzel, 2012-02-29 18:16:13]

Thanks for the review. The latest patchset restricts the
AddPrerender/RemovePrerender to the render process which generated the prerender
via <link rel="prerender">. It also addresses a number of other small problems.

I'll need to rebase now, so wanted you to be able to see it on a small delta.

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

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:251: // TODO(cbentzel): Need to
filter to the current profile only.
On 2012/02/28 16:21:00, dominich wrote:
> Should you also filter to the RPH that was the source of the prerender? That
> should be the only one that needs to know about it.

I am limiting to that now, per your comment as well as creis's.

There may be some prerenders we are dropping on the floor here as a result, but
my guess it it's pretty low. However, this may change if we start doing some
other triggers for prerender such as top sites or co-visited domains.

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:252: IPC::Message* message =
NULL;
On 2012/02/28 16:21:00, dominich wrote:
> IPC::Message* message = is_add ? new PrerenderMsg_AddPrerenderURL(url) :
>                                  new PrerenderMsg_RemovePrerenderURL(url);

This doesn't work because then the operands of the ternary operator are
different types. I could do a static_cast to IPC::Message, but then it's verbose
and doesn't buy much.

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

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:334: if (web_contents &&
On 2012/02/28 16:21:00, dominich wrote:
> I'm unclear what it means to have a NULL web_contents here. Should there be a
> DCHECK for that?

This is not needed anymore - thanks for the catch.

If you're interested, my CL originally called MaybeUsePrerenderedPage in a
different location then the current call location, and there was a NULL
web_contents when handling the shift-click case (open link in new window,
initiated from the renderer).

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:422: false));
On 2012/02/28 16:21:00, dominich wrote:
> nit: indent

Done.

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:768: virtual content::WebContents* OpenURLFromTab(
On 2012/02/28 16:21:00, dominich wrote:
> indent

Done.

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:549: if
(SwapInPrerender(params->target_contents, url)) {
On 2012/02/28 16:21:00, dominich wrote:
> nit: you don't need the {} here.

Done.

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.cc:44: if (it->second > 1) {
On 2012/02/28 16:21:00, dominich wrote:
> this could also be
> 
> it->second -= 1;
> if (it->second == 0)
>   prerender_urls_.erase(it);
> 

I like it. Done.

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/28 16:21:00, dominich wrote:
> nit: 2012

Done.

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:18: // PrerenderDispatcher pays
attention to which URLs are prerendered.
On 2012/02/28 16:21:00, dominich wrote:
> nit: 'pays attention to' is a bit vague. What does it actually do with them?
Who
> owns this? Is there only one instance, or one per render process?

Improved description.

http://codereview.chromium.org/9416031/diff/11022/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:31: std::map<GURL, int>
prerender_urls_;
On 2012/02/28 16:21:00, dominich wrote:
> nit: use PrerenderMap in this declaration.

Nice catch. Done.

http://codereview.chromium.org/9416031/diff/11022/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_view_helper.cc (right):

http://codereview.chromium.org/9416031/diff/11022/content/browser/tab_content...
content/browser/tab_contents/tab_contents_view_helper.cc:68: // We could
potentially add an opener here. That way we would know not to open. We also
On 2012/02/28 16:21:00, dominich wrote:
> nit: line length

Actually - I don't need this comment anymore.

[dominich, 2012-02-29 18:32:00]

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

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:70: // for the same URL in
another page will not swap in the prerender.
This isn't a problem for Omnibox as we cancel the prerender if the user
navigates somewhere other than we predicted already.

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:71: void
InformRenderProcessAboutPrerender(const GURL& url,
Given this is only called from methods in this class and |url| and |child_id|
are always |prerender_url_| and |creator_child_id_|, it would make more sense as
a private method of the class with a single bool parameter.

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:81: if (is_add) {
nit: {} not required

http://codereview.chromium.org/9416031/diff/25001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9416031/diff/25001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:653: // to swap in the
prerendered page on the browser process. If the prerendered
If the prerender swap fails for some reason, will the navigate somehow continue
successfully anyway? I'm a bit unclear on the logic here.

http://codereview.chromium.org/9416031/diff/25001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/9416031/diff/25001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:427:
browser->OpenURLFromTab(browser->GetSelectedWebContents(), OpenURLParams(
It's unclear how this change is related to the prerender changes.

[cbentzel, 2012-02-29 19:22:34]

OK, now I will actually rebase :)

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

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:70: // for the same URL in
another page will not swap in the prerender.
On 2012/02/29 18:32:01, dominich wrote:
> This isn't a problem for Omnibox as we cancel the prerender if the user
> navigates somewhere other than we predicted already.

Done.

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:71: void
InformRenderProcessAboutPrerender(const GURL& url,
On 2012/02/29 18:32:01, dominich wrote:
> Given this is only called from methods in this class and |url| and |child_id|
> are always |prerender_url_| and |creator_child_id_|, it would make more sense
as
> a private method of the class with a single bool parameter.

I like it here, because it means that if it changes I don't have to modify the
.h

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:81: if (is_add) {
On 2012/02/29 18:32:01, dominich wrote:
> nit: {} not required

Done.

http://codereview.chromium.org/9416031/diff/25001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/9416031/diff/25001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:427:
browser->OpenURLFromTab(browser->GetSelectedWebContents(), OpenURLParams(
On 2012/02/29 18:32:01, dominich wrote:
> It's unclear how this change is related to the prerender changes.

Ah - another great catch. This was related to the NULL web_contents check I had
in MaybeUsePrerenderedPage that is no longer needed. Removed, and un-public'ed
it from browser.

[dominich, 2012-02-29 19:44:35]

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

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:71: void
InformRenderProcessAboutPrerender(const GURL& url,
On 2012/02/29 19:22:36, cbentzel wrote:
> On 2012/02/29 18:32:01, dominich wrote:
> > Given this is only called from methods in this class and |url| and
|child_id|
> > are always |prerender_url_| and |creator_child_id_|, it would make more
sense
> as
> > a private method of the class with a single bool parameter.
> 
> I like it here, because it means that if it changes I don't have to modify the
> .h

Not modifying a header is a poor reason to base API decisions on. This should be
a class method as it uses state managed by the class. If it should change to be
more complex, then either more state will need to be added to the class, and
it's still correct, or it should be moved to a PrerenderBroadcaster class that
will have its own state.

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

http://codereview.chromium.org/9416031/diff/31026/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:422: false));
nit: indent is off by one.

[cbentzel, 2012-02-29 19:50:10]

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

http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:71: void
InformRenderProcessAboutPrerender(const GURL& url,
On 2012/02/29 19:44:35, dominich wrote:
> On 2012/02/29 19:22:36, cbentzel wrote:
> > On 2012/02/29 18:32:01, dominich wrote:
> > > Given this is only called from methods in this class and |url| and
> |child_id|
> > > are always |prerender_url_| and |creator_child_id_|, it would make more
> sense
> > as
> > > a private method of the class with a single bool parameter.
> > 
> > I like it here, because it means that if it changes I don't have to modify
the
> > .h
> 
> Not modifying a header is a poor reason to base API decisions on. This should
be
> a class method as it uses state managed by the class. If it should change to
be
> more complex, then either more state will need to be added to the class, and
> it's still correct, or it should be moved to a PrerenderBroadcaster class that
> will have its own state.

Huh? It's a pretty stateless function, and does not directly modify any state
related to that class. That to me screams function.

[cbentzel, 2012-02-29 19:50:40]

On Wed, Feb 29, 2012 at 2:50 PM, <cbentzel@chromium.org> wrote:

>
> http://codereview.chromium.**org/9416031/diff/25001/chrome/**
>
browser/prerender/prerender_**contents.cc<http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/prerender_contents.cc>
> File chrome/browser/prerender/**prerender_contents.cc (right):
>
> http://codereview.chromium.**org/9416031/diff/25001/chrome/**
>
browser/prerender/prerender_**contents.cc#newcode71<http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/prerender_contents.cc#newcode71>
> chrome/browser/prerender/**prerender_contents.cc:71: void
> InformRenderProcessAboutPreren**der(const GURL& url,
> On 2012/02/29 19:44:35, dominich wrote:
>
>> On 2012/02/29 19:22:36, cbentzel wrote:
>> > On 2012/02/29 18:32:01, dominich wrote:
>> > > Given this is only called from methods in this class and |url| and
>> |child_id|
>> > > are always |prerender_url_| and |creator_child_id_|, it would make
>>
> more
>
>> sense
>> > as
>> > > a private method of the class with a single bool parameter.
>> >
>> > I like it here, because it means that if it changes I don't have to
>>
> modify the
>
>> > .h
>>
>
>  Not modifying a header is a poor reason to base API decisions on. This
>>
> should be
>
>> a class method as it uses state managed by the class. If it should
>>
> change to be
>
>> more complex, then either more state will need to be added to the
>>
> class, and
>
>> it's still correct, or it should be moved to a PrerenderBroadcaster
>>
> class that
>
>> will have its own state.
>>
>
> Huh? It's a pretty stateless function, and does not directly modify any
> state related to that class. That to me screams function.
>

I guess it's not stateless - but it doesn't modify state in
PrerenderContents.


>
>
http://codereview.chromium.**org/9416031/<http://codereview.chromium.org/9416...
>

[cbentzel, 2012-02-29 19:52:00]

On 2012/02/29 19:50:10, cbentzel wrote:
>
http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.cc:71: void
> InformRenderProcessAboutPrerender(const GURL& url,
> On 2012/02/29 19:44:35, dominich wrote:
> > On 2012/02/29 19:22:36, cbentzel wrote:
> > > On 2012/02/29 18:32:01, dominich wrote:
> > > > Given this is only called from methods in this class and |url| and
> > |child_id|
> > > > are always |prerender_url_| and |creator_child_id_|, it would make more
> > sense
> > > as
> > > > a private method of the class with a single bool parameter.
> > > 
> > > I like it here, because it means that if it changes I don't have to modify
> the
> > > .h
> > 
> > Not modifying a header is a poor reason to base API decisions on. This
should
> be
> > a class method as it uses state managed by the class. If it should change to
> be
> > more complex, then either more state will need to be added to the class, and
> > it's still correct, or it should be moved to a PrerenderBroadcaster class
that
> > will have its own state.
> 
> Huh? It's a pretty stateless function, and does not directly modify any state
> related to that class. That to me screams function.

It's also not part of the public API for the class.

[dominich, 2012-02-29 20:10:42]

On 2012/02/29 19:52:00, cbentzel wrote:
> On 2012/02/29 19:50:10, cbentzel wrote:
> >
>
http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
> > File chrome/browser/prerender/prerender_contents.cc (right):
> > 
> >
>
http://codereview.chromium.org/9416031/diff/25001/chrome/browser/prerender/pr...
> > chrome/browser/prerender/prerender_contents.cc:71: void
> > InformRenderProcessAboutPrerender(const GURL& url,
> > On 2012/02/29 19:44:35, dominich wrote:
> > > On 2012/02/29 19:22:36, cbentzel wrote:
> > > > On 2012/02/29 18:32:01, dominich wrote:
> > > > > Given this is only called from methods in this class and |url| and
> > > |child_id|
> > > > > are always |prerender_url_| and |creator_child_id_|, it would make
more
> > > sense
> > > > as
> > > > > a private method of the class with a single bool parameter.
> > > > 
> > > > I like it here, because it means that if it changes I don't have to
modify
> > the
> > > > .h
> > > 
> > > Not modifying a header is a poor reason to base API decisions on. This
> should
> > be
> > > a class method as it uses state managed by the class. If it should change
to
> > be
> > > more complex, then either more state will need to be added to the class,
and
> > > it's still correct, or it should be moved to a PrerenderBroadcaster class
> that
> > > will have its own state.
> > 
> > Huh? It's a pretty stateless function, and does not directly modify any
state
> > related to that class. That to me screams function.
> 
> It's also not part of the public API for the class.

I agree it's not public API, but it uses state from the class. If you think that
it could be rewritten as two methods (add and remove) then there'd be no need
for any parameters (state) to be passed at all.

However, this is not a big deal by any means and if the other reviewers don't
care either way I'm happy to leave it as is.

[cbentzel, 2012-02-29 21:34:29]

jam: Could you review the non-prerender code? This touches IPC,
chrome/renderer/chrome_content_renderer_client, and some other portions.

[cbentzel, 2012-02-29 21:36:48]

jam: Could you review the non-prerender code?

creis: This is an FYI but appreciate any feedback - we talked high-level about
it earlier. Prerender now uses the decidedPolicyForNavigation/OpenURL path for
swapping in prerenders, like hosted apps and other renderer-initiated
cross-process navigations. This version also restricts advertising prerendered
URLs to only the render process which initiated them.

[cbentzel, 2012-02-29 21:38:28]

+jcivelli - this changes the mechanism for when prerenders get swapped in, but
should not have any user-perceived behavior change.

[jam, 2012-03-01 01:05:25]

On 2012/02/29 21:36:48, cbentzel wrote:
> jam: Could you review the non-prerender code?
> 
> creis: This is an FYI but appreciate any feedback - we talked high-level about
> it earlier. Prerender now uses the decidedPolicyForNavigation/OpenURL path for
> swapping in prerenders, like hosted apps and other renderer-initiated
> cross-process navigations. This version also restricts advertising prerendered
> URLs to only the render process which initiated them.

non pre-render code lgtm

[tburkard, 2012-03-01 01:38:22]

LGTM

On Wed, Feb 29, 2012 at 5:05 PM, <jam@chromium.org> wrote:

> On 2012/02/29 21:36:48, cbentzel wrote:
>
>> jam: Could you review the non-prerender code?
>>
>
>  creis: This is an FYI but appreciate any feedback - we talked high-level
>> about
>> it earlier. Prerender now uses the decidedPolicyForNavigation/**OpenURL
>> path for
>> swapping in prerenders, like hosted apps and other renderer-initiated
>> cross-process navigations. This version also restricts advertising
>> prerendered
>> URLs to only the render process which initiated them.
>>
>
> non pre-render code lgtm
>
>
http://codereview.chromium.**org/9416031/<http://codereview.chromium.org/9416...
>

[Charlie Reis, 2012-03-01 19:06:03]

Yes, this seems reasonable to me.  A few notes and one question about
opener_origin_ below.

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

http://codereview.chromium.org/9416031/diff/44004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:69: // will not be swapped in.
I think this is the right call.  For reference, we may eventually have renderers
check in with the browser on every navigation (e.g., if we enforce strict site
isolation).  In that case the browser would be able to make a more informed
decision.

http://codereview.chromium.org/9416031/diff/44004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:83: message = new
PrerenderMsg_RemovePrerenderURL(url);
Just as a sanity check, we don't need to be concerned about races here, right? 
If a renderer thinks we can prerender it and we've since deleted, we'll just
load it from scratch.  And if it thinks we can't prerender it but we can, it's
just some wasted effort.  Seems ok to me.

http://codereview.chromium.org/9416031/diff/44004/chrome/common/prerender_mes...
File chrome/common/prerender_messages.h (right):

http://codereview.chromium.org/9416031/diff/44004/chrome/common/prerender_mes...
chrome/common/prerender_messages.h:16:
IPC_MESSAGE_ROUTED1(PrerenderMsg_SetIsPrerendering,
nit: SetAllowPrerendering, perhaps?  Right now it sounds like we're actively
prerendering a page, but we use AddPrerenderURL for that.

http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:657: // The origin of the opener.
I don't quite follow this.  The origin of the opener can change if it navigates.
 How long is this valid?  If you only care about it for the initial value, maybe
we should be clearer in the name, like initial_opener_origin_ (though I'm not
sure what that's useful for).

[cbentzel, 2012-03-01 22:18:53]

Thanks for the review Charlie - particular the part about the opener.

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

http://codereview.chromium.org/9416031/diff/44004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:83: message = new
PrerenderMsg_RemovePrerenderURL(url);
On 2012/03/01 19:06:03, creis wrote:
> Just as a sanity check, we don't need to be concerned about races here, right?

> If a renderer thinks we can prerender it and we've since deleted, we'll just
> load it from scratch.  And if it thinks we can't prerender it but we can, it's
> just some wasted effort.  Seems ok to me.

You are correct - there is not a race here.

I commented about the impact of a delayed RemovePrerenderURL case in ShouldFork
- would it help to copy here as well?

http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:657: // The origin of the opener.
On 2012/03/01 19:06:03, creis wrote:
> I don't quite follow this.  The origin of the opener can change if it
navigates.
>  How long is this valid?  If you only care about it for the initial value,
maybe
> we should be clearer in the name, like initial_opener_origin_ (though I'm not
> sure what that's useful for).

I thought opener was associated with a browsing context, rather than an
individual navigation within the context?

This is probably incorrect either way. I'll rethink how to do this.

[Charlie Reis, 2012-03-01 22:24:38]

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

http://codereview.chromium.org/9416031/diff/44004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:83: message = new
PrerenderMsg_RemovePrerenderURL(url);
On 2012/03/01 22:18:53, cbentzel wrote:
> On 2012/03/01 19:06:03, creis wrote:
> > Just as a sanity check, we don't need to be concerned about races here,
right?
> 
> > If a renderer thinks we can prerender it and we've since deleted, we'll just
> > load it from scratch.  And if it thinks we can't prerender it but we can,
it's
> > just some wasted effort.  Seems ok to me.
> 
> You are correct - there is not a race here.
> 
> I commented about the impact of a delayed RemovePrerenderURL case in
ShouldFork
> - would it help to copy here as well?

That comment is sufficient, thanks.  This file just came up earlier in my
review.

http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:657: // The origin of the opener.
On 2012/03/01 22:18:53, cbentzel wrote:
> On 2012/03/01 19:06:03, creis wrote:
> > I don't quite follow this.  The origin of the opener can change if it
> navigates.
> >  How long is this valid?  If you only care about it for the initial value,
> maybe
> > we should be clearer in the name, like initial_opener_origin_ (though I'm
not
> > sure what that's useful for).
> 
> I thought opener was associated with a browsing context, rather than an
> individual navigation within the context?
> 

Correct.  The opener is a browser context, like a tab or a frame.  It stays
valid as the page within it (and thus the origin) changes.  That's why I don't
think it makes sense to remember the origin of the opener's current page.

> This is probably incorrect either way. I'll rethink how to do this.

[cbentzel, 2012-03-01 22:38:22]

On Thu, Mar 1, 2012 at 5:24 PM, <creis@chromium.org> wrote:

>
> http://codereview.chromium.**org/9416031/diff/44004/chrome/**
>
browser/prerender/prerender_**contents.cc<http://codereview.chromium.org/9416031/diff/44004/chrome/browser/prerender/prerender_contents.cc>
> File chrome/browser/prerender/**prerender_contents.cc (right):
>
> http://codereview.chromium.**org/9416031/diff/44004/chrome/**
>
browser/prerender/prerender_**contents.cc#newcode83<http://codereview.chromium.org/9416031/diff/44004/chrome/browser/prerender/prerender_contents.cc#newcode83>
> chrome/browser/prerender/**prerender_contents.cc:83: message = new
> PrerenderMsg_**RemovePrerenderURL(url);
> On 2012/03/01 22:18:53, cbentzel wrote:
>
>> On 2012/03/01 19:06:03, creis wrote:
>> > Just as a sanity check, we don't need to be concerned about races
>>
> here, right?
>
>  > If a renderer thinks we can prerender it and we've since deleted,
>>
> we'll just
>
>> > load it from scratch.  And if it thinks we can't prerender it but we
>>
> can, it's
>
>> > just some wasted effort.  Seems ok to me.
>>
>
>  You are correct - there is not a race here.
>>
>
>  I commented about the impact of a delayed RemovePrerenderURL case in
>>
> ShouldFork
>
>> - would it help to copy here as well?
>>
>
> That comment is sufficient, thanks.  This file just came up earlier in
> my review.
>
>
> http://codereview.chromium.**org/9416031/diff/44004/**
>
content/browser/tab_contents/**tab_contents.h<http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_contents/tab_contents.h>
> File content/browser/tab_contents/**tab_contents.h (right):
>
> http://codereview.chromium.**org/9416031/diff/44004/**
>
content/browser/tab_contents/**tab_contents.h#newcode657<http://codereview.chromium.org/9416031/diff/44004/content/browser/tab_contents/tab_contents.h#newcode657>
> content/browser/tab_contents/**tab_contents.h:657: // The origin of the
> opener.
> On 2012/03/01 22:18:53, cbentzel wrote:
>
>> On 2012/03/01 19:06:03, creis wrote:
>> > I don't quite follow this.  The origin of the opener can change if
>>
> it
>
>> navigates.
>> >  How long is this valid?  If you only care about it for the initial
>>
> value,
>
>> maybe
>> > we should be clearer in the name, like initial_opener_origin_
>>
> (though I'm not
>
>> > sure what that's useful for).
>>
>
>  I thought opener was associated with a browsing context, rather than
>>
> an
>
>> individual navigation within the context?
>>
>
>
> Correct.  The opener is a browser context, like a tab or a frame.  It
> stays valid as the page within it (and thus the origin) changes.  That's
> why I don't think it makes sense to remember the origin of the opener's
> current page.


Yes, makes sense. Let me think some more about a different approach.


>  This is probably incorrect either way. I'll rethink how to do this.
>>
>
>
http://codereview.chromium.**org/9416031/<http://codereview.chromium.org/9416...
>

[cbentzel, 2012-03-05 15:38:57]

I changed this to just record that a window.opener was set at all, rather than
remember the origin URL at the time that the opened browser context is created.

It turns out I had a misconception of how window.opener is supposed to work. We
used to allow prerenders when the window.opener was from a different origin than
the prerendered page at the time of creation, but this was incorrect since the
origin of the opener context could change to be same-origin.

However, in reality even if we allowed the opener case it would have been
canceled due to session storage namespace mismatch issues.

http://codereview.chromium.org/9416031/diff/44004/chrome/common/prerender_mes...
File chrome/common/prerender_messages.h (right):

http://codereview.chromium.org/9416031/diff/44004/chrome/common/prerender_mes...
chrome/common/prerender_messages.h:16:
IPC_MESSAGE_ROUTED1(PrerenderMsg_SetIsPrerendering,
On 2012/03/01 19:06:03, creis wrote:
> nit: SetAllowPrerendering, perhaps?  Right now it sounds like we're actively
> prerendering a page, but we use AddPrerenderURL for that.

This is sent to the actual prerendered RenderView to let it know that it is
prerendering.

AddPrerenderURL and RemovePrerenderURL are sent to the RenderView process that
created the prerender to let it know about possible ones.

[dominich, 2012-03-05 15:55:29]

lgtm

[cbentzel, 2012-03-05 17:03:17]

John - latest patches make a slight change to the WebContents API.

Charlie - how does this look for the opener case?

[Charlie Reis, 2012-03-05 17:27:04]

On 2012/03/05 17:03:17, cbentzel wrote:
> John - latest patches make a slight change to the WebContents API.
> 
> Charlie - how does this look for the opener case?

Yes, that's much better than before.  LGTM.

FYI, I still think the presence of an opener isn't quite the right thing to be
looking for to prevent a prerender.  It's only one of many cases that a page
might be sharing a script connection (and process) with another page, such as if
it *is* the opener for another page.  Let's get this fix landed, though, and
then consider improvements.  For example, Nasko is introducing a
RenderProcessHost::IsSharedByMultipleViews() function that might be a better
thing to check.

[jam, 2012-03-06 15:53:40]

On 2012/03/05 17:03:17, cbentzel wrote:
> John - latest patches make a slight change to the WebContents API.
> 
> Charlie - how does this look for the opener case?

stlgtm

[cbentzel, 2012-03-06 19:33:40]

Dominic + Timo: you may want to look at patches after 23.

This broke prerender swapin if the prerendered page had client-side or
server-side redirects and you clicked on a URL which matched one of the
redirected-to-URLs but not the original URLs.

I added browser_tests for this case so we don't regress in the future [I
discovered this by manually testing right before I was ready to land].

I'll land pending trybots.

[dominich, 2012-03-06 19:41:39]

still LGTM - just want to check if there's a better way to handle passing of all
alias urls.

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

http://codereview.chromium.org/9416031/diff/62015/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:399:
InformRenderProcessAboutPrerender(*it, false, creator_child_id_);
Is there any way to pass a list in a message instead of sending a whole bunch of
messages? I imagine this will only be one or two in most cases, but it could be
more, right?

[cbentzel, 2012-03-06 19:45:27]

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

http://codereview.chromium.org/9416031/diff/62015/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:399:
InformRenderProcessAboutPrerender(*it, false, creator_child_id_);
On 2012/03/06 19:41:39, dominich wrote:
> Is there any way to pass a list in a message instead of sending a whole bunch
of
> messages? I imagine this will only be one or two in most cases, but it could
be
> more, right?

I'm not sure if the IPCs can handle a vector<GURL> style argument. I'll see.

You bring up a good point about "it could be more". We might want to add a cap
of max allowable number of alias URLs for a particular prerendered page, and
enforce that cap in AddAliasURL.

[cbentzel, 2012-03-06 22:26:01]

Matt - can you review the net_internals_test change? You may want to look at the
prerender_browsertest for justification.

[mmenke, 2012-03-06 22:36:31]

On 2012/03/06 22:26:01, cbentzel wrote:
> Matt - can you review the net_internals_test change? You may want to look at
the
> prerender_browsertest for justification.

net_internals_tests LGTM (As does prerender_browsertest.cc)

[commit-bot: I haz the power, 2012-03-07 21:33:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/9416031/71032

[commit-bot: I haz the power, 2012-03-07 21:33:07]

Can't process patch for file chrome/renderer/chrome_content_renderer_client.cc.
File's status is None, patchset upload is incomplete.

[cbentzel, 2012-03-07 22:27:08]

Everyone - I'm going to have to move this over to a different CL issue due to a
rietveld problem. See
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...
for more info.

The new CL is http://codereview.chromium.org/9416031/

I'm only going to include John Abd-El-Malek on the reviewers list there, and it
will include no changes relative to this one except for merges if needed.

[cbentzel, 2012-03-07 22:29:02]

Oops, I mean http://codereview.chromium.org/9623018/