[Android] Use correct enhanced bookmarks enabling logic.

chrome/browser/bookmarks/enhanced_bookmarks_features.cc

 // Copyright 2013 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/bookmarks/enhanced_bookmarks_features.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
@@ skipping 21 matching lines @@
 #if defined(OS_ANDROID)
   return !ext_id.empty();
 #else
   const extensions::FeatureProvider* feature_provider =
       extensions::FeatureProvider::GetPermissionFeatures();
   extensions::Feature* feature = feature_provider->GetFeature("metricsPrivate");
   return feature && feature->IsIdInWhitelist(ext_id);
 #endif
 }
 
+PrefService* GetFlagsStorage(PrefService* user_prefs,
+                             PrefService* local_state) {
+#if defined(OS_CHROMEOS)
+  // Chrome OS is using user prefs for flags storage.
+  return user_prefs;
+#else
+  return local_state;
+#endif
+}
+
 };  // namespace
 
 bool GetBookmarksExperimentExtensionID(const PrefService* user_prefs,
                                        std::string* extension_id) {
   BookmarksExperimentState bookmarks_experiment_state =
       static_cast<BookmarksExperimentState>(user_prefs->GetInteger(
           sync_driver::prefs::kEnhancedBookmarksExperimentEnabled));
   if (bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED_FROM_FINCH) {
     *extension_id = GetEnhancedBookmarksExtensionIdFromFinch();
     return !extension_id->empty();
   }
   if (bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED) {
     *extension_id = user_prefs->GetString(
         sync_driver::prefs::kEnhancedBookmarksExtensionId);
     return !extension_id->empty();
   }
 
   return false;
 }
 
 void UpdateBookmarksExperimentState(
     PrefService* user_prefs,
     PrefService* local_state,
     bool user_signed_in,
     BookmarksExperimentState experiment_enabled_from_sync) {
- PrefService* flags_storage = local_state;
-#if defined(OS_CHROMEOS)
-  // Chrome OS is using user prefs for flags storage.
-  flags_storage = user_prefs;
-#endif
+  PrefService* flags_storage = GetFlagsStorage(user_prefs, local_state);
 
   BookmarksExperimentState bookmarks_experiment_state_before =
       static_cast<BookmarksExperimentState>(user_prefs->GetInteger(
           sync_driver::prefs::kEnhancedBookmarksExperimentEnabled));
   // If user signed out, clear possible previous state.
   if (!user_signed_in) {
     bookmarks_experiment_state_before = BOOKMARKS_EXPERIMENT_NONE;
     ForceFinchBookmarkExperimentIfNeeded(flags_storage,
         BOOKMARKS_EXPERIMENT_NONE);
   }
@@ skipping 40 matching lines @@
              BOOKMARKS_EXPERIMENT_ENABLED_USER_OPT_OUT) {
     if (opt_out) {
       bookmarks_experiment_new_state =
           BOOKMARKS_EXPERIMENT_ENABLED_USER_OPT_OUT;
     } else {
       // User opted in again.
       bookmarks_experiment_new_state = BOOKMARKS_EXPERIMENT_ENABLED;
     }
   }
 
+#if defined(OS_ANDROID)
+  bool opt_in = !opt_out
+      && CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kEnhancedBookmarksExperiment) == "1";
+  if (opt_in && bookmarks_experiment_new_state == BOOKMARKS_EXPERIMENT_NONE)
+    bookmarks_experiment_new_state = BOOKMARKS_EXPERIMENT_ENABLED;
+#endif
+
   UMA_HISTOGRAM_ENUMERATION("EnhancedBookmarks.SyncExperimentState",
                             bookmarks_experiment_new_state,
                             BOOKMARKS_EXPERIMENT_ENUM_SIZE);
   user_prefs->SetInteger(
       sync_driver::prefs::kEnhancedBookmarksExperimentEnabled,
       bookmarks_experiment_new_state);
   ForceFinchBookmarkExperimentIfNeeded(flags_storage,
                                        bookmarks_experiment_new_state);
 }
 
@@ skipping 19 matching lines @@
         new base::StringValue(switches::kManualEnhancedBookmarks));
   } else if (bookmarks_experiment_state ==
                  BOOKMARKS_EXPERIMENT_ENABLED_USER_OPT_OUT) {
     experiments_list->Remove(
         base::StringValue(switches::kManualEnhancedBookmarks), &index);
     experiments_list->AppendIfNotPresent(
         new base::StringValue(switches::kManualEnhancedBookmarksOptout));
   }
 }
 
-bool IsEnhancedBookmarksExperimentEnabled() {
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kManualEnhancedBookmarks) ||
-      command_line->HasSwitch(switches::kManualEnhancedBookmarksOptout)) {
-    return true;
-  }
+#if defined(OS_ANDROID)
 
-  return IsEnhancedBookmarksExperimentEnabledFromFinch();
-}
-
-#if defined(OS_ANDROID)
-bool IsEnhancedBookmarkImageFetchingEnabled() {
-  if (IsEnhancedBookmarksExperimentEnabled())
+bool IsEnhancedBookmarkImageFetchingEnabled(PrefService* user_prefs,
+                                            PrefService* local_state) {
+  if (IsEnhancedBookmarksExperimentEnabled(user_prefs, local_state))
     return true;
 
   // Salient images are collected from visited bookmarked pages even if the
   // enhanced bookmark feature is turned off. This is to have some images
   // available so that in the future, when the feature is turned on, the user
   // experience is not a big list of flat colors. However as a precautionary
   // measure it is possible to disable this collection of images from finch.
   std::string disable_fetching = variations::GetVariationParamValue(
       kFieldTrialName, "DisableImagesFetching");
   return disable_fetching.empty();
 }
+
+bool IsEnhancedBookmarksExperimentEnabled(PrefService* user_prefs,
+                                          PrefService* local_state) {
+  PrefService* flags_storage = GetFlagsStorage(user_prefs, local_state);
+
+  const base::ListValue* experiments_list = flags_storage->GetList(
+      prefs::kEnabledLabsExperiments);
+
+  if (flags_storage) {
+    if (experiments_list->Find(
+        base::StringValue(switches::kManualEnhancedBookmarks))

[Yaron, 2014/08/28 00:17:40]

I don't understand the distinction here. Either explain here or elsewhere (if
needed) but I thought that if sync sets the flags, they are propagated to the
command line. What causes this to be handled differently from desktop other than
the fact that optin is allowed?

[Kibeom Kim (inactive), 2014/08/28 18:31:00]

Done.

+        != experiments_list->end()) {
+      return true;
+    } else if (experiments_list->Find(
+        base::StringValue(switches::kManualEnhancedBookmarksOptout))
+        != experiments_list->end()) {
+      return false;
+    }
+  }
+
+  return IsEnhancedBookmarksExperimentEnabledFromFinch();
+}
+
+#else
+
+bool IsEnhancedBookmarksExperimentEnabled() {
+  CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kManualEnhancedBookmarks) ||
+      command_line->HasSwitch(switches::kManualEnhancedBookmarksOptout)) {
+    return true;
+  }
+  return IsEnhancedBookmarksExperimentEnabledFromFinch();
+}
+
 #endif
 
 bool IsEnableDomDistillerSet() {
   if (CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kEnableDomDistiller)) {
     return true;
   }
   if (variations::GetVariationParamValue(
           kFieldTrialName, "enable-dom-distiller") == "1")
     return true;
 
   return false;
 }
 
 bool IsEnableSyncArticlesSet() {
   if (CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kEnableSyncArticles)) {
     return true;
   }
   if (variations::GetVariationParamValue(
           kFieldTrialName, "enable-sync-articles") == "1")
     return true;
 
   return false;
 }