new implementation of default apps

chrome/browser/extensions/default_apps.cc

Index: chrome/browser/extensions/default_apps.cc
diff --git a/chrome/browser/extensions/default_apps.cc b/chrome/browser/extensions/default_apps.cc
index b7388bbb517977d34a74695b300187f45404fcc9..3f53cbf55819cffc4b07bee06a33fef2a10586b0 100644
--- a/chrome/browser/extensions/default_apps.cc
+++ b/chrome/browser/extensions/default_apps.cc
@@ -17,7 +17,9 @@
 #include "chrome/common/pref_names.h"
 #include "ui/base/l10n/l10n_util.h"
 
-static bool ShouldInstallInProfile(Profile* profile) {
+namespace default_apps {
+
+bool ShouldInstallInProfile(Profile* profile) {
   // We decide to install or not install default apps based on the following
   // criteria, from highest priority to lowest priority:
   //
@@ -26,8 +28,6 @@ static bool ShouldInstallInProfile(Profile* profile) {
   // - The command line option.  Tests use this option to disable installation
   //   of default apps in some cases.
   // - If the locale is not compatible with the defaults, don't install them.
-  // - If the profile says to either always install or never install default
-  //   apps, obey.
   // - The kDefaultApps preferences value in the profile.  This value is
   //   usually set in the master_preferences file.
   bool install_apps =
@@ -36,6 +36,13 @@ static bool ShouldInstallInProfile(Profile* profile) {
   default_apps::InstallState state =
       static_cast<default_apps::InstallState>(profile->GetPrefs()->GetInteger(
           prefs::kDefaultAppsInstallState));
+
+  // migrate old default_apps. This will install them once changing the location

[Mihai Parparita -not on Chrome, 2012/08/15 00:32:13]

- Capitalize "Migrate"
- Remove the underscore in default_apps.
- The comment should have a sentence or two about the old default apps mechanism
(mention that it used the external location, etc.).
- There should be a TODO to remove the migration code in ~6 months.

+  // from external to internal.
+  if (state == default_apps::kAlwaysProvideDefaultApps) {
+    state = default_apps::kUnknown;

[Mihai Parparita -not on Chrome, 2012/08/15 00:32:13]

I think this will mean that users will lose their default apps. If I had default
apps with Chrome 22 (value was kAlwaysProvideDefaultApps), then state gets reset
of kUnknown here. Then on line 51, we check if this is a new profile (assuming
we're now running Chrome 23), and since it's not, install_apps is now false.

Your test doesn't catch this, since it's testing with a profile created by the
same version as the one that's currently running.

+  }
+
   switch (state) {
     case default_apps::kUnknown: {
       // This is the first time the default apps feature runs on this profile.
@@ -45,9 +52,8 @@ static bool ShouldInstallInProfile(Profile* profile) {
         install_apps = false;
       break;
     }
-    case default_apps::kAlwaysProvideDefaultApps:
-      install_apps = true;
-      break;
+
+    case default_apps::kAlreadyInstalledDefaultApps:
     case default_apps::kNeverProvideDefaultApps:
       install_apps = false;
       break;
@@ -55,19 +61,8 @@ static bool ShouldInstallInProfile(Profile* profile) {
       NOTREACHED();
   }
 
-  if (install_apps) {
-    // Don't bother installing default apps in locales where it is known that
-    // they don't work.
-    // TODO(rogerta): Do this check dynamically once the webstore can expose
-    // an API. See http://crbug.com/101357
-    const std::string& locale = g_browser_process->GetApplicationLocale();
-    static const char* unsupported_locales[] = {"CN", "TR", "IR"};
-    for (size_t i = 0; i < arraysize(unsupported_locales); ++i) {
-      if (EndsWith(locale, unsupported_locales[i], false)) {
-        install_apps = false;
-        break;
-      }
-    }
+  if (install_apps && !isLocaleSupported()) {
+    install_apps = false;
   }
 
   if (CommandLine::ForCurrentProcess()->HasSwitch(
@@ -80,22 +75,13 @@ static bool ShouldInstallInProfile(Profile* profile) {
         kDefaultAppsTrialName)->group_name() != kDefaultAppsTrialNoAppsGroup;
   }
 
-  // Save the state if needed.  Once it is decided whether we are installing
-  // default apps or not, we want to always respond with same value.  Therefore
-  // on first run of this feature (i.e. the current state is kUnknown) the
-  // state is updated to remember the choice that was made at this time.  The
-  // next time chrome runs it will use the same decision.
-  //
-  // The reason for responding with the same value is that once an external
-  // extenson provider has provided apps for a given profile, it must continue
-  // to provide those extensions on each subsequent run.  Otherwise the
-  // extension manager will automatically uninstall the apps.  The extension
-  // manager is smart enough to know not to reinstall the apps on all
-  // subsequent runs of chrome.
+  // Default apps are only installed on profile creation or a new chrome
+  // download.
   if (state == default_apps::kUnknown) {
     if (install_apps) {
-      profile->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
-                                      default_apps::kAlwaysProvideDefaultApps);
+      profile->GetPrefs()->SetInteger(
+          prefs::kDefaultAppsInstallState,
+          default_apps::kAlreadyInstalledDefaultApps);
     } else {
       profile->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
                                       default_apps::kNeverProvideDefaultApps);
@@ -105,8 +91,6 @@ static bool ShouldInstallInProfile(Profile* profile) {
   return install_apps;
 }
 
-namespace default_apps {
-
 void RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterIntegerPref(prefs::kDefaultAppsInstallState, kUnknown,
                              PrefService::UNSYNCABLE_PREF);
@@ -135,4 +119,19 @@ void Provider::VisitRegisteredExtension() {
   extensions::ExternalProviderImpl::VisitRegisteredExtension();
 }
 
+bool isLocaleSupported() {
+    // Don't bother installing default apps in locales where it is known that

[Mihai Parparita -not on Chrome, 2012/08/15 00:32:13]

Intended one too many times.

+    // they don't work.
+    // TODO(rogerta): Do this check dynamically once the webstore can expose
+    // an API. See http://crbug.com/101357
+    const std::string& locale = g_browser_process->GetApplicationLocale();
+    static const char* unsupported_locales[] = {"CN", "TR", "IR"};
+    for (size_t i = 0; i < arraysize(unsupported_locales); ++i) {
+      if (EndsWith(locale, unsupported_locales[i], false)) {
+        return false;
+      }
+    }
+    return true;
+}
+
 }  // namespace default_apps