ntp4: improved app install, try 2

1. install to the correct page (0 -> -1 in crx_installer)
2. scroll to show the app (scrollTop change in apps_page.js)
3. reset tiles after trashing an app (trash.js change)
4. fix install-display for already-open NTPs. The old technique of always navigating stacked up history entries when the NTP had already been open. It was also flaky (seemed to be vulnerable to some race). Now we use an install notification which the NTP in question will be listening for.

BUG=92995,91583
TEST=manual

Review URL: http://codereview.chromium.org/7776001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@98715 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/crx_installer.cc

@@ -120,7 +120,7 @@ CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak,
       delete_source_(false),
       is_gallery_install_(false),
       create_app_shortcut_(false),
-      page_index_(0),
+      page_index_(-1),
       frontend_weak_(frontend_weak),
       client_(client),
       apps_require_extension_mime_type_(false),

chrome/browser/extensions/extension_install_ui.cc

@@ -231,12 +231,14 @@ void ExtensionInstallUI::OnImageLoaded(
 // static
 void ExtensionInstallUI::OpenAppInstalledNTP(Browser* browser,
                                              const std::string& app_id) {
-  std::string url = base::StringPrintf(
-      "%s#app-id=%s", chrome::kChromeUINewTabURL, app_id.c_str());
   browser::NavigateParams params =
-      browser->GetSingletonTabNavigateParams(GURL(url));
-  params.path_behavior = browser::NavigateParams::IGNORE_AND_NAVIGATE;
+      browser->GetSingletonTabNavigateParams(GURL(chrome::kChromeUINewTabURL));
   browser::Navigate(&params);
+
+  NotificationService::current()->Notify(
+      chrome::NOTIFICATION_APP_INSTALLED_TO_NTP,
+      Source<TabContents>(params.target_contents->tab_contents()),
+      Details<const std::string>(&app_id));
 }
 
 // static

chrome/browser/resources/ntp4/apps_page.js

@@ -424,6 +424,12 @@ cr.define('ntp4', function() {
      * @param {?boolean} animate If true, the app tile plays an animation.
      */
     appendApp: function(appData, animate) {
+      if (animate) {
+        // Select the page and scroll all the way down so the animation is
+        // visible.
+        ntp4.getCardSlider().selectCardByValue(this);
+        this.content_.scrollTop = this.content_.scrollHeight;
+      }
       this.appendTile(new App(appData), animate);
     },
 

chrome/browser/resources/ntp4/new_tab.js

@@ -137,10 +137,6 @@ cr.define('ntp4', function() {
     shownPage = templateData['shown_page_type'];
     shownPageIndex = templateData['shown_page_index'];
 
-    // When a new app has been installed, we will be opened with a hash value
-    // that corresponds to the new app ID.
-    highlightAppId = getAndClearAppIDHash();
-
     // Request data on the apps so we can fill them in.
     // Note that this is kicked off asynchronously.  'getAppsCallback' will be
     // invoked at some point after this function returns.
@@ -233,23 +229,6 @@ cr.define('ntp4', function() {
     return element;
   }
 
-  /**
-   * Gets the app ID stored in the hash of the URL, and resets the hash to
-   * empty. If there is not an app-id in the hash, does nothing.
-   * @return {String} The value of the app-id query string parameter.
-   */
-  function getAndClearAppIDHash() {
-    var hash = location.hash;
-    if (hash.indexOf('#app-id=') == 0) {
-      var appID = hash.split('=')[1];
-      // Clear the hash so if the user bookmarks this page, they'll just get
-      // chrome://newtab/.
-      window.history.replaceState({}, '', '/');
-      return appID;
-    }
-    return '';
-  }
-
   /**
    * Callback invoked by chrome with the apps available.
    *
@@ -318,12 +297,10 @@ cr.define('ntp4', function() {
         assert(appsPages.length == origPageCount + 1, 'expected new page');
       }
 
-      if (app.id == highlightAppId) {
+      if (app.id == highlightAppId)
         highlightApp = app;
-        highlightAppId = null;
-      } else {
+      else
         appsPages[pageIndex].appendApp(app);
-      }
     }
 
     ntp4.AppsPage.setPromo(data.showPromo ? data : null);
@@ -347,11 +324,10 @@ cr.define('ntp4', function() {
    *     app.
    */
   function appAdded(app, opt_highlight) {
-    // If the page is already open when a new app is installed, the hash will
-    // be set once again.
-    var appID = getAndClearAppIDHash();
-    if (appID == app.id)
+    if (app.id == highlightAppId) {
       opt_highlight = true;
+      highlightAppId = null;
+    }
 
     var pageIndex = app.page_index || 0;
 
@@ -363,9 +339,15 @@ cr.define('ntp4', function() {
     }
 
     var page = appsPages[pageIndex];
-    if (opt_highlight)
-      cardSlider.selectCardByValue(page);
-    page.appendApp(app, true);
+    page.appendApp(app, opt_highlight);
+  }
+
+  /**
+   * Sets that an app should be highlighted if it is added. Called right before
+   * appAdded for new installs.
+   */
+  function setAppToBeHighlighted(appId) {
+    highlightAppId = appId;
   }
 
   /**
@@ -808,6 +790,7 @@ cr.define('ntp4', function() {
     isRTL: isRTL,
     leaveRearrangeMode: leaveRearrangeMode,
     saveAppPageName: saveAppPageName,
+    setAppToBeHighlighted: setAppToBeHighlighted,
     setBookmarksData: setBookmarksData,
     setMostVisitedPages: setMostVisitedPages,
     setRecentlyClosedTabs: setRecentlyClosedTabs,

chrome/browser/resources/ntp4/trash.js

@@ -63,8 +63,10 @@ cr.define('ntp4', function() {
       e.preventDefault();
 
       var tile = ntp4.getCurrentlyDraggingTile();
+      var page = tile.tilePage;
       tile.firstChild.removeFromChrome();
       tile.landedOnTrash = true;
+      page.cleanupDrag();
     },
 
     /**

chrome/browser/ui/browser.cc

@@ -1997,7 +1997,7 @@ void Browser::OpenBookmarkManager() {
   UserMetrics::RecordAction(UserMetricsAction("ShowBookmarkManager"));
   UserMetrics::RecordAction(UserMetricsAction("ShowBookmarks"));
   ShowSingletonTabOverwritingNTP(
-    GetSingletonTabNavigateParams(GURL(chrome::kChromeUIBookmarksURL)));
+      GetSingletonTabNavigateParams(GURL(chrome::kChromeUIBookmarksURL)));
 }
 
 void Browser::OpenBookmarkManagerWithHash(const std::string& action,

chrome/browser/ui/webui/ntp/app_launcher_handler.cc

@@ -75,7 +75,8 @@ AppLauncherHandler::AppLauncherHandler(ExtensionService* extension_service)
     : extension_service_(extension_service),
       promo_active_(false),
       ignore_changes_(false),
-      attempted_bookmark_app_install_(false) {
+      attempted_bookmark_app_install_(false),
+      has_loaded_apps_(false) {
 }
 
 AppLauncherHandler::~AppLauncherHandler() {}
@@ -221,6 +222,8 @@ bool AppLauncherHandler::HandlePing(Profile* profile, const std::string& path) {
 }
 
 WebUIMessageHandler* AppLauncherHandler::Attach(WebUI* web_ui) {
+  registrar_.Add(this, chrome::NOTIFICATION_APP_INSTALLED_TO_NTP,
+      Source<TabContents>(web_ui->tab_contents()));
   return WebUIMessageHandler::Attach(web_ui);
 }
 
@@ -269,6 +272,15 @@ void AppLauncherHandler::Observe(int type,
       web_ui_->CallJavascriptFunction("appNotificationChanged", args);
       break;
     }
+    case chrome::NOTIFICATION_APP_INSTALLED_TO_NTP: {
+      if (!NewTabUI::NTP4Enabled())
+        break;
+
+      highlight_app_id_ = *Details<const std::string>(details).ptr();
+      if (has_loaded_apps_)
+        SetAppToBeHighlighted();
+      break;
+    }
     case chrome::NOTIFICATION_EXTENSION_LOADED: {
       const Extension* extension = Details<const Extension>(details).ptr();
       if (!extension->is_app())
@@ -463,12 +475,18 @@ void AppLauncherHandler::HandleGetApps(const ListValue* args) {
     ShownSectionsHandler::SetShownSection(prefs, THUMB);
   }
 
+  SetAppToBeHighlighted();
   FillAppDictionary(&dictionary);
   web_ui_->CallJavascriptFunction("getAppsCallback", dictionary);
 
   // First time we get here we set up the observer so that we can tell update
   // the apps as they change.
-  if (registrar_.IsEmpty()) {
+  if (!has_loaded_apps_) {
+    pref_change_registrar_.Init(
+        extension_service_->extension_prefs()->pref_service());
+    pref_change_registrar_.Add(ExtensionPrefs::kExtensionsPref, this);
+    pref_change_registrar_.Add(prefs::kNTPAppPageNames, this);
+
     registrar_.Add(this, chrome::NOTIFICATION_APP_NOTIFICATION_STATE_CHANGED,
         NotificationService::AllSources());
     registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
@@ -482,12 +500,8 @@ void AppLauncherHandler::HandleGetApps(const ListValue* args) {
     registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
         NotificationService::AllSources());
   }
-  if (pref_change_registrar_.IsEmpty()) {
-    pref_change_registrar_.Init(
-        extension_service_->extension_prefs()->pref_service());
-    pref_change_registrar_.Add(ExtensionPrefs::kExtensionsPref, this);
-    pref_change_registrar_.Add(prefs::kNTPAppPageNames, this);
-  }
+
+  has_loaded_apps_ = true;
 }
 
 void AppLauncherHandler::HandleLaunchApp(const ListValue* args) {
@@ -795,6 +809,15 @@ void AppLauncherHandler::OnFaviconForApp(FaviconService::Handle handle,
   attempted_bookmark_app_install_ = true;
 }
 
+void AppLauncherHandler::SetAppToBeHighlighted() {
+  if (highlight_app_id_.empty())
+    return;
+
+  scoped_ptr<StringValue> app_id(Value::CreateStringValue(highlight_app_id_));
+  web_ui_->CallJavascriptFunction("ntp4.setAppToBeHighlighted", *app_id);
+  highlight_app_id_.clear();
+}
+
 // static
 void AppLauncherHandler::RegisterUserPrefs(PrefService* pref_service) {
   // TODO(csilv): We will want this to be a syncable preference instead.
@@ -865,7 +888,7 @@ void AppLauncherHandler::PromptToEnableApp(const std::string& extension_id) {
     extension_service_->EnableExtension(extension_id);
 
     // Launch app asynchronously so the image will update.
-    StringValue* app_id = Value::CreateStringValue(extension_id);
+    scoped_ptr<StringValue> app_id(Value::CreateStringValue(extension_id));
     web_ui_->CallJavascriptFunction("launchAppAfterEnable", *app_id);
     return;
   }
@@ -915,7 +938,7 @@ void AppLauncherHandler::InstallUIProceed() {
   // If we don't launch the app asynchronously, then the app's disabled
   // icon disappears but isn't replaced by the enabled icon, making a poor
   // visual experience.
-  StringValue* app_id = Value::CreateStringValue(extension->id());
+  scoped_ptr<StringValue> app_id(Value::CreateStringValue(extension->id()));
   web_ui_->CallJavascriptFunction("launchAppAfterEnable", *app_id);
 
   extension_id_prompting_ = "";

chrome/browser/ui/webui/ntp/app_launcher_handler.h

@@ -163,6 +163,9 @@ class AppLauncherHandler : public WebUIMessageHandler,
   void OnFaviconForApp(FaviconService::Handle handle,
                        history::FaviconData data);
 
+  // Sends |highlight_app_id_| to the js.
+  void SetAppToBeHighlighted();
+
   // The apps are represented in the extensions model, which
   // outlives us since it's owned by our containing profile.
   ExtensionService* const extension_service_;
@@ -194,6 +197,14 @@ class AppLauncherHandler : public WebUIMessageHandler,
   // waiting to hear about success or failure from the extensions system.
   bool attempted_bookmark_app_install_;
 
+  // True if we have executed HandleGetApps() at least once.
+  bool has_loaded_apps_;
+
+  // The ID of the app to be highlighted on the NTP (i.e. shown on the page
+  // and pulsed). This is done for new installs. The actual higlighting occurs
+  // when the app is added to the page (via getAppsCallback or appAdded).
+  std::string highlight_app_id_;
+
   // Hold state for favicon requests.
   CancelableRequestConsumerTSimple<AppInstallInfo*> favicon_consumer_;
 

chrome/common/chrome_notification_types.h

@@ -841,6 +841,11 @@ enum {
   // Sent when the applications in the NTP app launcher have been reordered.
   NOTIFICATION_EXTENSION_LAUNCHER_REORDERED,
 
+  // Sent when an app is installed and an NTP has been shown. Source is the
+  // TabContents that was shown, and Details is the string ID of the extension
+  // which was installed.
+  NOTIFICATION_APP_INSTALLED_TO_NTP,
+
 #if defined(OS_CHROMEOS)
   // Sent when WebSocketProxy started accepting connections.
   NOTIFICATION_WEB_SOCKET_PROXY_STARTED,