Upstreaming prerendering changes for Android.

Upstreaming prerendering changes for Android.

These changes are used by the Android port.
Mostly they allow to:
- figure-out if a page has been fully rendered before switching to
  it.
- set a default size when there is no active browser (which is the
  case on Chrome for Android)

BUG=None
TEST=All prerender tests should still pass.


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

[dominich, 2012-02-14 01:11:20]

Almost there. I think the default bounds can be consolidated with the existing
default bounds better by putting that into a setting in prerender config.

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:257: gfx::Rect tab_bounds(640,
480);
This is a recent change, but a default is set here for the case that there's no
active browser and no source render view host (which is the case sometimes on
OSX).

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:277: gfx::Rect default_tab_bounds
= prerender_manager_->default_tab_bounds();
So you don't need this unless you want to override the default set above per
platform.

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:520: has_stopped_loading_ = true;
Do you need to set has_finished_loading_ = false explicitly here?

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:844: // still want the benefits or
prerendering even when several tabs are open.
nit 'or prerendering' -> 'of prerendering'

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:844: // still want the benefits or
prerendering even when several tabs are open.
This means that you can't lower the CPU process - have you measured the cost to
the user of mispredicting a prerender?

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:237: void
set_default_tab_bounds(const gfx::Rect& bounds) {
Maybe this would be better as part of the Config struct so it can be easily
overridden per platform. See prerender_config.h

[cbentzel, 2012-02-14 01:20:31]

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_manager.h (right):

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_manager.h:175: bool
DidPrerenderFinishLoading(const GURL& url);
const?

The comment about "Needs to be called" is also incorrect for non-Android
platforms. 

What's the rationale for not swapping in a partially-loaded prerendered page in
on Android? Visual noise?

[Jay Civelli, 2012-02-14 03:13:51]

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_contents.cc (right):

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_contents.cc:277: gfx::Rect default_tab_bounds
= prerender_manager_->default_tab_bounds();
On 2012/02/14 01:11:20, dominich wrote:
> So you don't need this unless you want to override the default set above per
> platform.
Moved to the config, as you suggested later on.

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_contents.cc:520: has_stopped_loading_ = true;
On 2012/02/14 01:11:20, dominich wrote:
> Do you need to set has_finished_loading_ = false explicitly here?
Do you mean has_finished_loading_ = true?
I don't think I do, we should always get a DidFinishLoad for the main frame.

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_manager.cc (right):

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_manager.cc:844: // still want the benefits or
prerendering even when several tabs are open.
On 2012/02/14 01:11:20, dominich wrote:
> nit 'or prerendering' -> 'of prerendering'

Done.

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_manager.cc:844: // still want the benefits or
prerendering even when several tabs are open.
On 2012/02/14 01:11:20, dominich wrote:
> This means that you can't lower the CPU process - have you measured the cost
to
> the user of mispredicting a prerender?
I thought there was only lower priority given to network requests from the
prerendered page and that we did not change the process priority? 
I compared the performances of the previous implementation (using the instant
code) and this one and saw no noticeable difference.

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_manager.h (right):

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_manager.h:175: bool
DidPrerenderFinishLoading(const GURL& url);
On 2012/02/14 01:20:31, cbentzel wrote:
> const?
Done.

> The comment about "Needs to be called" is also incorrect for non-Android
> platforms. 
I am not sure I understand why this is incorrect for non-Android. Once the
prerendered tab has been swapped, the prerender manager does not know anything
about it so cannot be queried for that specific URL.
 
> What's the rationale for not swapping in a partially-loaded prerendered page
in
> on Android? Visual noise? 
We do swap in that case.
It is just that Chrome for Android needs to send a notification that the page
has finished loading  (mostly for tests).
If the page has been fully prerendered, we never get that notification, so we
use this method to check for that.

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_manager.h:237: void
set_default_tab_bounds(const gfx::Rect& bounds) {
On 2012/02/14 01:11:20, dominich wrote:
> Maybe this would be better as part of the Config struct so it can be easily
> overridden per platform. See prerender_config.h
Good idea! Moved to prerender_config.

[cbentzel, 2012-02-14 10:33:36]

Generally looks fine, except:

We do drop the priority of the prerender renderer process. Since you removed the
restriction that the prerender needs to be isolated to a separate process, you
probably also want to remove the code that drops the priority, since it will
also impact foreground pages that are assigned to the same render process.

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_manager.h (right):

https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_manager.h:175: bool
DidPrerenderFinishLoading(const GURL& url);
On 2012/02/14 03:13:51, Jay Civelli wrote:
> On 2012/02/14 01:20:31, cbentzel wrote:
> > const?
> Done.
> 
> > The comment about "Needs to be called" is also incorrect for non-Android
> > platforms. 
> I am not sure I understand why this is incorrect for non-Android. Once the
> prerendered tab has been swapped, the prerender manager does not know anything
> about it so cannot be queried for that specific URL.
>  

Ah, misinterpretation on my part. I thought you meant that
MaybeUsePrerenderedPage depended on DidPrerenderFinishLoading being called
before it, not that if DidPrerenderFinishLoading is called, it must be done
prior to MaybeUsePrerenderedPage.

> > What's the rationale for not swapping in a partially-loaded prerendered page
> in
> > on Android? Visual noise? 
> We do swap in that case.
> It is just that Chrome for Android needs to send a notification that the page
> has finished loading  (mostly for tests).
> If the page has been fully prerendered, we never get that notification, so we
> use this method to check for that. 
>

[cbentzel, 2012-02-14 11:25:32]

On 2012/02/14 10:33:36, cbentzel wrote:
> Generally looks fine, except:
> 
> We do drop the priority of the prerender renderer process. Since you removed
the
> restriction that the prerender needs to be isolated to a separate process, you
> probably also want to remove the code that drops the priority, since it will
> also impact foreground pages that are assigned to the same render process.

And the place where this is done is indirectly through the HideContents() call
inside of prerender_contents.cc
> 
>
https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
> File chrome/browser/prerender/prerender_manager.h (right):
> 
>
https://chromiumcodereview.appspot.com/9387015/diff/2001/chrome/browser/prere...
> chrome/browser/prerender/prerender_manager.h:175: bool
> DidPrerenderFinishLoading(const GURL& url);
> On 2012/02/14 03:13:51, Jay Civelli wrote:
> > On 2012/02/14 01:20:31, cbentzel wrote:
> > > const?
> > Done.
> > 
> > > The comment about "Needs to be called" is also incorrect for non-Android
> > > platforms. 
> > I am not sure I understand why this is incorrect for non-Android. Once the
> > prerendered tab has been swapped, the prerender manager does not know
anything
> > about it so cannot be queried for that specific URL.
> >  
> 
> Ah, misinterpretation on my part. I thought you meant that
> MaybeUsePrerenderedPage depended on DidPrerenderFinishLoading being called
> before it, not that if DidPrerenderFinishLoading is called, it must be done
> prior to MaybeUsePrerenderedPage.
> 
> > > What's the rationale for not swapping in a partially-loaded prerendered
page
> > in
> > > on Android? Visual noise? 
> > We do swap in that case.
> > It is just that Chrome for Android needs to send a notification that the
page
> > has finished loading  (mostly for tests).
> > If the page has been fully prerendered, we never get that notification, so
we
> > use this method to check for that. 
> >

[dominich, 2012-02-14 16:38:59]

Seeing no difference from the instant version is great, but do confirm that
you're not dropping the CPU priority for foreground tabs. Also, once this is
checked in, you can check the FirstAfterMiss UMA data to determine any costs
being incurred.

As an aside - do you keep the field trials the same as trunk? Ie, is there a
control group running?

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/9387015/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:175: bool
DidPrerenderFinishLoading(const GURL& url);
On 2012/02/14 01:20:31, cbentzel wrote:
> const?
> 
> The comment about "Needs to be called" is also incorrect for non-Android
> platforms. 
> 
> What's the rationale for not swapping in a partially-loaded prerendered page
in
> on Android? Visual noise? 

It will still be swapped, but this lets the Android UI know to show a progress
spinner if loading is continuing.

http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:173: // finished loading. Needs to
be called before MaybeUsePrerenderedPage.
Maybe reword this to "Only valid if called before..." to clear up the
misunderstanding.

[Jay Civelli, 2012-02-14 17:37:21]

I don't think we have to worry about the process priority getting lowered. In
render_process_host_impl.cc WidgetHidden() it keeps track of how many widget are
hidden and only lower the process priority if all are hidden. Since we have a
non hidden foreground tab, it should not happen.

Regarding the field trial, I am setting explicitly the command line flag, so it
should always be on.

http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:173: // finished loading. Needs to
be called before MaybeUsePrerenderedPage.
On 2012/02/14 16:39:00, dominich wrote:
> Maybe reword this to "Only valid if called before..." to clear up the
> misunderstanding.

Done.

[dominich, 2012-02-14 17:41:50]

On 2012/02/14 17:37:21, Jay Civelli wrote:
> I don't think we have to worry about the process priority getting lowered. In
> render_process_host_impl.cc WidgetHidden() it keeps track of how many widget
are
> hidden and only lower the process priority if all are hidden. Since we have a
> non hidden foreground tab, it should not happen.

Which means that your foreground tab and prerender tab will be fighting for CPU
time. This might have a negative effect but we can track that with UMA and make
sure there's no problem. <1% of prerenders are canceled due to having too many
processes, so maybe you should keep that in and monitor it to see if it's
acceptable.

> 
> Regarding the field trial, I am setting explicitly the command line flag, so
it
> should always be on.

It is good to allow the field trial to run so you have control data for timings.
If you need a version for demoing, you can enable the flag for a given profile.

> 
>
http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_manager.h (right):
> 
>
http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_manager.h:173: // finished loading. Needs
to
> be called before MaybeUsePrerenderedPage.
> On 2012/02/14 16:39:00, dominich wrote:
> > Maybe reword this to "Only valid if called before..." to clear up the
> > misunderstanding.
> 
> Done.

[cbentzel, 2012-02-14 17:58:18]

On Tue, Feb 14, 2012 at 12:37 PM, <jcivelli@chromium.org> wrote:

> I don't think we have to worry about the process priority getting lowered.
> In
> render_process_host_impl.cc WidgetHidden() it keeps track of how many
> widget are
> hidden and only lower the process priority if all are hidden. Since we
> have a
> non hidden foreground tab, it should not happen.
>

Great.

LGTM, just so I don't block this going in. Dominic should still have the
last word.


>
> Regarding the field trial, I am setting explicitly the command line flag,
> so it
> should always be on.
>
>
>
> http://codereview.chromium.**org/9387015/diff/3005/chrome/**
>
browser/prerender/prerender_**manager.h<http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/prerender_manager.h>
> File chrome/browser/prerender/**prerender_manager.h (right):
>
> http://codereview.chromium.**org/9387015/diff/3005/chrome/**
>
browser/prerender/prerender_**manager.h#newcode173<http://codereview.chromium.org/9387015/diff/3005/chrome/browser/prerender/prerender_manager.h#newcode173>
> chrome/browser/prerender/**prerender_manager.h:173: // finished loading.
> Needs to be called before MaybeUsePrerenderedPage.
> On 2012/02/14 16:39:00, dominich wrote:
>
>> Maybe reword this to "Only valid if called before..." to clear up the
>> misunderstanding.
>>
>
> Done.
>
>
http://codereview.chromium.**org/9387015/<http://codereview.chromium.org/9387...
>

[dominich, 2012-02-14 18:35:30]

lgtm

[commit-bot: I haz the power, 2012-02-15 22:58:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/9387015/9001

[commit-bot: I haz the power, 2012-02-16 01:18:01]

Change committed as 122209