Make the ShortcutHelper toast instead of send the user home for web app banners

* Fix null pointer access when a shortcut is added via
  a manifest.  It was relying on the WebContents() being
  accessible immediately after stopping the observation of
  the WebContents.

* When adding a shortcut on the homescreen through web app
  banners, stop firing Intents to send the user to the
  home. Instead show a toast to indicate that a shortcut is
  being added to their homescreen.  The string was chosen by UI
  and is described on the bug.

* Hopefully fix the ShortcutHelperTests.

BUG=457424,303486
TEST=ShortcutHelperTest

Review URL: https://codereview.chromium.org/934183002

Cr-Commit-Position: refs/heads/master@{#317079}

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

@@ -8,11 +8,16 @@ import android.app.ActivityManager;
 import android.content.Context;
 import android.content.Intent;
 import android.graphics.Bitmap;
+import android.os.Handler;
+import android.os.Looper;
 import android.text.TextUtils;
 import android.util.Base64;
 import android.util.Log;
+import android.widget.Toast;
 
+import org.chromium.base.ApplicationStatus;
 import org.chromium.base.CalledByNative;
+import org.chromium.chrome.R;
 import org.chromium.content_public.common.ScreenOrientationConstants;
 
 import java.io.ByteArrayOutputStream;
@@ -123,7 +128,8 @@ public class ShortcutHelper {
     @SuppressWarnings("unused")
     @CalledByNative
     private static void addShortcut(Context context, String url, String title, Bitmap icon,
-            int red, int green, int blue, boolean isWebappCapable, int orientation) {
+            int red, int green, int blue, boolean isWebappCapable, int orientation,
+            boolean returnToHomescreen) {
         assert sFullScreenAction != null;
 
         Intent shortcutIntent;
@@ -161,11 +167,26 @@ public class ShortcutHelper {
         context.sendBroadcast(BookmarkUtils.createAddToHomeIntent(context, shortcutIntent, title,
                 icon, red, green, blue));
 
-        // User is sent to the homescreen as soon as the shortcut is created.
-        Intent homeIntent = new Intent(Intent.ACTION_MAIN);
-        homeIntent.addCategory(Intent.CATEGORY_HOME);
-        homeIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-        context.startActivity(homeIntent);
+        // Alert the user about adding the shortcut.
+        if (returnToHomescreen) {
+            Intent homeIntent = new Intent(Intent.ACTION_MAIN);
+            homeIntent.addCategory(Intent.CATEGORY_HOME);
+            homeIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+            context.startActivity(homeIntent);
+        } else {
+            final String shortUrl = UrlUtilities.getDomainAndRegistry(url, true);
+            Handler handler = new Handler(Looper.getMainLooper());
+            handler.post(new Runnable() {
+                @Override
+                public void run() {
+                    Context applicationContext = ApplicationStatus.getApplicationContext();
+                    String toastText =
+                            applicationContext.getString(R.string.added_to_homescreen, shortUrl);
+                    Toast toast = Toast.makeText(applicationContext, toastText, Toast.LENGTH_SHORT);
+                    toast.show();
+                }
+            });
+        }
     }
 
     private native long nativeInitialize(long tabAndroidPtr);

chrome/android/java/strings/android_chrome_strings.grd

@@ -980,6 +980,9 @@ You are signing in with a managed account and giving its administrator control o
       <message name="IDS_MENU_ADD_TO_HOMESCREEN" desc="Menu item for adding a shortcut to the homescreen. [CHAR-LIMIT=27]">
         Add to homescreen
       </message>
+      <message name="IDS_ADDED_TO_HOMESCREEN" desc="Indicates that the current domain was added to the user's homescreen.">
+        <ph name="DOMAIN_NAME">%1$s<ex>google.com</ex></ph> was added to your homescreen
+      </message>
 
       <!-- WebsiteSettingsPopup (PageInfo dialog) -->
       <message name="IDS_PAGE_INFO_COPY_URL_BUTTON" desc="Text in the button that copies the URL to the clipboard.">

chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java

-// Copyright 2013 The Chromium Authors. All rights reserved.
+// Copyright 2015 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;
 
 import android.content.Intent;
-import android.test.FlakyTest;
+import android.test.suitebuilder.annotation.SmallTest;
+import android.text.TextUtils;
 
+import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.UrlUtils;
 import org.chromium.chrome.shell.ChromeShellActivity;
 import org.chromium.chrome.shell.ChromeShellApplication;
 import org.chromium.chrome.shell.ChromeShellApplicationObserver;
 import org.chromium.chrome.shell.ChromeShellTestBase;
+import org.chromium.chrome.test.util.browser.TabLoadObserver;
 import org.chromium.content.browser.test.util.Criteria;
 import org.chromium.content.browser.test.util.CriteriaHelper;
 
@@ -42,12 +45,13 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
             + "<head><title>" + NORMAL_TITLE + "</title></head>"
             + "<body>Not Webapp capable</body></html>");
 
+    private static final String META_APP_NAME_PAGE_TITLE = "Not the right title";
     private static final String META_APP_NAME_TITLE = "Web application-name";
     private static final String META_APP_NAME_HTML = UrlUtils.encodeHtmlDataUri(
             "<html><head>"
             + "<meta name=\"mobile-web-app-capable\" content=\"yes\" />"
             + "<meta name=\"application-name\" content=\"" + META_APP_NAME_TITLE + "\">"
-            + "<title>Not the right title</title>"
+            + "<title>" + META_APP_NAME_PAGE_TITLE + "</title>"
             + "</head><body>Webapp capable</body></html>");
 
     private static class TestObserver implements ChromeShellApplicationObserver {
@@ -74,6 +78,8 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
 
     @Override
     public void setUp() throws Exception {
+        super.setUp();
+
         ShortcutHelper.setFullScreenAction(WEBAPP_ACTION_NAME);
         mActivity = launchChromeShellWithBlankPage();
 
@@ -82,19 +88,13 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
         ChromeShellApplication application =
                 (ChromeShellApplication) mActivity.getApplication();
         application.addObserver(mTestObserver);
-
-        super.setUp();
     }
 
-    /**
-     * @MediumTest
-     * @Feature("{Webapp}")
-     * crbug.com/303486
-     */
-    @FlakyTest
+    @SmallTest
+    @Feature("{Webapp}")
     public void testAddWebappShortcuts() throws InterruptedException {
         // Add a webapp shortcut and make sure the intent's parameters make sense.
-        addShortcutToURL(WEBAPP_HTML, "");
+        addShortcutToURL(WEBAPP_HTML, WEBAPP_TITLE, "");
         Intent firedIntent = mTestObserver.mFiredIntent;
         assertEquals(WEBAPP_TITLE, firedIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
 
@@ -105,7 +105,7 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
 
         // Add a second shortcut and make sure it matches the second webapp's parameters.
         mTestObserver.reset();
-        addShortcutToURL(SECOND_WEBAPP_HTML, "");
+        addShortcutToURL(SECOND_WEBAPP_HTML, SECOND_WEBAPP_TITLE, "");
         Intent newFiredIntent = mTestObserver.mFiredIntent;
         assertEquals(SECOND_WEBAPP_TITLE,
                 newFiredIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
@@ -116,14 +116,10 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
         assertEquals(mActivity.getPackageName(), newLaunchIntent.getPackage());
     }
 
-    /**
-     * @MediumTest
-     * @Feature("{Webapp}")
-     * crbug.com/303486
-     */
-    @FlakyTest
+    @SmallTest
+    @Feature("{Webapp}")
     public void testAddBookmarkShortcut() throws InterruptedException {
-        addShortcutToURL(NORMAL_HTML, "");
+        addShortcutToURL(NORMAL_HTML, NORMAL_TITLE, "");
 
         // Make sure the intent's parameters make sense.
         Intent firedIntent = mTestObserver.mFiredIntent;
@@ -135,48 +131,45 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
         assertEquals(NORMAL_HTML, launchIntent.getDataString());
     }
 
-    /**
-     * @MediumTest
-     * @Feature("{Webapp}")
-     * crbug.com/303486
-     */
-    @FlakyTest
+    @SmallTest
+    @Feature("{Webapp}")
     public void testAddWebappShortcutsWithoutTitleEdit() throws InterruptedException {
-        // Add a webapp shortcut to check unedited title.
-        addShortcutToURL(WEBAPP_HTML, "");
+        // Add a webapp shortcut using the page's title.
+        addShortcutToURL(WEBAPP_HTML, WEBAPP_TITLE, "");
         Intent firedIntent = mTestObserver.mFiredIntent;
         assertEquals(WEBAPP_TITLE, firedIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
     }
 
-    /**
-     * @MediumTest
-     * @Feature("{Webapp}")
-     * crbug.com/303486
-     */
-    @FlakyTest
+    @SmallTest
+    @Feature("{Webapp}")
     public void testAddWebappShortcutsWithTitleEdit() throws InterruptedException {
-        // Add a webapp shortcut to check edited title.
-        addShortcutToURL(WEBAPP_HTML, EDITED_WEBAPP_TITLE);
+        // Add a webapp shortcut with a custom title.
+        addShortcutToURL(WEBAPP_HTML, WEBAPP_TITLE, EDITED_WEBAPP_TITLE);
         Intent firedIntent = mTestObserver.mFiredIntent;
-        assertEquals(EDITED_WEBAPP_TITLE , firedIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
+        assertEquals(EDITED_WEBAPP_TITLE, firedIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
     }
 
-    /**
-     * @MediumTest
-     * @Feature("{Webapp}")
-     * crbug.com/303486
-     */
-    @FlakyTest
+    @SmallTest
+    @Feature("{Webapp}")
     public void testAddWebappShortcutsWithApplicationName() throws InterruptedException {
-        // Add a webapp shortcut to check edited title.
-        addShortcutToURL(META_APP_NAME_HTML, "");
+        addShortcutToURL(META_APP_NAME_HTML, META_APP_NAME_PAGE_TITLE, "");
         Intent firedIntent = mTestObserver.mFiredIntent;
-        assertEquals(META_APP_NAME_TITLE , firedIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
+        assertEquals(META_APP_NAME_TITLE, firedIntent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME));
     }
 
-    private void addShortcutToURL(String url, final String title) throws InterruptedException {
-        loadUrlWithSanitization(url);
-        assertTrue(waitForActiveShellToBeDoneLoading());
+    private void addShortcutToURL(String url, final String expectedPageTitle, final String title)
+            throws InterruptedException {
+        final Tab activeTab = getActivity().getActiveTab();
+        TabLoadObserver observer = new TabLoadObserver(activeTab, url) {
+            @Override
+            public boolean isSatisfied() {
+                // The page title is often updated over several iterations.  Wait until the right
+                // one appears.
+                return super.isSatisfied()
+                        && TextUtils.equals(activeTab.getTitle(), expectedPageTitle);
+            }
+        };
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(observer));
 
         // Add the shortcut.
         getInstrumentation().runOnMainSync(new Runnable() {
@@ -184,8 +177,7 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
             public void run() {
                 final ShortcutHelper shortcutHelper = new ShortcutHelper(
                         mActivity.getApplicationContext(), mActivity.getActiveTab());
-                // Calling initialize() isn't strictly required but it is
-                // testing this code path.
+                // Calling initialize() isn't strictly required but it is testing this code path.
                 shortcutHelper.initialize(new ShortcutHelper.OnInitialized() {
                     @Override
                     public void onInitialized(String t) {
@@ -196,7 +188,7 @@ public class ShortcutHelperTest extends ChromeShellTestBase {
         });
 
         // Make sure that the shortcut was added.
-        assertTrue(CriteriaHelper.pollForCriteria(new Criteria() {
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
             @Override
             public boolean isSatisfied() {
                 return mTestObserver.mFiredIntent != null;

chrome/browser/android/banners/app_banner_infobar_delegate.cc

@@ -172,7 +172,8 @@ bool AppBannerInfoBarDelegate::Accept() {
         FROM_HERE,
         base::Bind(&ShortcutHelper::AddShortcutInBackgroundWithSkBitmap,
                    info,
-                   *app_icon_.get()),
+                   *app_icon_.get(),
+                   false),
         true);
 
     TrackInstallEvent(INSTALL_EVENT_WEB_APP_INSTALLED);

chrome/browser/android/shortcut_helper.cc

@@ -216,16 +216,17 @@ void ShortcutHelper::AddShortcut(
 }
 
 void ShortcutHelper::AddShortcutUsingManifestIcon() {
+  RecordAddToHomescreen();
+
   // Stop observing so we don't get destroyed while doing the last steps.
   Observe(NULL);
 
-  RecordAddToHomescreen();
-
   base::WorkerPool::PostTask(
       FROM_HERE,
       base::Bind(&ShortcutHelper::AddShortcutInBackgroundWithSkBitmap,
                  shortcut_info_,
-                 manifest_icon_),
+                 manifest_icon_,
+                 true),
       true);
 
   Destroy();
@@ -306,12 +307,13 @@ void ShortcutHelper::AddShortcutInBackgroundWithRawBitmap(
                           &icon_bitmap);
   }
 
-  AddShortcutInBackgroundWithSkBitmap(info, icon_bitmap);
+  AddShortcutInBackgroundWithSkBitmap(info, icon_bitmap, true);
 }
 
 void ShortcutHelper::AddShortcutInBackgroundWithSkBitmap(
     const ShortcutInfo& info,
-    const SkBitmap& icon_bitmap) {
+    const SkBitmap& icon_bitmap,
+    const bool return_to_homescreen) {
   DCHECK(base::WorkerPool::RunsTasksOnCurrentThread());
 
   SkColor color = color_utils::CalculateKMeanColorOfBitmap(icon_bitmap);
@@ -339,7 +341,8 @@ void ShortcutHelper::AddShortcutInBackgroundWithSkBitmap(
       g_value,
       b_value,
       info.display == content::Manifest::DISPLAY_MODE_STANDALONE,
-      info.orientation);
+      info.orientation,
+      return_to_homescreen);
 }
 
 void ShortcutHelper::RecordAddToHomescreen() {

chrome/browser/android/shortcut_helper.h

@@ -91,7 +91,8 @@ class ShortcutHelper : public content::WebContentsObserver {
   // Must be called from a WorkerPool task.
   static void AddShortcutInBackgroundWithSkBitmap(
       const ShortcutInfo& info,
-      const SkBitmap& icon_bitmap);
+      const SkBitmap& icon_bitmap,
+      const bool return_to_homescreen);
 
   // Registers JNI hooks.
   static bool RegisterShortcutHelper(JNIEnv* env);