[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 111 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(const PrefService* user_prefs) {
+  if (IsEnhancedBookmarksExperimentEnabled(user_prefs))
     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(const PrefService* user_prefs) {
+  BookmarksExperimentState bookmarks_experiment_state =
+      static_cast<BookmarksExperimentState>(user_prefs->GetInteger(
+          sync_driver::prefs::kEnhancedBookmarksExperimentEnabled));
+  return bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED

[Yaron, 2014/08/27 20:42:15]

Why can't you use the switches variant? Isn't that set by
ForceFinchBookmarkExperimentIfNeeded which runsa fter you opt_in code?

[Kibeom Kim (inactive), 2014/08/27 21:11:41]

I see.

I was following the logic here
https://chromereviews.googleplex.com/37287013/diff/30010/chrome/browser/bookm...

but looks like the following codeblock should have the same effect.

bool IsEnhancedBookmarksExperimentEnabled() {
  CommandLine* command_line = CommandLine::ForCurrentProcess();
  if (command_line->HasSwitch(switches::kManualEnhancedBookmarks)) {
    return true;
  }
  return IsEnhancedBookmarksExperimentEnabledFromFinch();
}


Actually, I'm not sure why the current IsEnhancedBookmarksExperimentEnabled()
returns true on kManualEnhancedBookmarksOptout switch. Do you know? (or I guess
I should ask yefim@)

+      || bookmarks_experiment_state == BOOKMARKS_EXPERIMENT_ENABLED_FROM_FINCH;
+}
+
+#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;
 }