Force Enhanced Bookmark to be enabled if command line flags are set true

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 19 matching lines @@
 
 const char kFieldTrialName[] = "EnhancedBookmarks";
 
 }  // namespace
 
 bool GetBookmarksExperimentExtensionID(std::string* extension_id) {
   *extension_id = variations::GetVariationParamValue(kFieldTrialName, "id");
   if (extension_id->empty())
     return false;
 
-  // kEnhancedBookmarksExperiment flag could have values "", "1" and "0".
-  // "0" - user opted out.
-  bool opt_out = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-      switches::kEnhancedBookmarksExperiment) == "0";
-
-  if (opt_out)
-    return false;
-

[danduong, 2015/03/18 17:51:41]

This might still be needed for non-Android

[Mike Wittman, 2015/03/18 18:10:19]

Yes, it is. if you're moving this to IsEnhancedBookmarksEnabled() you'll need to
update this code to check both IsEnhancedBookmarksEnabled() and
GetBookmarksExperimentExtensionID():
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[Ian Wen, 2015/03/18 18:45:55]

Reverted this part. This check should be kept here as it is used by desktop.
Done.

 #if defined(OS_ANDROID)

[danduong, 2015/03/18 17:51:41]

Shouldn't this be IOS too?

[noyau (Ping after 24h), 2015/03/18 23:21:58]

Probably, but we do not compile this file for iOS in Chromium: The goal is that
none of the code in chrome/ should be used on iOS. For now the iOS mirror this
file with an additional test in the ios repository (which is getting smaller by
the day). Ideally this file should loose all its chrome and extension
dependencies and move to components where it could be properly shared. Now that
finch moved into components, this should probably be sone sooner rather than
later actually.

[danduong, 2015/03/18 23:28:56]

Thanks for the clarification.  I didn't realize that you guys mirrored this.

[noyau (Ping after 24h), 2015/03/19 12:22:26]

That's why I have a watch on this file, because when one of you guys does
anything to this file someone on iOS land has to figure out what the heck
happened and make it work for iOS as well.

So, yes, plea: move as much stuff as possible to components, where we could use
it in iOS, and use tests to make sure the changes don't introduce iOS
regressions using Chromium bots…

-  return base::android::BuildInfo::GetInstance()->sdk_int() >
-         base::android::SdkVersion::SDK_VERSION_ICE_CREAM_SANDWICH_MR1;
+  return true;
 #else
   const extensions::FeatureProvider* feature_provider =
       extensions::FeatureProvider::GetPermissionFeatures();
   extensions::Feature* feature = feature_provider->GetFeature("metricsPrivate");
   return feature && feature->IsIdInWhitelist(*extension_id);
 #endif
 }
 
 #if defined(OS_ANDROID)
 bool IsEnhancedBookmarkImageFetchingEnabled(const PrefService* user_prefs) {
   if (IsEnhancedBookmarksEnabled())
     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 IsEnhancedBookmarksEnabled() {
   std::string extension_id;
+
+  // kEnhancedBookmarksExperiment flag could have values "", "1" and "0".
+  // "0" - user opted out.
+  bool opt_out = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+                     switches::kEnhancedBookmarksExperiment) == "0";
+
+#if defined(OS_ANDROID)

[Ted C, 2015/03/18 15:42:00]

Unless I'm reading this wrong, this is an Android only function, so this
shouldn't be needed.

[Ian Wen, 2015/03/18 18:45:55]

Acknowledged.

+  bool opt_in = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+                    switches::kEnhancedBookmarksExperiment) == "1";
+  if (opt_in)
+    return true;
+
+  opt_out = base::android::BuildInfo::GetInstance()->sdk_int() <

[Ted C, 2015/03/18 15:42:00]

this needs to be |-ed otherwise the opt_out above will never be used.

[Ian Wen, 2015/03/18 18:45:55]

Oops sorry, I did not test this CL last night before submitting as I was
developing on origin/master and it could not compile...

+            base::android::SdkVersion::SDK_VERSION_ICE_CREAM_SANDWICH_MR1;

[noyau (Ping after 24h), 2015/03/18 13:32:14]

Unless I misunderstand this will skew the finch experiment: opting out like this
will still report those users to be part of the experiment and will lead to
incorrect data.

The opt-in is fine, as it is based on a command line flag which could be
detected by finch.

[Ted C, 2015/03/18 15:42:00]

We just need to make sure this ICS logic is always read before attempting to
query
whether we are in the finch group or not.

A client will only report they are in the finch group if they actually ask finch
what group they are in.  If we early exit on ICS, then they'll never ask what
group
and will not mess with the data.

I have no idea if this is actually done early enough though, so we'd need to
check
that.

+#endif
+
+  if (opt_out)
+    return false;
+
   return GetBookmarksExperimentExtensionID(&extension_id);
 }
 #endif
 
 bool IsEnableDomDistillerSet() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableDomDistiller)) {
     return true;
   }
   if (variations::GetVariationParamValue(
           kFieldTrialName, "enable-dom-distiller") == "1")
     return true;
 
   return false;
 }
 
 bool IsEnableSyncArticlesSet() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableSyncArticles)) {
     return true;
   }
   if (variations::GetVariationParamValue(
           kFieldTrialName, "enable-sync-articles") == "1")
     return true;
 
   return false;
 }