Restored an option to enable enhanced bookmarks experiment from Finch

chrome/browser/extensions/external_component_loader.cc

Index: chrome/browser/extensions/external_component_loader.cc
diff --git a/chrome/browser/extensions/external_component_loader.cc b/chrome/browser/extensions/external_component_loader.cc
index 98e332171c3cb186b27b1fbd53ad093872b6d248..144a3c158e8d6e9c87b9a4760a7938805c71968b 100644
--- a/chrome/browser/extensions/external_component_loader.cc
+++ b/chrome/browser/extensions/external_component_loader.cc
@@ -4,20 +4,21 @@
 
 #include "chrome/browser/extensions/external_component_loader.h"
 
-#include "base/command_line.h"
-#include "base/prefs/pref_change_registrar.h"
-#include "base/prefs/pref_service.h"
-#include "base/values.h"
 #include "chrome/browser/bookmarks/enhanced_bookmarks_features.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/search/hotword_service_factory.h"
-#include "chrome/common/chrome_switches.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/extensions/extension_constants.h"
-#include "chrome/common/pref_names.h"
-#include "components/sync_driver/pref_names.h"
-#include "components/user_prefs/pref_registry_syncable.h"
-#include "content/public/browser/browser_thread.h"
+#include "components/signin/core/browser/signin_manager.h"
+
+namespace {
+
+bool IsUserSignedin(Profile* profile) {
+  SigninManager* signin = SigninManagerFactory::GetForProfile(profile);
+  return signin && !signin->GetAuthenticatedUsername().empty();
+}
+
+}  // namespace
 
 namespace extensions {
 
@@ -39,55 +40,14 @@ void ExternalComponentLoader::StartLoading() {
                       extension_urls::GetWebstoreUpdateUrl().spec());
   }
 
-  BookmarksExperimentState bookmarks_experiment_state_before =
-      static_cast<BookmarksExperimentState>(profile_->GetPrefs()->GetInteger(
-          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled));
-  if (bookmarks_experiment_state_before == kBookmarksExperimentEnabled) {
-    // kEnhancedBookmarksExperiment flag could have values "", "1" and "0".
-    // "0" - user opted out.
-    if (CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-            switches::kEnhancedBookmarksExperiment) != "0") {
-      // Experiment enabled.
-      std::string ext_id = profile_->GetPrefs()->GetString(
-          sync_driver::prefs::kEnhancedBookmarksExtensionId);
-      if (!ext_id.empty()) {
-        prefs_->SetString(ext_id + ".external_update_url",
-                          extension_urls::GetWebstoreUpdateUrl().spec());
-      }
-    } else {
-      // Experiment enabled but user opted out.
-      profile_->GetPrefs()->SetInteger(
-          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
-          kBookmarksExperimentEnabledUserOptOut);
-    }
-  } else if (bookmarks_experiment_state_before ==
-             kBookmarksExperimentEnabledUserOptOut) {
-    if (CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-            switches::kEnhancedBookmarksExperiment) != "0") {
-      // User opted in again.
-      profile_->GetPrefs()->SetInteger(
-          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
-          kBookmarksExperimentEnabled);
-      std::string ext_id = profile_->GetPrefs()->GetString(
-          sync_driver::prefs::kEnhancedBookmarksExtensionId);
-      if (!ext_id.empty()) {
-        prefs_->SetString(ext_id + ".external_update_url",
-                          extension_urls::GetWebstoreUpdateUrl().spec());
-      }
-    }
-  } else {
-    // Experiment disabled.
-    profile_->GetPrefs()->ClearPref(
-        sync_driver::prefs::kEnhancedBookmarksExperimentEnabled);
-    profile_->GetPrefs()->ClearPref(
-        sync_driver::prefs::kEnhancedBookmarksExtensionId);
-  }
-  BookmarksExperimentState bookmarks_experiment_state =
-      static_cast<BookmarksExperimentState>(profile_->GetPrefs()->GetInteger(
-          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled));
-  if (bookmarks_experiment_state_before != bookmarks_experiment_state) {
-    UpdateBookmarksExperiment(g_browser_process->local_state(),
-                              bookmarks_experiment_state);
+  UpdateBookmarksExperimentState(profile_->GetPrefs(),
+                                 g_browser_process->local_state(),
+                                 IsUserSignedin(profile_));

[not at google - send to devlin, 2014/05/07 21:16:39]

Just a Profile* would be a nicer dependency for this function. Most of these are
implementation details, e.g. it can pull out g_browser_process itself.

[yefimt, 2014/05/07 21:27:48]

Yes, unfortunately bookmarks are not allowed to depend on Profile

On 2014/05/07 21:16:39, kalman wrote:

+  std::string ext_id;
+  if (IsBookmarksExperimentEnabled(profile_->GetPrefs(), &ext_id) &&

[not at google - send to devlin, 2014/05/07 21:16:39]

GetBookmarksExperimentExtensionID would be a better name since it gets an
extension ID.

[yefimt, 2014/05/07 21:27:48]

Done.

+      !ext_id.empty()) {

[not at google - send to devlin, 2014/05/07 21:16:39]

This function should have a contract that a return value of "true" will fill in
a valid extension ID. I.e. this should be a DCHECK(!ext_id.empty()).

[yefimt, 2014/05/07 21:27:48]

Done.

+    prefs_->SetString(ext_id + ".external_update_url",
+                      extension_urls::GetWebstoreUpdateUrl().spec());
   }
 
   LoadFinished();