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

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

kEnhancedBookmarksExperiment is used by android intrumentation tests to
force enhanced bookmark feature to be enabled.
https://codereview.chromium.org/1009673002 introduced a regression that
the even if the flag is set to be true, it does not take effect. This CL
fixes it.

BUG=468106
Committed: https://crrev.com/47c18b773aa75bdae2f4b39750534119bc3055ab
Cr-Commit-Position: refs/heads/master@{#321429}

[Ian Wen, 2015-03-17 23:24:47]

ianwen@chromium.org changed reviewers:
+ danduong@chromium.org, wittman@chromium.org

[Mike Wittman, 2015-03-17 23:26:23]

lgtm

[Ian Wen, 2015-03-17 23:31:39]

ianwen@chromium.org changed reviewers:
+ sky@chromium.org

[Ian Wen, 2015-03-17 23:31:39]

Hi sky@, could you please take a look?

Also, maybe we can make danduong@ or wittman@ or kkimabls@ or noyau@ the owner
of this file (enhanced_bookmarks_features.cc and enhanced_bookmarks_features.h),
so that we can spam you less. :)

[sky, 2015-03-18 00:01:50]

LGTM I'm fine with updating owners too

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

noyau@chromium.org changed reviewers:
+ noyau@chromium.org

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:80:
base::android::SdkVersion::SDK_VERSION_ICE_CREAM_SANDWICH_MR1;
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:41:59]

tedchoc@chromium.org changed reviewers:
+ tedchoc@chromium.org

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:73: #if
defined(OS_ANDROID)
Unless I'm reading this wrong, this is an Android only function, so this
shouldn't be needed.

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:79: opt_out =
base::android::BuildInfo::GetInstance()->sdk_int() <
this needs to be |-ed otherwise the opt_out above will never be used.

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:80:
base::android::SdkVersion::SDK_VERSION_ICE_CREAM_SANDWICH_MR1;
On 2015/03/18 13:32:14, noyau wrote:
> 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.

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.

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (left):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:47: 
This might still be needed for non-Android

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:40: #if
defined(OS_ANDROID)
Shouldn't this be IOS too?

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (left):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:47: 
On 2015/03/18 17:51:41, danduong wrote:
> This might still be needed for non-Android

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]

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (left):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:47: 
On 2015/03/18 18:10:19, Mike Wittman wrote:
> On 2015/03/18 17:51:41, danduong wrote:
> > This might still be needed for non-Android
> 
> 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...

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:73: #if
defined(OS_ANDROID)
On 2015/03/18 15:42:00, Ted C wrote:
> Unless I'm reading this wrong, this is an Android only function, so this
> shouldn't be needed.

Acknowledged.

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:79: opt_out =
base::android::BuildInfo::GetInstance()->sdk_int() <
On 2015/03/18 15:42:00, Ted C wrote:
> this needs to be |-ed otherwise the opt_out above will never be used.

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

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:40: #if
defined(OS_ANDROID)
On 2015/03/18 17:51:41, danduong wrote:
> Shouldn't this be IOS too?

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]

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:40: #if
defined(OS_ANDROID)
Thanks for the clarification.  I didn't realize that you guys mirrored this.

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

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:40: #if
defined(OS_ANDROID)
On 2015/03/18 23:28:56, danduong wrote:
> Thanks for the clarification.  I didn't realize that you guys mirrored this.

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…

[danduong, 2015-03-19 18:51:18]

lgtm

https://codereview.chromium.org/1017913002/diff/80001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1017913002/diff/80001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:23: #include
"extensions/common/features/feature_provider.h"
can you clean up the includes too?

[Mike Wittman, 2015-03-19 18:56:35]

https://codereview.chromium.org/1017913002/diff/80001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right):

https://codereview.chromium.org/1017913002/diff/80001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.h:16: bool
GetBookmarksExperimentExtensionID(std::string* extension_id);
this function can move inside the .cc file, since there are no external callers

[Ian Wen, 2015-03-19 20:16:36]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-03-19 20:16:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
wittman@chromium.org, sky@chromium.org, danduong@chromium.org
Link to the patchset: https://codereview.chromium.org/1017913002/#ps120001
(title: "Clean up includes")

[commit-bot: I haz the power, 2015-03-19 20:16:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017913002/120001

[Ian Wen, 2015-03-19 20:17:14]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2015-03-19 20:17:40]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-03-19 20:18:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017913002/120001

[commit-bot: I haz the power, 2015-03-19 21:03:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-19 21:04:29]

Patchset 7 (id:??) landed as
https://crrev.com/47c18b773aa75bdae2f4b39750534119bc3055ab
Cr-Commit-Position: refs/heads/master@{#321429}