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

chrome/browser/bookmarks/enhanced_bookmarks_features.cc

Index: chrome/browser/bookmarks/enhanced_bookmarks_features.cc
diff --git a/chrome/browser/bookmarks/enhanced_bookmarks_features.cc b/chrome/browser/bookmarks/enhanced_bookmarks_features.cc
index 9f681c6b38764215cc496431cb9dfae13fd0139f..4aa2a011df4b3d3968c5073276e08d41d5386392 100644
--- a/chrome/browser/bookmarks/enhanced_bookmarks_features.cc
+++ b/chrome/browser/bookmarks/enhanced_bookmarks_features.cc
@@ -37,17 +37,8 @@ bool GetBookmarksExperimentExtensionID(std::string* extension_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();
@@ -73,6 +64,25 @@ bool IsEnhancedBookmarkImageFetchingEnabled(const PrefService* user_prefs) {
 
 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