Remove race condition when installing default apps into a new profile.

BUG=99547
TEST=Install chrome and stop it as quickly as possible when the window opens.
Or create a new profile and close all windows as soon as the new profile's
window opens.  Do that as often as you like.  Make sure that eventually,
after leaving the windows open, that all default apps are correctly installed.

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

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

8 files changed, 173 insertions, 87 deletions

diff --git a/chrome/browser/extensions/default_apps.cc b/chrome/browser/extensions/default_apps.cc
new file mode 100644
index 0000000..8a0b3e5
--- /dev/null
+++ b/chrome/browser/extensions/default_apps.cc
@@ -0,0 +1,106 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/extensions/default_apps.h"
+
+#include "base/command_line.h"
+#include "base/metrics/field_trial.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/default_apps_trial.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace default_apps {
+
+void RegisterUserPrefs(PrefService* prefs) {
+  prefs->RegisterIntegerPref(prefs::kDefaultAppsInstallState, kUnknown,
+                             PrefService::UNSYNCABLE_PREF);
+}
+
+bool ShouldInstallInProfile(Profile* profile) {
+  // We decide to install or not install default apps based on the following
+  // criteria, from highest priority to lowest priority:
+  //
+  // - If this instance of chrome is participating in the default apps
+  //   field trial, then install apps based on the group.
+  // - 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 =
+      profile->GetPrefs()->GetString(prefs::kDefaultApps) == "install";
+
+  InstallState state = static_cast<InstallState>(profile->GetPrefs()->
+      GetInteger(prefs::kDefaultAppsInstallState));
+  switch (state) {
+    case kUnknown: {
+      // We get here for either new profile, or profiles created before the
+      // default apps feature was implemented.  In the former case, we always
+      // want to install default apps.  In the latter case, we don't want to
+      // disturb a user that has already installed and possibly curated a list
+      // of favourite apps, so we only install if there are no apps in the
+      // profile.  We can check for both these cases by looking to see if
+      // any apps already exist.
+      ExtensionService* extension_service = profile->GetExtensionService();
+      if (extension_service && extension_service->HasApps())
+        install_apps = false;
+      break;
+    }
+    case kAlwaysProvideDefaultApps:
+      install_apps = true;
+      break;
+    case kNeverProvideDefaultApps:
+      install_apps = false;
+      break;
+    default:
+      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 (CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kDisableDefaultApps)) {
+    install_apps = false;
+  }
+
+  if (base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name)) {
+    install_apps = base::FieldTrialList::Find(
+        kDefaultAppsTrial_Name)->group_name() != kDefaultAppsTrial_NoAppsGroup;
+  }
+
+  // Save the state if needed.
+  if (state == kUnknown) {
+    if (install_apps) {
+      profile->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                      kAlwaysProvideDefaultApps);
+    } else {
+      profile->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                      kNeverProvideDefaultApps);
+    }
+    profile->GetPrefs()->ScheduleSavePersistentPrefs();
+  }
+
+  return install_apps;
+}
+
+}  // namespace default_apps
diff --git a/chrome/browser/extensions/default_apps.h b/chrome/browser/extensions/default_apps.h
new file mode 100644
index 0000000..fb4264d4
--- /dev/null
+++ b/chrome/browser/extensions/default_apps.h
@@ -0,0 +1,36 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_DEFAULT_APPS_H_
+#define CHROME_BROWSER_EXTENSIONS_DEFAULT_APPS_H_
+#pragma once
+
+#include "base/basictypes.h"
+
+class PrefService;
+class Profile;
+
+// Functions and types related to installing default apps.
+namespace default_apps {
+
+// These enum values are persisted in the user preferences, so they should not
+// be changed.
+enum InstallState {
+  kUnknown,
+  kAlwaysProvideDefaultApps,
+  kNeverProvideDefaultApps
+};
+
+// Register preference properties used by default apps to maintain
+// install state.
+void RegisterUserPrefs(PrefService* prefs);
+
+// Determines whether default apps should be installed into the specified
+// profile.  If true, then an instance of ExternalExtensionProviderImpl
+// specific to default apps should be added to the external providers list.
+bool ShouldInstallInProfile(Profile* profile);
+
+}  // namespace default_apps
+
+#endif  // CHROME_BROWSER_EXTENSIONS_DEFAULT_APPS_H_
diff --git a/chrome/browser/extensions/external_extension_provider_impl.cc b/chrome/browser/extensions/external_extension_provider_impl.cc
index 60769d6..a16b68d 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.cc
+++ b/chrome/browser/extensions/external_extension_provider_impl.cc
@@ -14,7 +14,6 @@
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/extensions/default_apps_trial.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/external_extension_provider_interface.h"
 #include "chrome/browser/extensions/external_policy_extension_loader.h"
@@ -26,13 +25,15 @@
 #include "content/browser/browser_thread.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if !defined(OS_CHROMEOS)
+#include "chrome/browser/extensions/default_apps.h"
+#endif
+
 #if defined(OS_WIN)
 #include "chrome/browser/extensions/external_registry_extension_loader_win.h"
 #endif
 
 // Constants for keeping track of extension preferences in a dictionary.
-const char ExternalExtensionProviderImpl::kLocation[] = "location";
-const char ExternalExtensionProviderImpl::kState[] = "state";
 const char ExternalExtensionProviderImpl::kExternalCrx[] = "external_crx";
 const char ExternalExtensionProviderImpl::kExternalVersion[] =
     "external_version";
@@ -41,47 +42,6 @@ const char ExternalExtensionProviderImpl::kExternalUpdateUrl[] =
 const char ExternalExtensionProviderImpl::kSupportedLocales[] =
     "supported_locales";
 
-#if !defined(OS_CHROMEOS)
-class DefaultAppsProvider : public ExternalExtensionProviderImpl {
- public:
-  DefaultAppsProvider(VisitorInterface* service, Profile* profile)
-      : ExternalExtensionProviderImpl(service,
-            new ExternalPrefExtensionLoader(chrome::DIR_DEFAULT_APPS,
-                ExternalPrefExtensionLoader::NONE),
-            Extension::EXTERNAL_PREF, Extension::INVALID),
-        profile_(profile) {
-    DCHECK(profile_);
-  }
-
-  // ExternalExtensionProviderImpl overrides:
-  virtual void ServiceShutdown() OVERRIDE;
-  virtual void VisitRegisteredExtension() const OVERRIDE;
-
- private:
-  Profile* profile_;
-
-  DISALLOW_COPY_AND_ASSIGN(DefaultAppsProvider);
-};
-
-void DefaultAppsProvider::ServiceShutdown() {
-  profile_ = NULL;
-  ExternalExtensionProviderImpl::ServiceShutdown();
-}
-
-void DefaultAppsProvider::VisitRegisteredExtension() const {
-  // Don't install default apps if the profile already has apps installed.
-  if (profile_) {
-    ExtensionService* extension_service = profile_->GetExtensionService();
-    if (extension_service && extension_service->HasApps()) {
-      service()->OnExternalProviderReady(this);
-      return;
-    }
-  }
-
-  ExternalExtensionProviderImpl::VisitRegisteredExtension();
-}
-#endif
-
 ExternalExtensionProviderImpl::ExternalExtensionProviderImpl(
     VisitorInterface* service,
     ExternalExtensionLoader* loader,
@@ -382,46 +342,15 @@ void ExternalExtensionProviderImpl::CreateExternalProviders(
               Extension::EXTERNAL_POLICY_DOWNLOAD)));
 
 #if !defined(OS_CHROMEOS)
-  // We decide to install or not install default apps based on the following
-  // criteria, from highest priority to lowest priority:
-  //
-  // - if this instance of chrome is participating in the default apps
-  //   field trial, then install apps based on the group
-  // - the command line option.  Tests use this option to disable installation
-  //   of default apps in some cases
-  // - the preferences value in the profile.  This value is usually set in
-  //   the master_preferences file
-  bool install_apps =
-      profile->GetPrefs()->GetString(prefs::kDefaultApps) == "install";
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kDisableDefaultApps)) {
-    install_apps = false;
-  }
-  if (base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name)) {
-    install_apps = base::FieldTrialList::Find(
-        kDefaultAppsTrial_Name)->group_name() != kDefaultAppsTrial_NoAppsGroup;
-  }
-
-  if (install_apps) {
-    // Don't bother installing default apps in locales where its known that
-    // they don't work.
-    // TODO(rogerta): Do this check dynamically once the webstore can expose
-    // an API.
-    const std::string& locale = g_browser_process->GetApplicationLocale();
-    static const char* unsupported_locales[] = {"CN", "TR", "IR"};
-    bool supported_locale = true;
-    for (size_t i = 0; i < arraysize(unsupported_locales); ++i) {
-      if (EndsWith(locale, unsupported_locales[i], false)) {
-        supported_locale = false;
-        break;
-      }
-    }
-
-    if (supported_locale) {
-      provider_list->push_back(
-          linked_ptr<ExternalExtensionProviderInterface>(
-              new DefaultAppsProvider(service, profile)));
-    }
+  if (default_apps::ShouldInstallInProfile(profile)) {
+    provider_list->push_back(
+        linked_ptr<ExternalExtensionProviderInterface>(
+            new ExternalExtensionProviderImpl(
+                service,
+                new ExternalPrefExtensionLoader(
+                    chrome::DIR_DEFAULT_APPS, options),
+                Extension::EXTERNAL_PREF,
+                Extension::INVALID)));
   }
 #endif
 }
diff --git a/chrome/browser/extensions/external_extension_provider_impl.h b/chrome/browser/extensions/external_extension_provider_impl.h
index cf4c7e5..65b6afc 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.h
+++ b/chrome/browser/extensions/external_extension_provider_impl.h
@@ -69,9 +69,6 @@ class ExternalExtensionProviderImpl
   static const char kExternalUpdateUrl[];
   static const char kSupportedLocales[];
 
- protected:
-  VisitorInterface* service() const { return service_; }
-
  private:
   // Location for external extensions that are provided by this provider from
   // local crx files.
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index f82d27d..e9a6dfe 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -87,6 +87,8 @@
 #include "chrome/browser/chromeos/status/input_method_menu.h"
 #include "chrome/browser/chromeos/status/network_menu_button.h"
 #include "chrome/browser/chromeos/user_cros_settings_provider.h"
+#else
+#include "chrome/browser/extensions/default_apps.h"
 #endif
 
 namespace browser {
@@ -185,6 +187,9 @@ void RegisterUserPrefs(PrefService* user_prefs) {
 #endif
   SyncPromoUI::RegisterUserPrefs(user_prefs);
   chrome_browser_net::HttpServerPropertiesManager::RegisterPrefs(user_prefs);
+#if !defined(OS_CHROMEOS)
+  default_apps::RegisterUserPrefs(user_prefs);
+#endif
 }
 
 void MigrateBrowserPrefs(PrefService* user_prefs, PrefService* local_state) {
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index e1bcac1..5480adf 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -938,6 +938,8 @@
         'browser/extensions/app_notification_storage.h',
         'browser/extensions/apps_promo.cc',
         'browser/extensions/apps_promo.h',
+        'browser/extensions/default_apps.cc',
+        'browser/extensions/default_apps.h',
         'browser/extensions/default_apps_trial.cc',
         'browser/extensions/default_apps_trial.h',
         'browser/extensions/convert_user_script.cc',
@@ -4050,6 +4052,8 @@
           ],
           'sources!': [
             'browser/background/background_mode_manager_gtk.cc',
+            'browser/extensions/default_apps_provider.cc',
+            'browser/extensions/default_apps_provider.h',
             'browser/first_run/upgrade_util.cc',
             'browser/first_run/upgrade_util.h',
             'browser/first_run/upgrade_util_linux.cc',
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index e51f1c1..5e48bcc 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -382,6 +382,14 @@ const char kMultipleProfilePrefMigration[] =
 // preferences are not lost.
 const char kNetworkPredictionEnabled[] = "dns_prefetching.enabled";
 
+// An integer representing the state of the default apps installation process.
+// This value is persisted in the profile's user preferences because the process
+// is async, and the user may have stopped chrome in the middle.  The next time
+// the profile is opened, the process will continue from where it left off.
+//
+// See possible values in external_extension_provider_impl.cc.
+const char kDefaultAppsInstallState[] = "default_apps_install_state";
+
 #if defined(OS_CHROMEOS)
 // An integer pref to initially mute volume if 1.
 const char kAudioMute[] = "settings.audio.mute";
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index 6d78ba1..81e69d7 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -141,6 +141,7 @@ extern const char kInstantEnabledTime[];
 extern const char kInstantPromo[];
 extern const char kMultipleProfilePrefMigration[];
 extern const char kNetworkPredictionEnabled[];
+extern const char kDefaultAppsInstallState[];
 #if defined(OS_CHROMEOS)
 extern const char kAudioMute[];
 extern const char kAudioVolume[];