Commit ff0c52fb authored by yefim's avatar yefim Committed by Commit bot

Workaround to remove command line flag on ChromeOS.

When bookmarks experiment is enabled existing code will add a command line flag that will allow opt-out to be shown on a chrome:flags page.
On Chrome OS at time of login if they see command line flag from any experiment they restart browser. It creates delay and user will see a black screen for a couple seconds. It happens on every login, not only the first one.
I talked to CrOS devs and they strongly discourage from using command line flags.
So for Chrome OS only do not add flag to command line but still keep it in a flag_storage. Use flag storage when need to check if flag is set.

BUG=

Review URL: https://codereview.chromium.org/578333003

Cr-Commit-Position: refs/heads/master@{#296504}
parent 5a25f300
...@@ -2023,7 +2023,8 @@ void GetSanitizedEnabledFlags( ...@@ -2023,7 +2023,8 @@ void GetSanitizedEnabledFlags(
*result = flags_storage->GetFlags(); *result = flags_storage->GetFlags();
} }
bool SkipConditionalExperiment(const Experiment& experiment) { bool SkipConditionalExperiment(const Experiment& experiment,
FlagsStorage* flags_storage) {
if (experiment.internal_name == if (experiment.internal_name ==
std::string("enhanced-bookmarks-experiment")) { std::string("enhanced-bookmarks-experiment")) {
#if defined(OS_ANDROID) #if defined(OS_ANDROID)
...@@ -2036,7 +2037,7 @@ bool SkipConditionalExperiment(const Experiment& experiment) { ...@@ -2036,7 +2037,7 @@ bool SkipConditionalExperiment(const Experiment& experiment) {
if (command_line->HasSwitch(switches::kEnhancedBookmarksExperiment)) if (command_line->HasSwitch(switches::kEnhancedBookmarksExperiment))
return false; return false;
return !IsEnhancedBookmarksExperimentEnabled(); return !IsEnhancedBookmarksExperimentEnabled(flags_storage);
#endif #endif
} }
...@@ -2178,7 +2179,7 @@ void GetFlagsExperimentsData(FlagsStorage* flags_storage, ...@@ -2178,7 +2179,7 @@ void GetFlagsExperimentsData(FlagsStorage* flags_storage,
for (size_t i = 0; i < num_experiments; ++i) { for (size_t i = 0; i < num_experiments; ++i) {
const Experiment& experiment = experiments[i]; const Experiment& experiment = experiments[i];
if (SkipConditionalExperiment(experiment)) if (SkipConditionalExperiment(experiment, flags_storage))
continue; continue;
base::DictionaryValue* data = new base::DictionaryValue(); base::DictionaryValue* data = new base::DictionaryValue();
...@@ -2375,6 +2376,14 @@ void FlagsState::ConvertFlagsToSwitches(FlagsStorage* flags_storage, ...@@ -2375,6 +2376,14 @@ void FlagsState::ConvertFlagsToSwitches(FlagsStorage* flags_storage,
continue; continue;
} }
#if defined(OS_CHROMEOS)
// On Chrome OS setting command line flag may make browser to restart on
// user login. As this flag eventually will be set to a significant number
// of users skip manual-enhanced-bookmarks to avoid restart.
if (experiment_name == "manual-enhanced-bookmarks")
continue;
#endif
const std::pair<std::string, std::string>& const std::pair<std::string, std::string>&
switch_and_value_pair = name_to_switch_it->second; switch_and_value_pair = name_to_switch_it->second;
......
...@@ -34,4 +34,10 @@ specific_include_rules = { ...@@ -34,4 +34,10 @@ specific_include_rules = {
"+chrome/test/base/testing_profile.h", "+chrome/test/base/testing_profile.h",
"+chrome/utility/importer/bookmark_html_reader.h", "+chrome/utility/importer/bookmark_html_reader.h",
], ],
# Allow to include flags_storage into enhanced_bookmarks_features.cc because
# it has a function used by about_flags.cc and it needs flags_storage.
# This should be removed after enhanced bookmarks experiment is over.
'enhanced_bookmarks_features\.cc': [
"+chrome/browser/flags_storage.h",
],
} }
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#include "base/sha1.h" #include "base/sha1.h"
#include "base/strings/string_number_conversions.h" #include "base/strings/string_number_conversions.h"
#include "chrome/browser/browser_process.h" #include "chrome/browser/browser_process.h"
#include "chrome/browser/flags_storage.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_switches.h"
...@@ -190,12 +191,23 @@ void ForceFinchBookmarkExperimentIfNeeded( ...@@ -190,12 +191,23 @@ void ForceFinchBookmarkExperimentIfNeeded(
} }
} }
bool IsEnhancedBookmarksExperimentEnabled() { bool IsEnhancedBookmarksExperimentEnabled(
about_flags::FlagsStorage* flags_storage) {
#if defined(OS_CHROMEOS)
// We are not setting command line flags on Chrome OS to avoid browser restart
// but still have flags in flags_storage. So check flags_storage instead.
const std::set<std::string> flags = flags_storage->GetFlags();
if (flags.find(switches::kManualEnhancedBookmarks) != flags.end())
return true;
if (flags.find(switches::kManualEnhancedBookmarksOptout) != flags.end())
return true;
#else
CommandLine* command_line = CommandLine::ForCurrentProcess(); CommandLine* command_line = CommandLine::ForCurrentProcess();
if (command_line->HasSwitch(switches::kManualEnhancedBookmarks) || if (command_line->HasSwitch(switches::kManualEnhancedBookmarks) ||
command_line->HasSwitch(switches::kManualEnhancedBookmarksOptout)) { command_line->HasSwitch(switches::kManualEnhancedBookmarksOptout)) {
return true; return true;
} }
#endif
return IsEnhancedBookmarksExperimentEnabledFromFinch(); return IsEnhancedBookmarksExperimentEnabledFromFinch();
} }
......
...@@ -9,6 +9,10 @@ ...@@ -9,6 +9,10 @@
#include "extensions/common/extension.h" #include "extensions/common/extension.h"
namespace about_flags {
class FlagsStorage;
} // namespace about_flags
class PrefService; class PrefService;
class Profile; class Profile;
...@@ -54,7 +58,8 @@ void ForceFinchBookmarkExperimentIfNeeded( ...@@ -54,7 +58,8 @@ void ForceFinchBookmarkExperimentIfNeeded(
// Experiment could run by Chrome sync or by Finch. // Experiment could run by Chrome sync or by Finch.
// Note that this doesn't necessarily mean that enhanced bookmarks // Note that this doesn't necessarily mean that enhanced bookmarks
// is enabled, e.g., user can opt out using a flag. // is enabled, e.g., user can opt out using a flag.
bool IsEnhancedBookmarksExperimentEnabled(); bool IsEnhancedBookmarksExperimentEnabled(
about_flags::FlagsStorage* flags_storage);
#if defined(OS_ANDROID) #if defined(OS_ANDROID)
// Returns true if enhanced bookmark salient image prefetching is enabled. // Returns true if enhanced bookmark salient image prefetching is enabled.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment