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,101 @@
+// 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 "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"
+
+void DefaultAppsProvider::RegisterUserPrefs(PrefService* prefs) {
+  prefs->RegisterIntegerPref(prefs::kDefaultAppsInstallState, kUnknown,
+                             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
+  // - 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

[Finnur, 2011/10/21 09:59:32]

Nit: Capitalize first word of comments (personal preference).
Nit: End comments with period.

[Roger Tawa OOO till Jul 10th, 2011/10/24 14:23:14]

Done.

+  bool install_apps =
+      profile->GetPrefs()->GetString(prefs::kDefaultApps) == "install";
+
+  int state = 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();

[Finnur, 2011/10/21 09:59:32]

Just something to consider: If you leave out the default case here then whenever
someone adds to the enum |InstallState| they get a compile assert (on Linux and
Mac) if they don't add a case to handle the new enum. Maybe you need to change
the type of |state| from int to InstallState (not sure).

[Roger Tawa OOO till Jul 10th, 2011/10/24 14:23:14]

Done.

+  }
+
+  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.

[Finnur, 2011/10/21 09:59:32]

Is there a bug on this you can reference?

[Roger Tawa OOO till Jul 10th, 2011/10/24 14:23:14]

No, created one and added to comment.

+    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;
+}
Property changes on: chrome\browser\extensions\default_apps_provider.cc
___________________________________________________________________
Added: svn:eol-style
   + LF