Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut

chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java b/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
index ab323ca96c30f4f1505cdc19fb7d94f5fe19b0eb..389876914e6b949387df9107f182c439a3c27dad 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
@@ -34,7 +34,6 @@ import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
-import org.chromium.blink_public.platform.WebDisplayMode;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.webapps.WebApkBuilder;
 import org.chromium.chrome.browser.webapps.WebappAuthenticator;
@@ -132,49 +131,49 @@ public class ShortcutHelper {
     }
 
     /**
-     * Called when we have to fire an Intent to add a shortcut to the home screen.
-     * If the webpage indicated that it was capable of functioning as a webapp, it is added as a
-     * shortcut to a webapp Activity rather than as a general bookmark. User is sent to the
-     * home screen as soon as the shortcut is created.
-     *
-     * This method must not be called on the UI thread.
+     * Installs a WebAPK.

[dominickn, 2016/07/15 00:18:59]

Nit: "This method must not be called on the UI thread."

Asserts only trigger on KK and older devices so it's good to have an explicit
comment.

      */
     @SuppressWarnings("unused")
     @CalledByNative
-    private static void addShortcut(String id, String url, String scopeUrl,
+    private static void installWebApk(String url, String scopeUrl, final String name,
+            String shortName, String iconUrl, Bitmap icon, int displayMode, int orientation,
+            long themeColor, long backgroundColor, String manifestUrl) {
+        assert !ThreadUtils.runningOnUiThread();
+        Context context = ContextUtils.getApplicationContext();

[dominickn, 2016/07/15 00:18:59]

Super nit: newline after the assert for consistency :)

+        WebApkBuilder apkBuilder = ((ChromeApplication) context).createWebApkBuilder();
+        if (apkBuilder == null) {
+            // TODO(pkotwicz): Figure out what to do when building WebAPK fails. (crbug.com/626950)
+            return;
+        }
+
+        if (TextUtils.isEmpty(scopeUrl)) {
+            scopeUrl = GURLUtils.getOrigin(url);
+        }
+        apkBuilder.buildWebApkAsync(url, scopeUrl, name, shortName, iconUrl, icon, displayMode,
+                orientation, themeColor, backgroundColor, manifestUrl);
+    }
+
+    /**
+     * Adds home screen shortcut which opens in a {@link WebappActivity}.
+     */

[dominickn, 2016/07/15 00:18:59]

Nit: This method must not be called on the UI thread.

+    @SuppressWarnings("unused")
+    @CalledByNative
+    private static void addWebappShortcut(String id, String url, String scopeUrl,

[dominickn, 2016/07/15 00:18:59]

Rename: "addWebapp" as per comment in shortcut_helper.h

             final String userTitle, String name, String shortName, String iconUrl, Bitmap icon,
             int displayMode, int orientation, int source, long themeColor, long backgroundColor,
-            String manifestUrl, final long callbackPointer) {
+            final long callbackPointer) {
         assert !ThreadUtils.runningOnUiThread();
 
         Context context = ContextUtils.getApplicationContext();
-        final Intent shortcutIntent;
-        boolean isWebappCapable = (displayMode == WebDisplayMode.Standalone
-                                || displayMode == WebDisplayMode.Fullscreen);
-        if (isWebappCapable) {
-            if (CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_WEBAPK)) {
-                WebApkBuilder apkBuilder = ((ChromeApplication) context).createWebApkBuilder();
-                if (apkBuilder != null) {
-                    if (TextUtils.isEmpty(scopeUrl)) {
-                        scopeUrl = GURLUtils.getOrigin(url);
-                    }
-                    apkBuilder.buildWebApkAsync(url, scopeUrl, name, shortName, iconUrl, icon,
-                            displayMode, orientation, themeColor, backgroundColor, manifestUrl);
-                    return;
-                }
-            }
-            if (TextUtils.isEmpty(scopeUrl)) {
-                scopeUrl = getScopeFromUrl(url);
-            }
-            shortcutIntent = createWebappShortcutIntent(id, sDelegate.getFullscreenAction(), url,
-                    scopeUrl, name, shortName, icon, WEBAPP_SHORTCUT_VERSION, displayMode,
-                    orientation, themeColor, backgroundColor, iconUrl.isEmpty());
-            shortcutIntent.putExtra(EXTRA_MAC, getEncodedMac(context, url));
-        } else {
-            // Add the shortcut as a launcher icon to open in the browser Activity.
-            shortcutIntent = createShortcutIntent(url);
+        if (TextUtils.isEmpty(scopeUrl)) {
+            scopeUrl = getScopeFromUrl(url);

[dominickn, 2016/07/15 00:18:59]

This variable isn't used. Replace getScopeFromUrl(url) in the method call below
with scopeUrl.

         }
 
+        final Intent shortcutIntent = createWebappShortcutIntent(id,
+                sDelegate.getFullscreenAction(), url, getScopeFromUrl(url), name, shortName, icon,
+                WEBAPP_SHORTCUT_VERSION, displayMode, orientation, themeColor, backgroundColor,
+                iconUrl.isEmpty());
+        shortcutIntent.putExtra(EXTRA_MAC, getEncodedMac(context, url));
         // Always attach a source (one of add to home screen menu item, app banner, or unknown) to
         // the intent. This allows us to distinguish where a shortcut was added from in metrics.

[dominickn, 2016/07/15 00:18:59]

This source comment is duplicated - you can probably remove it from both
locations or shorten it to "The source distinguishes where a shortcut was added
from in metrics".

[pkotwicz, 2016/07/15 01:21:56]

Ok, removed

         shortcutIntent.putExtra(EXTRA_SOURCE, source);
@@ -182,19 +181,44 @@ public class ShortcutHelper {
         sDelegate.sendBroadcast(
                 context, createAddToHomeIntent(url, userTitle, icon, shortcutIntent));
 
-        if (isWebappCapable) {
-            // Store the webapp data so that it is accessible without the intent. Once this process
-            // is complete, call back to native code to start the splash image download.
-            WebappRegistry.registerWebapp(context, id,
-                    new WebappRegistry.FetchWebappDataStorageCallback() {
-                        @Override
-                        public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
-                            storage.updateFromShortcutIntent(shortcutIntent);
-                            nativeOnWebappDataStored(callbackPointer);
-                        }
+        // Store the webapp data so that it is accessible without the intent. Once this process
+        // is complete, call back to native code to start the splash image download.
+        WebappRegistry.registerWebapp(context, id,
+                new WebappRegistry.FetchWebappDataStorageCallback() {
+                    @Override
+                    public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
+                        storage.updateFromShortcutIntent(shortcutIntent);
+                        nativeOnWebappDataStored(callbackPointer);
                     }
-            );
-        }
+                });
+
+        showAddedToHomescreenToast(userTitle);
+    }
+
+    /**
+     * Adds home screen shortcut which opens in the browser Activity.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private static void addBookmarkShortcut(String url, String userTitle, Bitmap icon, int source) {

[dominickn, 2016/07/15 00:19:00]

Rename: "addShortcut" as per comment in shortcut_helper.h

+        assert !ThreadUtils.runningOnUiThread();
+
+        Context context = ContextUtils.getApplicationContext();
+        final Intent shortcutIntent = createShortcutIntent(url);
+        // Always attach a source (one of add to home screen menu item, app banner, or unknown) to
+        // the intent. This allows us to distinguish where a shortcut was added from in metrics.
+        shortcutIntent.putExtra(EXTRA_SOURCE, source);
+        shortcutIntent.setPackage(context.getPackageName());
+        sDelegate.sendBroadcast(
+                context, createAddToHomeIntent(url, userTitle, icon, shortcutIntent));
+        showAddedToHomescreenToast(userTitle);
+    }
+
+    /**
+     * Show toast to alert user that the shortcut was added to the home screen.
+     */
+    private static void showAddedToHomescreenToast(final String title) {
+        assert !ThreadUtils.runningOnUiThread();

[dominickn, 2016/07/15 00:18:59]

I don't think this method needs the assert - it doesn't seem to be doing
anything particularly expensive? Or does handler.post have to happen off the UI
thread?

[pkotwicz, 2016/07/15 01:21:56]

Removed the assert and changed the Handler logic to use
ThreadUtils#runOnUiThread() instead

 
         // Alert the user about adding the shortcut.

[dominickn, 2016/07/15 00:19:00]

Remove redundant comment now this is a method.

         Handler handler = new Handler(Looper.getMainLooper());
@@ -203,7 +227,7 @@ public class ShortcutHelper {
             public void run() {
                 Context applicationContext = ContextUtils.getApplicationContext();
                 String toastText =
-                        applicationContext.getString(R.string.added_to_homescreen, userTitle);
+                        applicationContext.getString(R.string.added_to_homescreen, title);
                 Toast toast = Toast.makeText(applicationContext, toastText, Toast.LENGTH_SHORT);
                 toast.show();
             }