customtabs: Pre-create a renderer in mayLaunchUrl().

customtabs: Pre-create a renderer in mayLaunchUrl().

When the client application indicates that it is likely to navigate to a
URL, we either pre-render it, or starting with this CL, pre-create a
renderer.

Renderer creation is relatively slow on Android, and even when a bound
service is already there, additional initialization is necessary to get
a usable renderer.

This commit makes the "spare" renderer navigate to about:blank, to force
a complete initialization of it. Testing on a Nexus 5 with Android 5.1.1
has shown ~200ms improvement with this change for the time to navigation
commit.

Most of the patch is plumbing to make sure that the "about:blank"
navigation doesn't get registered, so that a back navigation in a Custom
Tab doesn't go to about:blank.

BUG=511280
Committed: https://crrev.com/e8adeb87d8f923c541b93148a8c2149efd07811e
Cr-Commit-Position: refs/heads/master@{#344480}

[Benoit L, 2015-08-12 13:49:44]

lizeb@chromium.org changed reviewers:
+ davidben@chromium.org, pasko@chromium.org, yfriedman@chromium.org,
yusufo@chromium.org

[Benoit L, 2015-08-12 13:49:44]

Hello fellow Chromium-ers,

pasko, yusufo: customtabs/ (and generally FYI)
davidben: content/
yfriedman: */android/*

A bit of context:
This is a patch that pre-warms a renderer for Custom Tabs. The aim is to make
Custom Tabs faster than both WebView and Chrome even without prerendering.
It turns out that creating a renderer and navigating to about:blank ahead of
time yields sizable improvements. Median time from intent to commit goes from
837ms to 611ms on a Nexus 5 (see the bug and the document linked for the
results).

For context, rationale and experimental results, see:
https://docs.google.com/document/d/1Z7Gm5hr5nGoNx30So2rXFml-WKVuO0NFqNkrNKwpO...


This patch is fairly trivial, and most of it is plumbing to pass an extra
LoadUrlParams parameter from Java to native code.

Thanks!

[davidben, 2015-08-12 15:14:56]

davidben@chromium.org changed reviewers:
+ creis@chromium.org

[davidben, 2015-08-12 15:14:57]

Charlie's actually probably a better person than me to review this, even though
the actual code changes within content are just plumbing. +creis

On 2015/08/12 13:49:44, Benoit L wrote:
> Hello fellow Chromium-ers,
> 
> pasko, yusufo: customtabs/ (and generally FYI)
> davidben: content/
> yfriedman: */android/*
> 
> A bit of context:
> This is a patch that pre-warms a renderer for Custom Tabs. The aim is to make
> Custom Tabs faster than both WebView and Chrome even without prerendering.
> It turns out that creating a renderer and navigating to about:blank ahead of
> time yields sizable improvements. Median time from intent to commit goes from
> 837ms to 611ms on a Nexus 5 (see the bug and the document linked for the
> results).
> 
> For context, rationale and experimental results, see:
>
https://docs.google.com/document/d/1Z7Gm5hr5nGoNx30So2rXFml-WKVuO0NFqNkrNKwpO...

I don't seem to have access to the document.

> 
> This patch is fairly trivial, and most of it is plumbing to pass an extra
> LoadUrlParams parameter from Java to native code.
> 
> Thanks!

[davidben, 2015-08-12 15:19:34]

On 2015/08/12 15:14:57, David Benjamin wrote:
> > For context, rationale and experimental results, see:
> >
>
https://docs.google.com/document/d/1Z7Gm5hr5nGoNx30So2rXFml-WKVuO0NFqNkrNKwpO...
> 
> I don't seem to have access to the document.

(Nevermind, now it works. Weird.)

[Charlie Reis, 2015-08-12 16:50:48]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-08-12 17:04:29]

I've been chatting with Nasko about this, and the idea sounds fine but the
approach feels a bit wrong in principle.

Generally, creating a new WebContents and initializing it should give you a
renderer process with the initial about:blank page, which Blink automatically
knows to replace for the first navigation.  There shouldn't be a need to
explicitly navigate it to about:blank and deal with should_replace_current_entry
directly.  (That should mostly be an internal implementation detail to
content/.)

It's worth seeing if there's a simple way to get that initialization done for
you without the explicit about:blank navigation, or if this CL's approach is
needed in the short term.  I imagine we could take a look at what happens in
content_browsertests when creating a shell with no initial URL.  I'm busy today
but I can try to take a look tonight if that helps.

[Benoit L, 2015-08-13 15:58:31]

Thank you for the input, a few questions below.
First of all, I am not really familiar with content::, so please correct me.

On 2015/08/12 17:04:29, Charlie Reis (slow till 8-14) wrote:
> I've been chatting with Nasko about this, and the idea sounds fine but the
> approach feels a bit wrong in principle.
> 
> Generally, creating a new WebContents and initializing it should give you a
> renderer process with the initial about:blank page, which Blink automatically
> knows to replace for the first navigation.  There shouldn't be a need to
> explicitly navigate it to about:blank and deal with
should_replace_current_entry
> directly.  (That should mostly be an internal implementation detail to
> content/.)
> 

From the code, it looks to be the case, indeed. I will do benchmarks to see
whether it compares, if there is a way to initialize a WebContents from
"outside".

> It's worth seeing if there's a simple way to get that initialization done for
> you without the explicit about:blank navigation, or if this CL's approach is
> needed in the short term.  I imagine we could take a look at what happens in
> content_browsertests when creating a shell with no initial URL.  I'm busy
today
> but I can try to take a look tonight if that helps.

Currently, WebContents are created on Android from WebContentsFactory, in
chrome/browser/android/web_contents_factory.cc. It uses WebContents::Create(),
that returns a WebContents. And Init() is a member function of WebContentsImpl.
What would be the preferred way to call Init() from outside content:: ?

[davidben, 2015-08-14 17:52:44]

davidben@chromium.org changed reviewers:
- davidben@chromium.org

[Benoit L, 2015-08-17 16:45:38]

Sorry for the ping, but we would like this optimization to be in for M46, which
branches on August 21st.

Would it be possible to land this CL for M46 if we don't have a cleaner way to
do it by then, and move to a better solution for M47? As this CL is
self-contained and doesn't impact anything out of Custom Tabs (which is new, and
of which I am an owner), the likelihood of troubles ahead with a nasty
refactoring to "undo" this CL seems small.

What do you think?

[Charlie Reis, 2015-08-17 18:51:23]

On 2015/08/17 16:45:38, Benoit L wrote:
> Sorry for the ping, but we would like this optimization to be in for M46,
which
> branches on August 21st.
> 
> Would it be possible to land this CL for M46 if we don't have a cleaner way to
> do it by then, and move to a better solution for M47? As this CL is
> self-contained and doesn't impact anything out of Custom Tabs (which is new,
and
> of which I am an owner), the likelihood of troubles ahead with a nasty
> refactoring to "undo" this CL seems small.
> 
> What do you think?

I looked into the initialization question and it will require a bit of work in
the content/ side.  I've filed https://crbug.com/521729 for that.  We can land
this in the meantime, though I want to discourage others from using the same
pattern or relying on should_replace_current_entry.

content/ LGTM with nits.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:323:
* navigation to ensure that a back navigation doesn't lead to about:blank.
Maybe add a TODO to provide an initialization method that doesn't require the
about:blank navigation + should replace steps?  (See 521729.)

https://codereview.chromium.org/1232123009/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java:399:
* the current navigation entry.
Maybe add a TODO to remove these two methods once https://crbug.com/521729 is
fixed?  (I don't think we want to encourage more uses of these in general.)

[Yaron, 2015-08-17 19:31:39]

On 2015/08/17 18:51:23, Charlie Reis wrote:
> On 2015/08/17 16:45:38, Benoit L wrote:
> > Sorry for the ping, but we would like this optimization to be in for M46,
> which
> > branches on August 21st.
> > 
> > Would it be possible to land this CL for M46 if we don't have a cleaner way
to
> > do it by then, and move to a better solution for M47? As this CL is
> > self-contained and doesn't impact anything out of Custom Tabs (which is new,
> and
> > of which I am an owner), the likelihood of troubles ahead with a nasty
> > refactoring to "undo" this CL seems small.
> > 
> > What do you think?
> 
> I looked into the initialization question and it will require a bit of work in
> the content/ side.  I've filed https://crbug.com/521729 for that.  We can land
> this in the meantime, though I want to discourage others from using the same
> pattern or relying on should_replace_current_entry.
> 
> content/ LGTM with nits.
> 
>
https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
> (right):
> 
>
https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:323:
> * navigation to ensure that a back navigation doesn't lead to about:blank.
> Maybe add a TODO to provide an initialization method that doesn't require the
> about:blank navigation + should replace steps?  (See 521729.)
> 
>
https://codereview.chromium.org/1232123009/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java
> (right):
> 
>
https://codereview.chromium.org/1232123009/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java:399:
> * the current navigation entry.
> Maybe add a TODO to remove these two methods once https://crbug.com/521729 is
> fixed?  (I don't think we want to encourage more uses of these in general.)

all of the tab-plumbing lgtm. However, I'm not a good reviewer for the
customtabs changes. You should probably have yusufo or lizeb review those

[Yusuf, 2015-08-18 00:02:51]

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:147:
if (webContents != null) {
This will replace current entry even if we didn't take the spare web contents
but the prerendered one. I think that is not intended right? If prerendering
worked and created a web contents, we shouldn't be replacing the current entry.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:148:
// We got a spare renderer that has already navigated to
I think the javadoc for the above call makes this clear enough.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:301:
// is filtered by CanAddURLToHistory.
lets have a private createSpareWebContents below and add these comments as
javadoc to that

[Benoit L, 2015-08-18 12:04:24]

Thanks for the review!
Added the TODOs, and addressed yusufo's comments.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:147:
if (webContents != null) {
On 2015/08/18 00:02:51, Yusuf wrote:
> This will replace current entry even if we didn't take the spare web contents
> but the prerendered one. I think that is not intended right? If prerendering
> worked and created a web contents, we shouldn't be replacing the current
entry.

Thank you for spotting that!
Done.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:148:
// We got a spare renderer that has already navigated to
On 2015/08/18 00:02:50, Yusuf wrote:
> I think the javadoc for the above call makes this clear enough.

Done.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:301:
// is filtered by CanAddURLToHistory.
On 2015/08/18 00:02:51, Yusuf wrote:
> lets have a private createSpareWebContents below and add these comments as
> javadoc to that

Done.

https://codereview.chromium.org/1232123009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:323:
* navigation to ensure that a back navigation doesn't lead to about:blank.
On 2015/08/17 18:51:22, Charlie Reis wrote:
> Maybe add a TODO to provide an initialization method that doesn't require the
> about:blank navigation + should replace steps?  (See 521729.)

Done.

https://codereview.chromium.org/1232123009/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/1232123009/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java:399:
* the current navigation entry.
On 2015/08/17 18:51:22, Charlie Reis wrote:
> Maybe add a TODO to remove these two methods once https://crbug.com/521729 is
> fixed?  (I don't think we want to encourage more uses of these in general.)

Done.

[Benoit L, 2015-08-18 12:25:23]

lizeb@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Benoit L, 2015-08-18 12:25:24]

Hello sdefresne@chromium.org! Please review changes in
chrome/browser/history/android/android_provider_backend_unittest.cc

Thanks!

[sdefresne, 2015-08-18 12:34:29]

lgtm

[Yusuf, 2015-08-19 17:16:13]

https://codereview.chromium.org/1232123009/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1232123009/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:172:
if (webContents != null) mShouldReplaceCurrentEntry = true;
:( now if it was null at the beginning and is still null, we end up with a null
web contents and never create one.

[Benoit L, 2015-08-19 17:44:48]

https://codereview.chromium.org/1232123009/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java
(right):

https://codereview.chromium.org/1232123009/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTab.java:172:
if (webContents != null) mShouldReplaceCurrentEntry = true;
On 2015/08/19 17:16:13, Yusuf wrote:
> :( now if it was null at the beginning and is still null, we end up with a
null
> web contents and never create one.

Done.

[Yusuf, 2015-08-19 17:45:41]

lgtm

[Benoit L, 2015-08-20 09:59:56]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2015-08-20 09:59:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, creis@chromium.org, sdefresne@chromium.org,
yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/1232123009/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-08-20 10:00:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232123009/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232123009/100001

[commit-bot: I haz the power, 2015-08-20 12:18:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-20 12:18:48]

Patchset 6 (id:??) landed as
https://crrev.com/e8adeb87d8f923c541b93148a8c2149efd07811e
Cr-Commit-Position: refs/heads/master@{#344480}