ntp4: app install improvements

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 09f2438b87d06fac99c807a739ff6de3151889ba..b97c925254283190518c3858618ffd95a732307a 100644
--- a/chrome/browser/resources/ntp4/new_tab.js
+++ b/chrome/browser/resources/ntp4/new_tab.js
@@ -317,14 +317,14 @@ cr.define('ntp4', function() {
 
     ntp4.AppsPage.setPromo(data.showPromo ? data : null);
 
-    // Tell the slider about the pages
+    // Tell the slider about the pages.
     updateSliderCards();
 
     if (highlightApp)
-      appAdded(highlightApp);
+      appAdded(highlightApp, true);
 
-    // Mark the current page
-    dots[cardSlider.currentCard].classList.add('selected');
+    // Mark the current page.
+    cardSlider.currentCardValue.navigationDot.classList.add('selected');

[Rick Byers, 2011/08/23 20:52:13]

Looks like you're not using the 'dots' list anymore, remove it.

[Evan Stade, 2011/08/25 03:08:56]

Done.

     logEvent('apps.layout: ' + (Date.now() - startTime));
   }
 
@@ -333,10 +333,27 @@ cr.define('ntp4', function() {
    * @param {Object} app A data structure full of relevant information for the
    *     app.
    */
-  function appAdded(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 hash = location.hash;
+    if (hash && hash.indexOf('#app-id=') == 0 && app.id == hash.split('=')[1]) {

[Rick Byers, 2011/08/23 20:52:13]

the logic to parse the hash is duplicated in initialize.  I suggest adding a
little helper function to reduce/eliminate the duplication (eg.
getAndClearHashAppId - but it'll be a little more complicated than that if you
want to avoid unconditionally clearing it here).

[Evan Stade, 2011/08/25 03:08:56]

Done.

+      window.history.replaceState({}, '', '/');
+      opt_highlight = true;
+    }
+
     var pageIndex = app.page_index || 0;
-    var page = appsPages[pageIndex];
-    cardSlider.selectCardByValue(page);
+    var page;
+    if (pageIndex < appsPages.length) {
+      page = appsPages[pageIndex];
+    } else {
+      page = new ntp4.AppsPage();

[Rick Byers, 2011/08/23 20:52:13]

is it not possible for page_index to be more than 1 greater than the number of
current pages?  Eg., what if all apps are removed from some middle page?  I'd
suggest instead looping here until you know you have enough pages...

[Evan Stade, 2011/08/25 03:08:56]

Done.

+      appendAppsPage(page, '');
+      updateSliderCards();
+    }
+
+    if (opt_highlight)
+      cardSlider.selectCardByValue(page);
     page.appendApp(app, true);
   }
 
@@ -399,12 +416,14 @@ cr.define('ntp4', function() {
       pageArray[i] = tilePages[i];
     cardSlider.setCards(pageArray, pageNo);
 
-    if (shownPage == templateData['most_visited_page_id'])
+    if (shownPage == templateData['most_visited_page_id']) {
       cardSlider.selectCardByValue(mostVisitedPage);
-    else if (shownPage == templateData['apps_page_id'])
-      cardSlider.selectCardByValue(appsPages[shownPageIndex]);
-    else if (shownPage == templateData['bookmarks_page_id'])
+    } else if (shownPage == templateData['apps_page_id']) {
+      cardSlider.selectCardByValue(
+          appsPages[Math.min(shownPageIndex, appsPages.length - 1)]);
+    } else if (shownPage == templateData['bookmarks_page_id']) {
       cardSlider.selectCardByValue(bookmarksPage);
+    }
   }
 
   /**
@@ -487,7 +506,7 @@ cr.define('ntp4', function() {
   function leaveRearrangeMode(e) {
     var tempPage = document.querySelector('.tile-page.temporary');
     var dot = tempPage.navigationDot;
-    if (!tempPage.tileCount) {
+    if (!tempPage.tileCount && tempPage != cardSlider.currentCardValue) {
       dot.animateRemove();
       tempPage.parentNode.removeChild(tempPage);
       updateSliderCards();
@@ -628,8 +647,7 @@ cr.define('ntp4', function() {
     var curDot = dotList.getElementsByClassName('selected')[0];
     if (curDot)
       curDot.classList.remove('selected');
-    var newPageIndex = e.cardSlider.currentCard;
-    dots[newPageIndex].classList.add('selected');
+    page.navigationDot.classList.add('selected');
     updatePageSwitchers();
   }