More cleanup of DefaultApps code

chrome/browser/extensions/default_apps_unittest.cc

Index: chrome/browser/extensions/default_apps_unittest.cc
diff --git a/chrome/browser/extensions/default_apps_unittest.cc b/chrome/browser/extensions/default_apps_unittest.cc
index 4fa9bc2c987e03b541e02e6517a4b0553aceabf8..d7edfc0d6c394abd991b5e12e799425cced19169 100644
--- a/chrome/browser/extensions/default_apps_unittest.cc
+++ b/chrome/browser/extensions/default_apps_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/logging.h"
 #include "chrome/browser/extensions/default_apps.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/test/testing_pref_service.h"
@@ -10,93 +11,140 @@
 // 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(ExtensionDefaultApps, Basics) {
+TEST(ExtensionDefaultApps, HappyPath) {
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
-  DefaultApps default_apps(&pref_service);
+  DefaultApps default_apps(&pref_service, "en-US");
 
-  ExtensionIdSet default_app_ids = *default_apps.GetAppsToInstall();
+  const ExtensionIdSet& default_app_ids = default_apps.default_apps();
   ASSERT_GT(default_app_ids.size(), 0u);
   EXPECT_FALSE(default_apps.GetDefaultAppsInstalled());
   EXPECT_EQ(0, default_apps.GetPromoCounter());
-  EXPECT_EQ(default_app_ids, *default_apps.GetDefaultApps());
 
-  // The promo should not be shown until the default apps have been installed.
+  // If no apps are installed, the default apps should be installed.
   ExtensionIdSet installed_app_ids;
-  EXPECT_FALSE(default_apps.CheckShouldShowPromo(installed_app_ids));
+  EXPECT_TRUE(default_apps.ShouldInstallDefaultApps(installed_app_ids));
+
+  // The launcher should not be shown until the default apps have been installed.

[Erik does not do reviews, 2011/01/03 22:01:30]

nit: 80 cols

+  EXPECT_FALSE(default_apps.ShouldShowAppLauncher(installed_app_ids));
+
+  // The promo should not be shown until the default apps have been installed.
+  EXPECT_FALSE(default_apps.ShouldShowPromo(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
   // default apps.
-  ExtensionIdSet extension_id_sets[] = {
-    default_app_ids,
-    default_app_ids,
-    default_app_ids
-  };
-  extension_id_sets[0].clear();
-  extension_id_sets[1].erase(extension_id_sets[1].begin());
-  extension_id_sets[2].erase(extension_id_sets[2].begin(),
-                             ++extension_id_sets[2].begin());
-  for (size_t i = 0; i < arraysize(extension_id_sets); ++i) {
-    default_apps.DidInstallApp(extension_id_sets[i]);
-    EXPECT_TRUE(default_app_ids == *default_apps.GetAppsToInstall());
+  for (size_t i = 0; i < default_app_ids.size() - 1; ++i) {
+    ExtensionIdSet::const_iterator iter = default_app_ids.begin();
+    for (size_t j = 0; j <= i; ++j)
+      ++iter;
+    installed_app_ids.insert(*iter);
+    default_apps.DidInstallApp(installed_app_ids);
     EXPECT_FALSE(default_apps.GetDefaultAppsInstalled());
-    EXPECT_FALSE(default_apps.CheckShouldShowPromo(extension_id_sets[i]));
+    EXPECT_TRUE(default_apps.ShouldInstallDefaultApps(installed_app_ids));
+    EXPECT_FALSE(default_apps.ShouldShowAppLauncher(installed_app_ids));
+    EXPECT_FALSE(default_apps.ShouldShowPromo(installed_app_ids));
   }
 
   // Simulate all the default apps being installed. Now we should stop getting
   // default apps to install.
-  default_apps.DidInstallApp(default_app_ids);
-  EXPECT_EQ(NULL, default_apps.GetAppsToInstall());
+  installed_app_ids = default_app_ids;
+  default_apps.DidInstallApp(installed_app_ids);
   EXPECT_TRUE(default_apps.GetDefaultAppsInstalled());
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_app_ids));
 
-  // And the promo should become available.
-  EXPECT_TRUE(default_apps.CheckShouldShowPromo(default_app_ids));
+  // And the promo and launcher should become available.
+  EXPECT_TRUE(default_apps.ShouldShowAppLauncher(installed_app_ids));
+  EXPECT_TRUE(default_apps.ShouldShowPromo(installed_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.CheckShouldShowPromo(default_app_ids));
+    EXPECT_TRUE(default_apps.ShouldShowPromo(installed_app_ids));
     default_apps.DidShowPromo();
     EXPECT_EQ(i + 1, default_apps.GetPromoCounter());
   }
-  EXPECT_FALSE(default_apps.CheckShouldShowPromo(default_app_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_app_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
+
+  // Even if all the apps are subsequently removed, the apps section should
+  // remain.
+  installed_app_ids.clear();
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_app_ids));
+  EXPECT_TRUE(default_apps.ShouldShowAppLauncher(installed_app_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_app_ids));
+}
+
+TEST(ExtensionDefaultApps, UnsupportedLocale) {
+  TestingPrefService pref_service;
+  DefaultApps::RegisterUserPrefs(&pref_service);
+  DefaultApps default_apps(&pref_service, "fr");
+
+  const ExtensionIdSet& default_app_ids = default_apps.default_apps();
+  EXPECT_GT(default_app_ids.size(), 0u);
+
+  // Since the store only supports en-US at the moment, we don't install default
+  // apps or promote the store.
+  ExtensionIdSet installed_ids;
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowAppLauncher(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
+
+  // If the user installs an app manually, then we show the apps section, but
+  // no promotion or default apps.
+  installed_ids.insert(*(default_app_ids.begin()));
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_ids));
+  EXPECT_TRUE(default_apps.ShouldShowAppLauncher(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
+
+  // Even if the user installs the exact set of default apps, we don't show the
+  // promo.
+  installed_ids = default_app_ids;
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_ids));
+  EXPECT_TRUE(default_apps.ShouldShowAppLauncher(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
+
+  // If the user uninstalls the apps again, we go back to not showing the
+  // apps section.
+  installed_ids.clear();
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowAppLauncher(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
 }
 
 TEST(ExtensionDefaultApps, HidePromo) {
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
-  DefaultApps default_apps(&pref_service);
+  DefaultApps default_apps(&pref_service, "en-US");
 
-  ExtensionIdSet default_app_ids = *default_apps.GetAppsToInstall();
+  const ExtensionIdSet& default_app_ids = default_apps.default_apps();
   default_apps.DidInstallApp(default_app_ids);
 
-  EXPECT_TRUE(default_apps.CheckShouldShowPromo(default_app_ids));
+  EXPECT_TRUE(default_apps.ShouldShowPromo(default_app_ids));
   default_apps.DidShowPromo();
   EXPECT_EQ(1, default_apps.GetPromoCounter());
 
   default_apps.SetPromoHidden();
-  EXPECT_FALSE(default_apps.CheckShouldShowPromo(default_app_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(default_app_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
 }
 
 TEST(ExtensionDefaultApps, InstallingAnAppHidesPromo) {
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
-  DefaultApps default_apps(&pref_service);
+  DefaultApps default_apps(&pref_service, "en-US");
 
-  ExtensionIdSet default_app_ids = *default_apps.GetAppsToInstall();
+  const ExtensionIdSet& default_app_ids = default_apps.default_apps();
   ExtensionIdSet installed_app_ids = default_app_ids;
   default_apps.DidInstallApp(installed_app_ids);
 
-  EXPECT_TRUE(default_apps.CheckShouldShowPromo(installed_app_ids));
+  EXPECT_TRUE(default_apps.ShouldShowPromo(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.CheckShouldShowPromo(installed_app_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_app_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
 }
 
@@ -111,27 +159,31 @@ TEST(ExtensionDefaultApps, ManualAppInstalledWhileInstallingDefaultApps) {
   // the default ones.
   TestingPrefService pref_service;
   DefaultApps::RegisterUserPrefs(&pref_service);
-  DefaultApps default_apps(&pref_service);
+  DefaultApps default_apps(&pref_service, "en-US");
 
   // Simulate an app getting installed before the complete set of default apps.
-  // This shouldn't affect us installing default apps. We should keep trying.
+  // This should stop the default apps from trying to be installed. The launcher

[Erik does not do reviews, 2011/01/03 22:01:30]

Doesn't this put us in a weird state?  They could have some default apps
installed, but no explanation as to where they came from.  Do we need to
uninstall any default apps in this situation?

[Aaron Boodman, 2011/01/04 20:59:35]

It does.

This code was mainly intended to handle the case where you already have apps
installed before the default app code runs, but it also affects the case you
describe.

The problem with trying to gracefully handle this situation is that we don't
really know if an app was installed because it was a default app or if it was
installed by the user. So the user could conceivably install one of the default
apps manually, then the default app installation runs, and we end up
uninstallling something the user installed.

In practice, the bad situation seems unlikely to occur since it would be weird
for one default app to install but another not to.

Adding more code to work around these edge cases seemed scary, and a little
wasteful since we're going to be reworking the location code, and would
hopefully be able to use that in the future to better track where apps came
from.

What do you think? Can you think of some half measure I could do now to better
handle this issue that would align with future work?

+  // should also immediately show up.
   ExtensionIdSet installed_ids;
   installed_ids.insert("foo");
-  default_apps.DidInstallApp(installed_ids);
-  EXPECT_FALSE(default_apps.GetDefaultAppsInstalled());
-  EXPECT_TRUE(default_apps.GetAppsToInstall());
-
-  // Now add all the default apps in addition to the extra app. We should stop
-  // trying to install default apps.
-  installed_ids = *default_apps.GetAppsToInstall();
-  installed_ids.insert("foo");
-  default_apps.DidInstallApp(installed_ids);
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_ids));
   EXPECT_TRUE(default_apps.GetDefaultAppsInstalled());
-  EXPECT_FALSE(default_apps.GetAppsToInstall());
+  EXPECT_TRUE(default_apps.ShouldShowAppLauncher(installed_ids));
 
   // The promo shouldn't turn on though, because it would look weird with the
   // user's extra, manually installed extensions.
-  EXPECT_FALSE(default_apps.CheckShouldShowPromo(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
   EXPECT_EQ(DefaultApps::kAppsPromoCounterMax, default_apps.GetPromoCounter());
+
+  // Going back to a subset of the default apps shouldn't allow the default app
+  // install to continue.
+  installed_ids.clear();
+  EXPECT_FALSE(default_apps.ShouldInstallDefaultApps(installed_ids));
+  EXPECT_TRUE(default_apps.GetDefaultAppsInstalled());
+  EXPECT_TRUE(default_apps.ShouldShowAppLauncher(installed_ids));
+  EXPECT_FALSE(default_apps.ShouldShowPromo(installed_ids));
+
+  // Going to the exact set of default apps shouldn't show the promo.
+  EXPECT_FALSE(default_apps.ShouldShowPromo(default_apps.default_apps()));
 }
 #endif  // OS_CHROMEOS