Redirecting off-origin navigations in PWAs to CCT.

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java

Index: chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java
new file mode 100644
index 0000000000000000000000000000000000000000..9e16699cf98ae00845736c86bb90b01384ff2465
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInterceptNavigationDelegate.java
@@ -0,0 +1,89 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.webapps;
+
+import android.content.ComponentName;
+import android.net.Uri;
+import android.os.Bundle;
+import android.support.customtabs.CustomTabsCallback;
+import android.support.customtabs.CustomTabsClient;
+import android.support.customtabs.CustomTabsIntent;
+import android.support.customtabs.CustomTabsServiceConnection;
+import android.support.customtabs.CustomTabsSession;
+
+import org.chromium.chrome.browser.UrlConstants;
+import org.chromium.chrome.browser.tab.InterceptNavigationDelegateImpl;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.util.UrlUtilities;
+import org.chromium.components.navigation_interception.NavigationParams;
+
+import javax.annotation.Nullable;
+
+/**
+ * Intercepts navigations made by the Web App and sends off-origin http(s) ones to a Custom Tab.
+ */
+public class WebappInterceptNavigationDelegate extends InterceptNavigationDelegateImpl {
+    private final WebappActivity mActivity;
+    private CustomTabsClient mClient;
+
+    public WebappInterceptNavigationDelegate(WebappActivity activity, Tab tab) {
+        super(tab);
+        this.mActivity = activity;
+
+        CustomTabsServiceConnection connection = new CustomTabsServiceConnection() {

[Yusuf, 2017/05/03 17:56:06]

I dont think you actually need to maintain this at this point. The only benefit
you are getting is having a service connection, which could have been used to
warmup Chrome by other apps. But since we dont have to worry about that for
WebApps, we can just call the CustomTabsIntent related bits and not worry about
maintaining a connection and client.

[piotrs, 2017/05/03 22:58:18]

There are 2 reasons I went this way:
1) This allows me to bindCustomTabsService to the current implementation. We can
stay in the scope of the same implementation, so no point bothering the user
with a browser picker I think. Also it makes it much easier to test, as system
UI doesn't interrupt.
2) When I was trying out the code without CustomTabsClient on try bots, I was
always getting strict mode violations (policy=2655 violation=2) on
linux_android_rel_ng, and even wrapping these calls in
StrictMode.allowThreadDiskReads() was not helping.

Can I pick CCT implementation if I only use the intent? There's nothing in
CustomTabsIntent.Builder that seems to allow that.

[Yusuf, 2017/05/04 19:14:25]

once you build your CustomTabsIntent, CustomTabsIntent.intent is public and you
can call setPackageName on that one to our own package name.

Not really sure what is happening around StringModeViolations but using the
service definitely won't make you less susceptible to them. I would just have
the intent related bits here.

[piotrs, 2017/05/09 23:43:14]

Done as suggested.

+            @Override
+            public void onCustomTabsServiceConnected(
+                    ComponentName componentName, CustomTabsClient customTabsClient) {
+                mClient = customTabsClient;
+            }
+
+            @Override
+            public void onServiceDisconnected(ComponentName componentName) {
+                mClient = null;
+            }
+        };
+
+        CustomTabsClient.bindCustomTabsService(mActivity, mActivity.getPackageName(), connection);
+    }
+
+    @Nullable
+    private CustomTabsSession getSession() {
+        if (mClient == null) {
+            return null;
+        }
+        return mClient.newSession(new CustomTabsCallback() {

[Yusuf, 2017/05/03 17:56:05]

At some point, we might do more with each session and may need to use
CustomTabsSession etc, but at this point we dont.

[piotrs, 2017/05/03 22:58:18]

See reply to the comment above.

+            @Override
+            public void onNavigationEvent(int navigationEvent, Bundle extras) {
+                super.onNavigationEvent(navigationEvent, extras);

[Yusuf, 2017/05/03 17:56:05]

super does nothing. This part is not needed.

[piotrs, 2017/05/03 22:58:18]

Done, thanks for noticing.

+            }
+        });
+    }
+
+    @Override
+    public boolean shouldIgnoreNavigation(NavigationParams navigationParams) {
+        if (super.shouldIgnoreNavigation(navigationParams)) {
+            return true;
+        }
+
+        if (isHttpOrHttps(navigationParams.url)

[Yusuf, 2017/05/03 17:56:05]

Is this strictly http or https only? Data? File? About? Javascript not included?

Might be good to check with UrlUtilities.isAcceptedScheme or
UrlUtilities.isValidForIntentFallbackNavigation to see if any of them are what
you want rather than doing a one off here (things might diverge without someone
remembering the scheme restriction here).

I think your case actually do look like isValidForIntentFallbackNavigation

[piotrs, 2017/05/03 22:58:18]

I know this intent would crash if I pass data: URL (android cannot find suitable
component), it's what made me narrow down the protocol. I don't see why I would
want to pass file, about, or JS URLs to CCT, so I think HTTP+HTTPS is indeed the
only thing I want to allow.

And you are correct that isValidForIntentFallbackNavigation expresses exactly
what I need!

FYI added some more tests for isValidForIntentFallbackNavigation to specify its
behavior better in https://codereview.chromium.org/2855293002/.

[Yusuf, 2017/05/04 19:14:25]

Thanks.

+                && !UrlUtilities.sameDomainOrHost(
+                           mActivity.mWebappInfo.uri().toString(), navigationParams.url, true)) {
+            CustomTabsIntent.Builder intentBuilder = new CustomTabsIntent.Builder(getSession());
+            if (mActivity.mWebappInfo.hasValidThemeColor()) {
+                // Need to cast as themeColor is a long to contain possible error results.
+                intentBuilder.setToolbarColor((int) mActivity.mWebappInfo.themeColor());
+            }
+            intentBuilder.build().launchUrl(mActivity, Uri.parse(navigationParams.url));
+            return true;
+        }
+
+        return false;
+    }
+
+    private static boolean isHttpOrHttps(String url) {
+        return url.startsWith(UrlConstants.HTTP_URL_PREFIX)
+                || url.startsWith(UrlConstants.HTTPS_URL_PREFIX);
+    }
+}