Restored an option to enable enhanced bookmarks experiment from Finch

chrome/browser/bookmarks/enhanced_bookmarks_features.h

 // 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.
 
 #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_BOOKMARKS_FEATURES_H_
 #define CHROME_BROWSER_BOOKMARKS_ENHANCED_BOOKMARKS_FEATURES_H_
 
 #include <string>
 
 #include "extensions/common/extension.h"
 
 class PrefService;
 
 enum BookmarksExperimentState {

[brettw, 2014/05/08 20:12:04]

Can you have a comment above here giving an overview of the experiment states
and what they mean? It seems there are two similar but different experiment
types that may mean different things.

[yefimt, 2014/05/08 20:32:31]

Done.

   kNoBookmarksExperiment,
   kBookmarksExperimentEnabled,
-  kBookmarksExperimentEnabledUserOptOut
+  kBookmarksExperimentEnabledUserOptOut,
+  kBookmarksExperimentEnabledFromFinch,
+  kBookmarksExperimentOptOutFromFinch,
+  kBookmarksExperimentEnabledFromFinchUserSignedIn,
+  kBookmarksExperimentEnumSize
 };
 
+bool GetBookmarksExperimentExtensionID(const PrefService* user_prefs,

[brettw, 2014/05/08 20:12:04]

This header file is quite confusing. There are no comments on these two new
functions. There is a function UpdateBookmarksExperimentState and
UpdateBookmarksExperiment and I don't see how these are supposed to be different
and why I might call one or the other.

[yefimt, 2014/05/08 20:32:31]

Done.

+                                       std::string* extension_id);
+
+void UpdateBookmarksExperimentState(const PrefService* user_prefs,
+                                    PrefService* local_state,
+                                    bool user_signed_in);
+
 // Sets flag to opt-in user into Finch experiment.
 void UpdateBookmarksExperiment(
     PrefService* local_state,
     BookmarksExperimentState bookmarks_experiment_state);
 
 // Returns true if enhanced bookmarks experiment is enabled.

[brettw, 2014/05/08 20:12:04]

I think this should say mroe clearly how this is different than the Finch one.

[yefimt, 2014/05/08 20:32:31]

Done.

 bool IsEnhancedBookmarksExperimentEnabled();
 
+// Returns true if enhanced bookmarks experiment is enabled from Finch.
+bool IsEnhancedBookmarksExperimentEnabledFromFinch();

[brettw, 2014/05/08 20:12:04]

Does this need to be a public function or can it be in an anon namespace in the
.cc file? It doesn't seem to be used elsewhere and its presence is confusing
here. Can you do the same with any of the other functions?

[yefimt, 2014/05/08 20:32:31]

Done.

+
 // Returns true when flag enable-dom-distiller is set or enabled from Finch.
 bool IsEnableDomDistillerSet();
 
 // Returns true when flag enable-sync-articles is set or enabled from Finch.
 bool IsEnableSyncArticlesSet();
 
 // Get extension id from Finch EnhancedBookmarks group parameters.
 std::string GetEnhancedBookmarksExtensionIdFromFinch();
 
 #endif  // CHROME_BROWSER_BOOKMARKS_ENHANCED_BOOKMARKS_FEATURES_H_