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

chrome/browser/extensions/default_apps_provider.cc

Index: chrome/browser/extensions/default_apps_provider.cc
===================================================================
--- chrome/browser/extensions/default_apps_provider.cc	(revision 0)
+++ chrome/browser/extensions/default_apps_provider.cc	(revision 0)
@@ -0,0 +1,186 @@
+// 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_provider.h"
+
+#include "base/command_line.h"
+#include "base/metrics/field_trial.h"
+#include "base/values.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/crx_installer.h"
+#include "chrome/browser/extensions/default_apps_provider.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_notification_types.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_source.h"
+#include "ui/base/l10n/l10n_util.h"
+
+DefaultAppsProvider::DefaultAppsProvider(VisitorInterface* service,
+                                         ExternalExtensionLoader* loader,
+                                         Profile* profile)
+    : ExternalExtensionProviderImpl(service, loader,
+          Extension::EXTERNAL_PREF, Extension::INVALID),
+      profile_(profile) {
+  DCHECK(profile_);
+}
+
+DefaultAppsProvider::~DefaultAppsProvider() {
+}
+
+void DefaultAppsProvider::RegisterUserPrefs(PrefService* prefs) {
+  prefs->RegisterIntegerPref(prefs::kDefaultAppsInstallState, 0,
+                             PrefService::UNSYNCABLE_PREF);
+}
+
+bool DefaultAppsProvider::ShouldRegister(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
+  // - 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"};
+    for (size_t i = 0; i < arraysize(unsupported_locales); ++i) {
+      if (EndsWith(locale, unsupported_locales[i], false)) {
+        install_apps = false;
+        break;
+      }
+    }
+  }
+
+  return install_apps;
+}
+
+void DefaultAppsProvider::SetPrefs(base::DictionaryValue* prefs) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (prefs->size() > 0) {
+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
+                   content::Source<Profile>(profile_));
+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
+                   NotificationService::AllSources());
+  }
+
+  ExternalExtensionProviderImpl::SetPrefs(prefs);
+
+  // If the number of invalid extension is the same as the total, then we are
+  // done.
+  if (prefs->size() == invalid_extensions().size()) {
+    profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                     kInstallDone);
+    profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+  }
+}
+
+void DefaultAppsProvider::ServiceShutdown() {
+  profile_ = NULL;
+  ExternalExtensionProviderImpl::ServiceShutdown();
+}
+
+void DefaultAppsProvider::VisitRegisteredExtension() const {
+  if (profile_) {
+    int state = profile_->GetPrefs()->GetInteger(
+        prefs::kDefaultAppsInstallState);
+    switch (state) {
+      case kInstallNotStarted: {
+        // We never even tried to install the default apps, so try now.
+        // Don't install default apps if the profile already has apps installed.
+        ExtensionService* extension_service = profile_->GetExtensionService();
+        if (extension_service && extension_service->HasApps()) {
+          // Keep track of the fact that we are done.
+          profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                           kInstallDone);
+          profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+          service()->OnExternalProviderReady();
+          return;
+        }
+
+        // Remember that we are now installing the default apps.
+        profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                         kInstalling);
+        profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+        break;
+      }
+      case kInstalling:
+        // We were in the process of installing default apps into the profile
+        // when the profile was last closed.  This may happen because the the

[Finnur, 2011/10/20 10:10:43]

nit: The The? Do you mean The Who? ;)

[Roger Tawa OOO till Jul 10th, 2011/10/20 14:31:15]

no, I really meant the the: http://en.wikipedia.org/wiki/The_The

OK, fixed :-)

+        // process takes time and is asynchronous.  In this case we want to
+        // continue the install process.
+        break;
+      case kInstallDone:
+        // Defaults are installed, so don't need to try again.
+        service()->OnExternalProviderReady();
+        return;
+    }
+  }
+
+  ExternalExtensionProviderImpl::VisitRegisteredExtension();

[Finnur, 2011/10/20 10:10:43]

nit: I'm wondering if it makes sense to comment this line, saying that this will
cause registered extensions found to be installed, since it is not clear from
the code about that break means "install" and return means "not install". Just a
thought. I'll leave it up to you.

[Roger Tawa OOO till Jul 10th, 2011/10/20 14:31:15]

I think the comment in the interface is pretty good, and in chrome we don't
usually copy the comments from the interface into the classes that implement
that interface.  But if you really want, I can add something. The interface
comments says:

// Enumerate registered extensions, calling
// OnExternalExtension(File|UpdateUrl)Found on the |visitor| object for each
// registered extension found.

[Finnur, 2011/10/20 14:39:14]

No, that's ok. Never mind...

On 2011/10/20 14:31:15, Roger Tawa wrote:

+}
+
+void DefaultAppsProvider::Observe(int type,
+                                  const content::NotificationSource& source,
+                                  const content::NotificationDetails& details) {
+  if (!profile_)
+    return;
+
+  // If the notification is for a failed extension, remember its id so that
+  // we don't wait forever for it to get installed.
+  if (type == chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR) {
+    CrxInstaller* crx_installer = content::Source<CrxInstaller>(source).ptr();
+    install_error_extensions_.insert(crx_installer->expected_id());
+  }
+
+  ExtensionService* extension_service = profile_->GetExtensionService();
+  if (!extension_service)
+    return;
+
+  for (DictionaryValue::key_iterator i = prefs()->begin_keys();
+       i != prefs()->end_keys(); ++i) {
+    const std::string& extension_id = *i;
+
+    // If the extension id is for one that is invalid or failed to install,
+    // then consider it "done".
+    if (invalid_extensions().count(extension_id) ||
+        install_error_extensions_.count(extension_id)) {
+      continue;
+    }
+
+    if (!extension_service->GetExtensionById(extension_id, true)) {
+      // We found an extension that is not yet known to the service.  We
+      // need to keep going.
+      return;
+    }
+  }
+
+  profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                   kInstallDone);
+  profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+  registrar_.RemoveAll();
+}
Property changes on: chrome\browser\extensions\default_apps_provider.cc
___________________________________________________________________
Added: svn:eol-style
   + LF