ntp4: improved app install, try 2

chrome/browser/resources/ntp4/new_tab.js

 // Copyright (c) 2011 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.
 
 /**
  * @fileoverview New tab page
  * This is the main code for the new tab page used by touch-enabled Chrome
  * browsers.  For now this is still a prototype.
  */
 
@@ skipping 329 matching lines @@
 
     document.documentElement.classList.remove('starting-up');
   }
 
   /**
    * Called by chrome when a new app has been added to chrome.
    * @param {Object} app A data structure full of relevant information for the
    *     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;
 
     if (pageIndex >= appsPages.length) {
       while (pageIndex >= appsPages.length) {
         appendAppsPage(new ntp4.AppsPage(), '');
       }
       updateSliderCards();
     }
 
     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;

[Rick Byers, 2011/08/29 16:01:00]

Should you perhaps assert that highlightAppId is null?  Or is it possible that
we'd get two app installs in a row without an intervening getAppsCallback?  In
which case is it OK to just highlight the last?  Probably at least worth a
comment.  I assume it's not worth the extra complexity to keep a list and
highlight all in the list.

[Evan Stade, 2011/08/29 18:15:16]

yea I assume it's possible if unlikely to have a race condition with installing
multiple apps. And it's not just an intervening call to getAppsCallback, usually
after setAppToBeHighlighted there will be an appAdded call. I'm not sure what
would go into a comment, something like

// This should be null but it's harmless if we are overwriting something.

is in my mind adds verbosity but doesn't really explain anything subtle.

[Rick Byers, 2011/08/29 19:51:12]

Yeah, I guess the race potential and highlight-last behavior is fairly obvious
and OK not to dwell on.

+  }
+
+  /**
    * Called by chrome when an existing app has been removed/uninstalled from
    * chrome.
    * @param {Object} appData A data structure full of relevant information for
    *     the app.
    */
   function appRemoved(appData) {
     var app = $(appData.id);
     assert(app, 'trying to remove an app that doesn\'t exist');
 
     var tile = findAncestorByClass(app, 'tile');
@@ skipping 386 matching lines @@
     appsPrefChangeCallback: appsPrefChangeCallback,
     assert: assert,
     enterRearrangeMode: enterRearrangeMode,
     getAppsCallback: getAppsCallback,
     getAppsPageIndex: getAppsPageIndex,
     getCardSlider: getCardSlider,
     initialize: initialize,
     isRTL: isRTL,
     leaveRearrangeMode: leaveRearrangeMode,
     saveAppPageName: saveAppPageName,
+    setAppToBeHighlighted: setAppToBeHighlighted,
     setBookmarksData: setBookmarksData,
     setMostVisitedPages: setMostVisitedPages,
     setRecentlyClosedTabs: setRecentlyClosedTabs,
     showNotification: showNotification,
     themeChanged: themeChanged
   };
 });
 
 // publish ntp globals
 // TODO(estade): update the content handlers to use ntp namespace instead of
 // making these global.
 var assert = ntp4.assert;
 var getAppsCallback = ntp4.getAppsCallback;
 var appsPrefChangeCallback = ntp4.appsPrefChangeCallback;
 var themeChanged = ntp4.themeChanged;
 var recentlyClosedTabs = ntp4.setRecentlyClosedTabs;
 var setMostVisitedPages = ntp4.setMostVisitedPages;
 
 document.addEventListener('DOMContentLoaded', ntp4.initialize);