customtabs: Extract a redirect endpoint, and maybe connect to it.

customtabs: Extract a redirect endpoint, and maybe connect to it.

When an intent is received in Custom Tabs, extract a redirect endpoint
from an extra. If it exists, the URL to load is first party with respect
to the calling app, preconnect to the redirect endpoint.

This is part of Leapfrog, see the bug.

Also, only disables preconnection with the data reduction proxy for HTTP
URLs, as HTTPS URLs don't go through the proxy.

BUG=739165
Review-Url: https://codereview.chromium.org/2970003003
Cr-Commit-Position: refs/heads/master@{#485295}
Committed: https://chromium.googlesource.com/chromium/src/+/c0c7a5551424c67edbd4439c9da6a03c12295ad9

[Benoit L, 2017-07-04 17:02:19]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-04 17:02:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 18:57:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-04 18:57:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Benoit L, 2017-07-05 09:07:10]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 09:07:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 09:50:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 09:50:59]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-07-05 16:17:44]

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

[Yusuf, 2017-07-06 06:34:42]

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
(right):

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: &&
TextUtils.equals("http", uri.normalizeScheme().getScheme())) {
UrlConstants.HTTP_SCHEME

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

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:122:
static final String REDIRECT_ENDPOINT_KEY =
"android.support.customtabs.REDIRECT_ENDPOINT";
some todo around moving this to support lib?

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:750:
Uri redirectEndpoint = intent.getParcelableExtra(REDIRECT_ENDPOINT_KEY);
does it make sense to have this logic be in CustomTabIntentDataProvider?

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:755:
if (!mClientManager.isFirstPartyOriginForSession(session, Uri.parse(origin)))
return;
Going back all the way to how we handle isValidOrigin, since this is independent
of current navigation and best effort, why not try to verify async as well? That
way we dont have to wait for the verifyOrigin support lib part to come to use
this, it would mostly improve and move that network request to earlier. Wdyt?

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:758:
Profile.getLastUsedProfile(), redirectEndpoint.toString());
getLastUsedProfile().getOriginalProfile() to avoid incognito.

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

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:60:
private class VerifiedCallback implements Runnable {
Thanks!

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:102:
static boolean isValidOrigin(String packageName, Uri origin) {
javadoc

Maybe something around no async verification happening in this?

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(right):

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:118:
public boolean hasNativeInitializationCompleted() {
hasAppLevelNativeInitializationCompleted? since Activity level native init might
still be going on/

[Benoit L, 2017-07-07 11:30:56]

Thanks!

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
(right):

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: &&
TextUtils.equals("http", uri.normalizeScheme().getScheme())) {
On 2017/07/06 06:34:41, Yusuf wrote:
> UrlConstants.HTTP_SCHEME

Done.

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

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:122:
static final String REDIRECT_ENDPOINT_KEY =
"android.support.customtabs.REDIRECT_ENDPOINT";
On 2017/07/06 06:34:42, Yusuf wrote:
> some todo around moving this to support lib?

Done.

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:750:
Uri redirectEndpoint = intent.getParcelableExtra(REDIRECT_ENDPOINT_KEY);
On 2017/07/06 06:34:41, Yusuf wrote:
> does it make sense to have this logic be in CustomTabIntentDataProvider?

This talks to ClientManager, and also I find it convenient to keep all the
"optimizations" in the same location.
wdyt?

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:755:
if (!mClientManager.isFirstPartyOriginForSession(session, Uri.parse(origin)))
return;
On 2017/07/06 06:34:41, Yusuf wrote:
> Going back all the way to how we handle isValidOrigin, since this is
independent
> of current navigation and best effort, why not try to verify async as well?
That
> way we dont have to wait for the verifyOrigin support lib part to come to use
> this, it would mostly improve and move that network request to earlier. Wdyt?

That would be a neat solution indeed.

However it would also likely cause complications, as in this case we would issue
the domain verification request at the same time as the navigations, and
essentially have a "race" between walking the redirects and getting the
validation of the origin.

If the two servers as as fast, then we don't gain anything, and end up with an
unused socket. Otherwise we might still see benefits.

I would err on the side of not doing it for now, as this would simplify metrics.
But I'm not firm on that.

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:758:
Profile.getLastUsedProfile(), redirectEndpoint.toString());
On 2017/07/06 06:34:41, Yusuf wrote:
> getLastUsedProfile().getOriginalProfile() to avoid incognito.

Thanks!

Done.

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

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:102:
static boolean isValidOrigin(String packageName, Uri origin) {
On 2017/07/06 06:34:42, Yusuf wrote:
> javadoc
> 
> Maybe something around no async verification happening in this?

Done.

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(right):

https://codereview.chromium.org/2970003003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:118:
public boolean hasNativeInitializationCompleted() {
On 2017/07/06 06:34:42, Yusuf wrote:
> hasAppLevelNativeInitializationCompleted? since Activity level native init
might
> still be going on/

The whole call is ChromeBrowserInitializer.hasNativeInitializationCompleted(),
isn't it explicit enough?

[Yusuf, 2017-07-10 05:44:06]

lgtm

[Benoit L, 2017-07-10 09:00:45]

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

[Benoit L, 2017-07-10 09:00:46]

+bauerb
for
chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java


PTAL, thanks!

[Bernhard Bauer, 2017-07-10 12:13:45]

lgtm

https://codereview.chromium.org/2970003003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
(right):

https://codereview.chromium.org/2970003003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: &&
TextUtils.equals(UrlConstants.HTTP_SCHEME, uri.normalizeScheme().getScheme())) {
Nit: TextUtils.equals feels like overkill here -- both are strings, and you know
that HTTP_SCHEME is non-null.

[Benoit L, 2017-07-10 12:35:39]

Thanks!

https://codereview.chromium.org/2970003003/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
(right):

https://codereview.chromium.org/2970003003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: &&
TextUtils.equals(UrlConstants.HTTP_SCHEME, uri.normalizeScheme().getScheme())) {
On 2017/07/10 12:13:44, Bernhard Bauer wrote:
> Nit: TextUtils.equals feels like overkill here -- both are strings, and you
know
> that HTTP_SCHEME is non-null.

Done.

[Benoit L, 2017-07-10 12:35:51]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2017-07-10 12:35:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2970003003/#ps120001
(title: "Address comment.")

[commit-bot: I haz the power, 2017-07-10 12:35:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 14:36:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 14:36:33]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Benoit L, 2017-07-10 15:58:10]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2017-07-10 15:58:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2970003003/#ps140001
(title: "Typo.")

[commit-bot: I haz the power, 2017-07-10 15:58:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 16:47:42]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499702290428210,
"parent_rev": "aeff4fbf357e9bf0756e72d35eb74ee607a4d568", "commit_rev":
"c0c7a5551424c67edbd4439c9da6a03c12295ad9"}

[commit-bot: I haz the power, 2017-07-10 16:47:54]

Description was changed from

==========
customtabs: Extract a redirect endpoint, and maybe connect to it.

When an intent is received in Custom Tabs, extract a redirect endpoint
from an extra. If it exists, the URL to load is first party with respect
to the calling app, preconnect to the redirect endpoint.

This is part of Leapfrog, see the bug.

Also, only disables preconnection with the data reduction proxy for HTTP
URLs, as HTTPS URLs don't go through the proxy.

BUG=739165
==========

to

==========
customtabs: Extract a redirect endpoint, and maybe connect to it.

When an intent is received in Custom Tabs, extract a redirect endpoint
from an extra. If it exists, the URL to load is first party with respect
to the calling app, preconnect to the redirect endpoint.

This is part of Leapfrog, see the bug.

Also, only disables preconnection with the data reduction proxy for HTTP
URLs, as HTTPS URLs don't go through the proxy.

BUG=739165

Review-Url: https://codereview.chromium.org/2970003003
Cr-Commit-Position: refs/heads/master@{#485295}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0c7a5551424c67edbd4439c9da6...
==========

[commit-bot: I haz the power, 2017-07-10 16:47:57]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/c0c7a5551424c67edbd4439c9da6...