Remove enhanced bookmarks sync experiment

Remove enhanced bookmarks sync experiment

Uses the enhanced bookmarks Finch experiment as the only mechanism for
enabling enhanced bookmarks in Chrome. With this change we no longer
need the EnhancedBookmarks.SyncExperimentState UMA histogram since we
can now get all the relevant metrics via Finch.

BUG=467040
Committed: https://crrev.com/c8ae1d65bada9e45e2fcfa4a00f0f18018e823bb
Cr-Commit-Position: refs/heads/master@{#320973}

[Mike Wittman, 2015-03-16 17:47:06]

wittman@chromium.org changed reviewers:
+ finnur@chromium.org

[Mike Wittman, 2015-03-16 17:48:47]

wittman@chromium.org changed reviewers:
+ zea@chromium.org

[Mike Wittman, 2015-03-16 17:49:44]

wittman@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Mike Wittman, 2015-03-16 17:52:47]

wittman@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Mike Wittman, 2015-03-16 17:53:32]

wittman@chromium.org changed reviewers:
+ mpearson@chromium.org
- asvitkine@chromium.org

[Mike Wittman, 2015-03-16 17:53:54]

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

[Mike Wittman, 2015-03-16 18:05:51]

wittman@chromium.org changed reviewers:
+ kkimlabs@chromium.org

[Mike Wittman, 2015-03-16 18:06:51]

wittman@chromium.org changed reviewers:
+ sky@google.com
- kkimlabs@chromium.org, sky@chromium.org

[Mike Wittman, 2015-03-16 18:14:14]

wittman@chromium.org changed reviewers:
+ sky@chromium.org
- sky@google.com

[Mike Wittman, 2015-03-16 18:16:05]

PTAL

zea:
 chrome/browser/sync
 components/sync_driver
 sync

yfriedman:
 chrome/browser/android
 chrome/browser/chrome_browser_main_android.cc
 chrome/browser/enhanced_bookmarks/android/enhanced_bookmark_tab_helper.cc

finnur:
 chrome/browser/extensions/external_component_loader.cc

mpearson:
 tools/metrics/histograms/histograms.xml

sky:
 chrome/browser
 chrome/common/chrome_switches.cc
 components/enhanced_bookmarks

[Yaron, 2015-03-16 19:41:21]

lgtm

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

https://codereview.chromium.org/1009673002/diff/60001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:67: void
UpdateBookmarksExperimentState(
I don't think anybody will miss this function :)

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6805: -   
enum="BookmarksExperimentState">
You can remove the <enum> too

[Mark P, 2015-03-16 19:46:14]

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6805: -   
enum="BookmarksExperimentState">
Do not remove a histogram declaration here.  Instead, mark it as <obsolete> with
the appropriate explanation.

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:52821: -  <int value="838887742"
label="manual-enhanced-bookmarks"/>
Please do not remove this or the enum below.  It helps keep the data in logs
interpretable.

[Mike Wittman, 2015-03-16 20:15:48]

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6805: -   
enum="BookmarksExperimentState">
On 2015/03/16 19:46:13, Mark P wrote:
> Do not remove a histogram declaration here.  Instead, mark it as <obsolete>
with
> the appropriate explanation.

Done.

https://codereview.chromium.org/1009673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:52821: -  <int value="838887742"
label="manual-enhanced-bookmarks"/>
On 2015/03/16 19:46:14, Mark P wrote:
> Please do not remove this or the enum below.  It helps keep the data in logs
> interpretable.

Done.

[Mark P, 2015-03-16 21:02:39]

histograms.xml lgtm

[Nicolas Zea, 2015-03-16 21:24:05]

sync LGTM! Thanks for cleaning this up

[sky, 2015-03-16 21:35:33]

LGTM

[Finnur, 2015-03-17 13:50:19]

external_component_loader.cc LGTM

[Mike Wittman, 2015-03-17 19:43:30]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-03-17 19:43:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, finnur@chromium.org, zea@chromium.org,
mpearson@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1009673002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-17 19:44:07]

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

[commit-bot: I haz the power, 2015-03-17 21:15:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-17 21:16:53]

Patchset 6 (id:??) landed as
https://crrev.com/c8ae1d65bada9e45e2fcfa4a00f0f18018e823bb
Cr-Commit-Position: refs/heads/master@{#320973}

[Ted C, 2015-03-17 22:58:10]

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

[Ted C, 2015-03-17 22:58:11]

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:43:
switches::kEnhancedBookmarksExperiment) == "0";
This seems like it removed the ability to opt into enhanced bookmarks via the
about:flags page right?  That seems like a bad thing (and it broke our tests
down stream).

Shouldn't we be checking if the value == "1" above the GetVaritionParamValue
check above?

[danduong, 2015-03-17 23:03:54]

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

[danduong, 2015-03-17 23:03:55]

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:43:
switches::kEnhancedBookmarksExperiment) == "0";
On 2015/03/17 22:58:11, Ted C wrote:
> This seems like it removed the ability to opt into enhanced bookmarks via the
> about:flags page right?  That seems like a bad thing (and it broke our tests
> down stream).
> 
> Shouldn't we be checking if the value == "1" above the GetVaritionParamValue
> check above?

Ah, that's right. On mobile, the extension_id is not technically required. Sorry
about that.

[Mike Wittman, 2015-03-17 23:12:00]

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:43:
switches::kEnhancedBookmarksExperiment) == "0";
On 2015/03/17 22:58:11, Ted C wrote:
> This seems like it removed the ability to opt into enhanced bookmarks via the
> about:flags page right?  That seems like a bad thing (and it broke our tests
> down stream).

Yes. We'd have to continue to force the Finch experiment to support it on
desktop. I'm not sure it's worth the complexity there since we plan to roll out
more widely soon.

> Shouldn't we be checking if the value == "1" above the GetVaritionParamValue
> check above?

Sure, I'll update the logic to preserve the opt-in support for mobile.

[danduong, 2015-03-17 23:15:44]

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/bookmar...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:43:
switches::kEnhancedBookmarksExperiment) == "0";
On 2015/03/17 23:11:59, Mike Wittman wrote:
> On 2015/03/17 22:58:11, Ted C wrote:
> > This seems like it removed the ability to opt into enhanced bookmarks via
the
> > about:flags page right?  That seems like a bad thing (and it broke our tests
> > down stream).
> 
> Yes. We'd have to continue to force the Finch experiment to support it on
> desktop. I'm not sure it's worth the complexity there since we plan to roll
out
> more widely soon.
> 
> > Shouldn't we be checking if the value == "1" above the GetVaritionParamValue
> > check above?
> 
> Sure, I'll update the logic to preserve the opt-in support for mobile.

Talked to Ian about it. He'll provide the fix. This is probably an awkward
method for that.  For mobile, we should ifdef a section in
IsEnhancedBookmarksEnabled() to fix this.

[danduong, 2015-03-18 03:16:03]

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1009673002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2376: FlagsStorage* flags_storage) {
Can we drop flags_stoage here from the list of params?