ntp4: improved app install, try 2

chrome/browser/resources/ntp4/new_tab.js

Index: chrome/browser/resources/ntp4/new_tab.js
diff --git a/chrome/browser/resources/ntp4/new_tab.js b/chrome/browser/resources/ntp4/new_tab.js
index df176032abc0cfbf2bcb33893f356f5d0b61b31e..1f1a5f500cc8ad61af9da3d244cf484c160bbb6a 100644
--- a/chrome/browser/resources/ntp4/new_tab.js
+++ b/chrome/browser/resources/ntp4/new_tab.js
@@ -347,11 +347,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 +362,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;

[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.

   }
 
   /**
@@ -775,6 +780,7 @@ cr.define('ntp4', function() {
     isRTL: isRTL,
     leaveRearrangeMode: leaveRearrangeMode,
     saveAppPageName: saveAppPageName,
+    setAppToBeHighlighted: setAppToBeHighlighted,
     setBookmarksData: setBookmarksData,
     setMostVisitedPages: setMostVisitedPages,
     setRecentlyClosedTabs: setRecentlyClosedTabs,