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..f5b775c6e91b7a982fcc7b70827ec1ac3d03fdea 100644
--- a/chrome/browser/extensions/external_component_loader.cc
+++ b/chrome/browser/extensions/external_component_loader.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/external_component_loader.h"
 
 #include "base/command_line.h"
+#include "base/metrics/histogram.h"
 #include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_service.h"
 #include "base/values.h"
@@ -12,13 +13,33 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/search/hotword_service_factory.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
+#include "components/signin/core/browser/signin_manager.h"
 #include "components/sync_driver/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 
+namespace {
+
+enum FinchBookmarksExperimentState {
+  kFinchBookmarksExperimentEnabled,
+  kFinchBookmarksExperimentEnabledUserSignedIn,
+  kFinchBookmarksExperimentEnabledUserOptOut,
+  kFinchBookmarksExperimentEnumSize

[not at google - send to devlin, 2014/05/05 20:19:11]

what is the difference between this and the similar enum in bookmarks?

[yefimt, 2014/05/05 20:54:24]

This one is used for UMA_HISTOGRAM_ENUMERATION only.
I want it to have size after values we report and have only values relevant to
reported state.

On 2014/05/05 20:19:11, kalman wrote:

[not at google - send to devlin, 2014/05/05 21:25:19]

Having it separately has the disadvantage of a repeated enum, and the enum being
private means it's harder to refer to meaningfully from histograms.xml*.

I'd just use the bookmarks enum for UMA. people reading the UMA can intepret the
data themselves.


[*] you need to update this.

+};
+
+bool IsUserSignedin(Profile* profile) {
+  SigninManager* signin = SigninManagerFactory::GetForProfile(profile);
+  if (!signin)
+    return false;
+  return !signin->GetAuthenticatedUsername().empty();

[not at google - send to devlin, 2014/05/05 20:19:11]

could phrase these last 3 lines as "return signin &&
!signin->GetAuthenticatedUsername().empty()"

[yefimt, 2014/05/05 20:54:24]

Done.

+}
+
+}  // namespace
+
 namespace extensions {
 
 ExternalComponentLoader::ExternalComponentLoader(Profile* profile)
@@ -42,28 +63,69 @@ void ExternalComponentLoader::StartLoading() {
   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") {
+  // kEnhancedBookmarksExperiment flag could have values "", "1" and "0".
+  // "0" - user opted out.
+  bool opt_out = CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+      switches::kEnhancedBookmarksExperiment) == "0";
+
+  bool enabled_from_finch = false;
+  if (IsEnhancedBookmarksExperimentEnabledFromFinch()) {
+    enabled_from_finch = !IsUserSignedin(profile_);
+    FinchBookmarksExperimentState state_to_report =
+        enabled_from_finch ?
+            (opt_out ? kFinchBookmarksExperimentEnabledUserOptOut :
+                       kFinchBookmarksExperimentEnabled) :
+            kFinchBookmarksExperimentEnabledUserSignedIn;
+    UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.FinchExperimentState",
+                              state_to_report,
+                              kFinchBookmarksExperimentEnumSize);
+  }
+
+  if (enabled_from_finch) {
+    if (opt_out) {
+      // Experiment enabled but user opted out.
+      profile_->GetPrefs()->SetInteger(

[not at google - send to devlin, 2014/05/05 20:19:11]

why do you need to store whether the command line flag has opted out? if the
user gives the command line flag, it shouldn't work. if they don't, it
maybe-should. I.e. persisting a command line flag choice doesn't seem the right
idiom for command line flags.

[yefimt, 2014/05/05 20:54:24]

In case of finch it not really necessary. Just it was convenient to unify it
with the rest of the code when I call UpdateBookmarksExperiment() below only if
state was changed.

On 2014/05/05 20:19:11, kalman wrote:

+          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
+          kBookmarksExperimentOptoutFromFinch);
+    } else {
       // Experiment enabled.
-      std::string ext_id = profile_->GetPrefs()->GetString(
-          sync_driver::prefs::kEnhancedBookmarksExtensionId);
+      profile_->GetPrefs()->SetInteger(
+          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
+          kBookmarksExperimentEnabledFromFinch);
+      std::string ext_id = GetEnhancedBookmarksExtensionIdFromFinch();
       if (!ext_id.empty()) {
         prefs_->SetString(ext_id + ".external_update_url",
                           extension_urls::GetWebstoreUpdateUrl().spec());
       }
-    } else {
+    }
+  } else if (bookmarks_experiment_state_before == kBookmarksExperimentEnabled) {
+    if (opt_out) {
       // Experiment enabled but user opted out.
       profile_->GetPrefs()->SetInteger(
-          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
-          kBookmarksExperimentEnabledUserOptOut);
+        sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
+        kBookmarksExperimentEnabledUserOptOut);
+        UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.SyncExperimentState",
+                                  kBookmarksExperimentEnabledUserOptOut,
+                                  kBookmarksExperimentEnumSize);
+    } else {
+      // 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());

[not at google - send to devlin, 2014/05/05 20:19:11]

I could say the same for finch actually. does prefs need to be involved at all?

[yefimt, 2014/05/05 20:54:24]

If you are talking about statement above, |prefs_| is not preference, it is
dictionary with list of extensions to be loaded from a base class

On 2014/05/05 20:19:11, kalman wrote:

[not at google - send to devlin, 2014/05/05 21:25:19]

having prefs_ and GetPrefs() as totally different things is very confusing.

but no I just meant modifying GetPrefs() at all. it shouldn't be necessary to
mirror the command line nor finch state in preferences.

+      }
+      UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.SyncExperimentState",
+                                kBookmarksExperimentEnabled,
+                                kBookmarksExperimentEnumSize);
     }
   } else if (bookmarks_experiment_state_before ==
              kBookmarksExperimentEnabledUserOptOut) {
-    if (CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-            switches::kEnhancedBookmarksExperiment) != "0") {
+    if (opt_out) {
+      UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.SyncExperimentState",
+                                kBookmarksExperimentEnabledUserOptOut,
+                                kBookmarksExperimentEnumSize);
+    } else {
       // User opted in again.
       profile_->GetPrefs()->SetInteger(
           sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
@@ -74,6 +136,9 @@ void ExternalComponentLoader::StartLoading() {
         prefs_->SetString(ext_id + ".external_update_url",
                           extension_urls::GetWebstoreUpdateUrl().spec());
       }
+      UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.SyncExperimentState",
+                                kBookmarksExperimentEnabled,
+                                kBookmarksExperimentEnumSize);
     }
   } else {
     // Experiment disabled.
@@ -81,6 +146,9 @@ void ExternalComponentLoader::StartLoading() {
         sync_driver::prefs::kEnhancedBookmarksExperimentEnabled);
     profile_->GetPrefs()->ClearPref(
         sync_driver::prefs::kEnhancedBookmarksExtensionId);
+    UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.SyncExperimentState",
+                              kNoBookmarksExperiment,
+                              kBookmarksExperimentEnumSize);
   }
   BookmarksExperimentState bookmarks_experiment_state =
       static_cast<BookmarksExperimentState>(profile_->GetPrefs()->GetInteger(