Default apps promo was getting expired early if NTP open.

This was a timing issue: we had a pref change observer that was
triggering refresh of NTP. Inside this refresh, we checked the
promo status. But setting up the pref tree of an extension
during installation was also triggering this listener.

BUG=64736
TEST=See bug

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

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

5 files changed, 52 insertions, 38 deletions

diff --git a/chrome/browser/dom_ui/app_launcher_handler.cc b/chrome/browser/dom_ui/app_launcher_handler.cc
index ec9aa965..a6f35fb 100644
--- a/chrome/browser/dom_ui/app_launcher_handler.cc
+++ b/chrome/browser/dom_ui/app_launcher_handler.cc
@@ -181,16 +181,6 @@ void AppLauncherHandler::FillAppDictionary(DictionaryValue* dictionary) {
   }
   dictionary->Set("apps", list);
 
-  DefaultApps* default_apps = extensions_service_->default_apps();
-  if (default_apps->ShouldShowPromo(extensions_service_->GetAppIds())) {
-    dictionary->SetBoolean("showPromo", true);
-    default_apps->DidShowPromo();
-    promo_active_ = true;
-  } else {
-    dictionary->SetBoolean("showPromo", false);
-    promo_active_ = false;
-  }
-
   bool showLauncher =
       CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAppLauncher);
   dictionary->SetBoolean("showLauncher", showLauncher);
@@ -205,6 +195,25 @@ void AppLauncherHandler::FillAppDictionary(DictionaryValue* dictionary) {
 void AppLauncherHandler::HandleGetApps(const ListValue* args) {
   DictionaryValue dictionary;
   FillAppDictionary(&dictionary);
+
+  // Tell the client whether to show the promo for this view. We don't do this
+  // in the case of PREF_CHANGED because:
+  //
+  // a) At that point in time, depending on the pref that changed, it can look
+  //    like the set of apps installed has changed, and we will mark the promo
+  //    expired.
+  // b) Conceptually, it doesn't really make sense to count a
+  //    prefchange-triggered refresh as a promo 'view'.
+  DefaultApps* default_apps = extensions_service_->default_apps();
+  if (default_apps->CheckShouldShowPromo(extensions_service_->GetAppIds())) {
+    dictionary.SetBoolean("showPromo", true);
+    default_apps->DidShowPromo();
+    promo_active_ = true;
+  } else {
+    dictionary.SetBoolean("showPromo", false);
+    promo_active_ = false;
+  }
+
   dom_ui_->CallJavascriptFunction(L"getAppsCallback", dictionary);
 
   // First time we get here we set up the observer so that we can tell update
diff --git a/chrome/browser/extensions/default_apps.cc b/chrome/browser/extensions/default_apps.cc
index 43db8ba..a9e305f 100644
--- a/chrome/browser/extensions/default_apps.cc
+++ b/chrome/browser/extensions/default_apps.cc
@@ -52,7 +52,7 @@ void DefaultApps::DidInstallApp(const ExtensionIdSet& installed_ids) {
   }
 }
 
-bool DefaultApps::ShouldShowPromo(const ExtensionIdSet& installed_ids) {
+bool DefaultApps::CheckShouldShowPromo(const ExtensionIdSet& installed_ids) {
 #if defined(OS_CHROMEOS)
   // Don't show the promo at all on Chrome OS.
   return false;
diff --git a/chrome/browser/extensions/default_apps.h b/chrome/browser/extensions/default_apps.h
index 767b2f3..a6e899e 100644
--- a/chrome/browser/extensions/default_apps.h
+++ b/chrome/browser/extensions/default_apps.h
@@ -50,10 +50,11 @@ class DefaultApps {
   // all the default apps, GetAppsToInstall() will start returning NULL.
   void DidInstallApp(const ExtensionIdSet& installed_ids);
 
-  // Returns true if the apps promo should be displayed in the launcher. This
-  // starts returning true once the default apps have all been installed and
-  // stops after the promo expires.
-  bool ShouldShowPromo(const ExtensionIdSet& installed_ids);
+  // Returns true if the apps promo should be displayed in the launcher.
+  //
+  // NOTE: If the default apps have been installed, but |installed_ids| is
+  // different than GetDefaultApps(), this will permanently expire the promo.
+  bool CheckShouldShowPromo(const ExtensionIdSet& installed_ids);
 
   // Should be called after each time the promo is installed.
   void DidShowPromo();
@@ -62,10 +63,10 @@ class DefaultApps {
   void SetPromoHidden();
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(DefaultApps, Basics);
-  FRIEND_TEST_ALL_PREFIXES(DefaultApps, HidePromo);
-  FRIEND_TEST_ALL_PREFIXES(DefaultApps, InstallingAnAppHidesPromo);
-  FRIEND_TEST_ALL_PREFIXES(DefaultApps,
+  FRIEND_TEST_ALL_PREFIXES(ExtensionDefaultApps, Basics);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionDefaultApps, HidePromo);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionDefaultApps, InstallingAnAppHidesPromo);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionDefaultApps,
                            ManualAppInstalledWhileInstallingDefaultApps);
 
   bool GetDefaultAppsInstalled() const;
diff --git a/chrome/browser/extensions/default_apps_unittest.cc b/chrome/browser/extensions/default_apps_unittest.cc
index 381e679..4fa9bc2 100644
--- a/chrome/browser/extensions/default_apps_unittest.cc
+++ b/chrome/browser/extensions/default_apps_unittest.cc
@@ -10,7 +10,7 @@
 // TODO(dpolukhin): On Chrome OS all apps are installed via external extensions,
 // and the web store promo is never shown.
 #if !defined(OS_CHROMEOS)
-TEST(DefaultApps, Basics) {
+TEST(ExtensionDefaultApps, Basics) {
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
   DefaultApps default_apps(&pref_service);
@@ -23,7 +23,7 @@ TEST(DefaultApps, Basics) {
 
   // The promo should not be shown until the default apps have been installed.
   ExtensionIdSet installed_app_ids;
-  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_app_ids));
+  EXPECT_FALSE(default_apps.CheckShouldShowPromo(installed_app_ids));
 
   // Simulate installing the apps one by one and notifying default_apps after
   // each intallation. Nothing should change until we have installed all the
@@ -41,7 +41,7 @@ TEST(DefaultApps, Basics) {
     default_apps.DidInstallApp(extension_id_sets[i]);
     EXPECT_TRUE(default_app_ids == *default_apps.GetAppsToInstall());
     EXPECT_FALSE(default_apps.GetDefaultAppsInstalled());
-    EXPECT_FALSE(default_apps.ShouldShowPromo(extension_id_sets[i]));
+    EXPECT_FALSE(default_apps.CheckShouldShowPromo(extension_id_sets[i]));
   }
 
   // Simulate all the default apps being installed. Now we should stop getting
@@ -51,19 +51,19 @@ TEST(DefaultApps, Basics) {
   EXPECT_TRUE(default_apps.GetDefaultAppsInstalled());
 
   // And the promo should become available.
-  EXPECT_TRUE(default_apps.ShouldShowPromo(default_app_ids));
+  EXPECT_TRUE(default_apps.CheckShouldShowPromo(default_app_ids));
 
   // The promo should be available up to the max allowed times, then stop.
   for (int i = 0; i < DefaultApps::kAppsPromoCounterMax; ++i) {
-    EXPECT_TRUE(default_apps.ShouldShowPromo(default_app_ids));
+    EXPECT_TRUE(default_apps.CheckShouldShowPromo(default_app_ids));
     default_apps.DidShowPromo();
     EXPECT_EQ(i + 1, default_apps.GetPromoCounter());
   }
-  EXPECT_FALSE(default_apps.ShouldShowPromo(default_app_ids));
+  EXPECT_FALSE(default_apps.CheckShouldShowPromo(default_app_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
 }
 
-TEST(DefaultApps, HidePromo) {
+TEST(ExtensionDefaultApps, HidePromo) {
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
   DefaultApps default_apps(&pref_service);
@@ -71,16 +71,16 @@ TEST(DefaultApps, HidePromo) {
   ExtensionIdSet default_app_ids = *default_apps.GetAppsToInstall();
   default_apps.DidInstallApp(default_app_ids);
 
-  EXPECT_TRUE(default_apps.ShouldShowPromo(default_app_ids));
+  EXPECT_TRUE(default_apps.CheckShouldShowPromo(default_app_ids));
   default_apps.DidShowPromo();
   EXPECT_EQ(1, default_apps.GetPromoCounter());
 
   default_apps.SetPromoHidden();
-  EXPECT_FALSE(default_apps.ShouldShowPromo(default_app_ids));
+  EXPECT_FALSE(default_apps.CheckShouldShowPromo(default_app_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
 }
 
-TEST(DefaultApps, InstallingAnAppHidesPromo) {
+TEST(ExtensionDefaultApps, InstallingAnAppHidesPromo) {
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
   DefaultApps default_apps(&pref_service);
@@ -89,18 +89,18 @@ TEST(DefaultApps, InstallingAnAppHidesPromo) {
   ExtensionIdSet installed_app_ids = default_app_ids;
   default_apps.DidInstallApp(installed_app_ids);
 
-  EXPECT_TRUE(default_apps.ShouldShowPromo(installed_app_ids));
+  EXPECT_TRUE(default_apps.CheckShouldShowPromo(installed_app_ids));
   default_apps.DidShowPromo();
   EXPECT_EQ(1, default_apps.GetPromoCounter());
 
   // Now simulate a new extension being installed. This should cause the promo
   // to be hidden.
   installed_app_ids.insert("foo");
-  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_app_ids));
+  EXPECT_FALSE(default_apps.CheckShouldShowPromo(installed_app_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
 }
 
-TEST(DefaultApps, ManualAppInstalledWhileInstallingDefaultApps) {
+TEST(ExtensionDefaultApps, ManualAppInstalledWhileInstallingDefaultApps) {
   // It is possible to have apps manually installed while the default apps are
   // being installed. The network or server might be down, causing the default
   // app installation to fail. The updater might take awhile to get around to
@@ -131,7 +131,7 @@ TEST(DefaultApps, ManualAppInstalledWhileInstallingDefaultApps) {
 
   // The promo shouldn't turn on though, because it would look weird with the
   // user's extra, manually installed extensions.
-  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
+  EXPECT_FALSE(default_apps.CheckShouldShowPromo(installed_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
 }
 #endif  // OS_CHROMEOS
diff --git a/chrome/browser/resources/ntp/apps.js b/chrome/browser/resources/ntp/apps.js
index 9c032fe..41a5ebf 100644
--- a/chrome/browser/resources/ntp/apps.js
+++ b/chrome/browser/resources/ntp/apps.js
@@ -14,11 +14,17 @@ var PING_WEBSTORE_LAUNCH_PREFIX = 'record-webstore-launch';
 
 function getAppsCallback(data) {
   logEvent('received apps');
+
+  // In the case of prefchange-triggered updates, we don't receive this flag.
+  // Just leave it set as it was before in that case.
+  if ('showPromo' in data)
+    apps.showPromo = data.showPromo;
+
   var appsSection = $('apps');
   var appsSectionContent = $('apps-content');
   var appsMiniview = appsSection.getElementsByClassName('miniview')[0];
   var appsPromo = $('apps-promo');
-  var appsPromoPing = PING_WEBSTORE_LAUNCH_PREFIX + '+' + data.showPromo;
+  var appsPromoPing = PING_WEBSTORE_LAUNCH_PREFIX + '+' + apps.showPromo;
   var webStoreEntry;
 
   // Hide menu options that are not supported on the OS or windowing system.
@@ -34,8 +40,6 @@ function getAppsCallback(data) {
   appsMiniview.textContent = '';
   appsSectionContent.textContent = '';
 
-  apps.showPromo = data.showPromo;
-
   data.apps.sort(function(a,b) {
     return a.app_launch_index - b.app_launch_index;
   });
@@ -65,7 +69,7 @@ function getAppsCallback(data) {
   addClosedMenuFooter(apps.menu, 'apps', MINIMIZED_APPS, Section.APPS);
 
   apps.loaded = true;
-  if (data.showPromo)
+  if (apps.showPromo)
     document.documentElement.classList.add('apps-promo-visible');
   else
     document.documentElement.classList.remove('apps-promo-visible');
@@ -73,7 +77,7 @@ function getAppsCallback(data) {
   maybeDoneLoading();
 
   if (data.apps.length > 0 && isDoneLoading()) {
-    if (!data.showPromo && data.apps.length >= MAX_APPS_PER_ROW[layoutMode])
+    if (!apps.showPromo && data.apps.length >= MAX_APPS_PER_ROW[layoutMode])
       webStoreEntry.classList.add('loner');
     else
       webStoreEntry.classList.remove('loner');