Filter flags to enable ARC on non ARC installed boards.
Ignore ARC availability related flags if ARC container is not installed
on the board at all. Move ARC related logic from libchromeos-ui to
chromeos-login
Showing TOS and opt-in relate UI on non ARC installed board is
misleading to user and make noise to our statics. Add filter to ignore
flags to enbale ARC in /ect/chrome_dev.conf.
BUG=b:36067509
TEST=Add unit test for libchromeos-ui for added/modified api.
TEST=Manual test. Modify /etc/chrome_dev.conf to enable ARC on link. No
ARC related UI is shown.
Change-Id: Ia9561acb6b2e609518c8d64c3b17fb9bcead9ec9
Reviewed-on: https://chromium-review.googlesource.com/464201
Commit-Ready: Long Cheng <lgcheng@google.com>
Tested-by: Long Cheng <lgcheng@google.com>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/474086
Commit-Queue: Long Cheng <lgcheng@google.com>
Trybot-Ready: Long Cheng <lgcheng@google.com>
diff --git a/libchromeos-ui/chromeos/ui/chromium_command_builder.cc b/libchromeos-ui/chromeos/ui/chromium_command_builder.cc
index 36c3ddd..4b29d25 100644
--- a/libchromeos-ui/chromeos/ui/chromium_command_builder.cc
+++ b/libchromeos-ui/chromeos/ui/chromium_command_builder.cc
@@ -97,6 +97,15 @@
return false;
}
+// Returns true if |str| has prefix in |prefixes|.
+bool HasPrefix(const std::string& str, const std::set<std::string>& prefixes) {
+ for (const auto& prefix : prefixes) {
+ if (base::StartsWith(str, prefix, base::CompareCase::SENSITIVE))
+ return true;
+ }
+ return false;
+}
+
} // namespace
const char ChromiumCommandBuilder::kUser[] = "chronos";
@@ -114,6 +123,7 @@
gid_(0),
is_chrome_os_hardware_(false),
is_developer_end_user_(false),
+ is_test_build_(false),
vmodule_argument_index_(-1),
enable_features_argument_index_(-1) {
}
@@ -149,6 +159,17 @@
is_developer_end_user_ = base::GetAppOutput(
base::CommandLine(base::FilePath("is_developer_end_user")), &output);
+ // Provide /etc/lsb-release contents and timestamp so that they are available
+ // to Chrome immediately without requiring a blocking file read.
+ const base::FilePath lsb_path(GetPath(kLsbReleasePath));
+ base::File::Info info;
+ if (!base::ReadFileToString(lsb_path, &lsb_data_) ||
+ !base::GetFileInfo(lsb_path, &info)) {
+ LOG(ERROR) << "Unable to read or stat " << kLsbReleasePath;
+ return false;
+ }
+ lsb_release_time_ = info.creation_time;
+ is_test_build_ = IsTestBuild(lsb_data_);
return true;
}
@@ -165,19 +186,8 @@
if (!util::EnsureDirectoryExists(data_dir, uid_, gid_, 0755))
return false;
- // Provide /etc/lsb-release contents and timestamp so that they are available
- // to Chrome immediately without requiring a blocking file read.
- const base::FilePath lsb_path(GetPath(kLsbReleasePath));
- std::string lsb_data;
- base::File::Info info;
- if (!base::ReadFileToString(lsb_path, &lsb_data) ||
- !base::GetFileInfo(lsb_path, &info)) {
- LOG(ERROR) << "Unable to read or stat " << kLsbReleasePath;
- return false;
- }
- AddEnvVar("LSB_RELEASE", lsb_data);
- AddEnvVar("LSB_RELEASE_TIME",
- base::IntToString(info.creation_time.ToTimeT()));
+ AddEnvVar("LSB_RELEASE", lsb_data_);
+ AddEnvVar("LSB_RELEASE_TIME", base::IntToString(lsb_release_time_.ToTimeT()));
// By default, libdbus treats all warnings as fatal errors. That's too strict.
AddEnvVar("DBUS_FATAL_WARNINGS", "0");
@@ -217,11 +227,6 @@
SetUpPepperPlugins();
AddUiFlags();
- if (UseFlagIsSet("arc") || (UseFlagIsSet("cheets") && IsTestBuild(lsb_data)))
- AddArg("--arc-availability=officially-supported");
- else if (UseFlagIsSet("cheets"))
- AddArg("--arc-availability=installed");
-
if (UseFlagIsSet("pointer_events"))
AddFeatureEnableOverride("PointerEvent");
@@ -249,7 +254,8 @@
kPattern.c_str(), kPattern.size());
}
-bool ChromiumCommandBuilder::ApplyUserConfig(const base::FilePath& path) {
+bool ChromiumCommandBuilder::ApplyUserConfig(const base::FilePath& path,
+ const std::set<std::string>& disallowed_prefixes) {
std::string data;
if (!base::ReadFileToString(path, &data)) {
PLOG(WARNING) << "Unable to read " << path.value();
@@ -292,7 +298,7 @@
AddFeatureEnableOverride(pairs[0].second);
else if (pairs.size() == 1U && IsEnvironmentVariableName(pairs[0].first))
AddEnvVar(pairs[0].first, pairs[0].second);
- else
+ else if (!HasPrefix(line, disallowed_prefixes))
AddArg(line);
}
}
diff --git a/libchromeos-ui/chromeos/ui/chromium_command_builder.h b/libchromeos-ui/chromeos/ui/chromium_command_builder.h
index 442300a..64495b8 100644
--- a/libchromeos-ui/chromeos/ui/chromium_command_builder.h
+++ b/libchromeos-ui/chromeos/ui/chromium_command_builder.h
@@ -14,6 +14,7 @@
#include <base/files/file_path.h>
#include <base/macros.h>
+#include <base/time/time.h>
namespace chromeos {
namespace ui {
@@ -51,6 +52,7 @@
gid_t gid() const { return gid_; }
bool is_chrome_os_hardware() const { return is_chrome_os_hardware_; }
bool is_developer_end_user() const { return is_developer_end_user_; }
+ bool is_test_build() const { return is_test_build_; }
const StringMap& environment_variables() const {
return environment_variables_;
}
@@ -91,8 +93,10 @@
// NAME=VALUE
// Calls AddEnvVar("NAME", "VALUE").
//
+ // Any flags beginning with prefixes in |disallowed_prefixes| are disregarded.
// Returns true on success.
- bool ApplyUserConfig(const base::FilePath& path);
+ bool ApplyUserConfig(const base::FilePath& path,
+ const std::set<std::string>& disallowed_prefixes);
// Returns true if a USE flag named |flag| was set when the system image was
// built.
@@ -159,6 +163,16 @@
// True if this is a developer system, per the is_developer_end_user command.
bool is_developer_end_user_;
+ // True if this is a test build, per CHROMEOS_RELEASE_TRACK in
+ // /etc/lsb-release.
+ bool is_test_build_;
+
+ // Data in /etc/lsb-release.
+ std::string lsb_data_;
+
+ // Creation time of /etc/lsb-release.
+ base::Time lsb_release_time_;
+
// Environment variables that the caller should export before starting the
// executable.
StringMap environment_variables_;
diff --git a/libchromeos-ui/chromeos/ui/chromium_command_builder_unittest.cc b/libchromeos-ui/chromeos/ui/chromium_command_builder_unittest.cc
index eff6bd8..a519a73 100644
--- a/libchromeos-ui/chromeos/ui/chromium_command_builder_unittest.cc
+++ b/libchromeos-ui/chromeos/ui/chromium_command_builder_unittest.cc
@@ -112,8 +112,7 @@
TEST_F(ChromiumCommandBuilderTest, MissingLsbReleaseFile) {
write_lsb_release_file_ = false;
- ASSERT_TRUE(Init());
- EXPECT_FALSE(builder_.SetUpChromium());
+ EXPECT_FALSE(Init());
}
TEST_F(ChromiumCommandBuilderTest, LsbRelease) {
@@ -123,6 +122,13 @@
EXPECT_EQ(lsb_release_data_, ReadEnvVar("LSB_RELEASE"));
EXPECT_FALSE(ReadEnvVar("LSB_RELEASE_TIME").empty());
+ EXPECT_FALSE(builder_.is_test_build());
+}
+
+TEST_F(ChromiumCommandBuilderTest, IsTestBuild) {
+ lsb_release_data_ = "abc\nCHROMEOS_RELEASE_TRACK=testabc\ndef";
+ ASSERT_TRUE(Init());
+ EXPECT_TRUE(builder_.is_test_build());
}
TEST_F(ChromiumCommandBuilderTest, TimeZone) {
@@ -223,8 +229,9 @@
"!--blah\n";
base::FilePath path(util::GetReparentedPath("/config.txt", base_path_));
ASSERT_EQ(strlen(kConfig), base::WriteFile(path, kConfig, strlen(kConfig)));
+ std::set<std::string> disallowed_prefixes;
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
ASSERT_EQ(2U, builder_.arguments().size());
EXPECT_EQ("--foo=1", builder_.arguments()[0]);
EXPECT_EQ("--bar=3", builder_.arguments()[1]);
@@ -232,6 +239,34 @@
EXPECT_EQ("4", ReadEnvVar("BAR"));
}
+TEST_F(ChromiumCommandBuilderTest, UserConfigWithDisallowedPrefixes) {
+ ASSERT_TRUE(Init());
+ ASSERT_TRUE(builder_.SetUpChromium());
+
+ size_t default_size = builder_.arguments().size();
+
+ const char kConfig[] =
+ "# Here's a comment followed by 3 lines with disallowed prefixes and\n"
+ "# 2 lines without disallowed prefixes.\n"
+ "--disallowed-prefix1=bar\n"
+ " --disallowed-prefix2=bar\n"
+ "--notallowed-prefix3=bar\n"
+ "--Disallowed-prefix=foo\n"
+ "--allowed-prefix=foo";
+
+ base::FilePath path(util::GetReparentedPath("/config.txt", base_path_));
+ ASSERT_EQ(strlen(kConfig), base::WriteFile(path, kConfig, strlen(kConfig)));
+ std::set<std::string> disallowed_prefixes;
+
+ disallowed_prefixes.insert("--disallowed-prefix");
+ disallowed_prefixes.insert("--notallowed-prefix");
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
+
+ ASSERT_EQ(default_size + 2, builder_.arguments().size());
+ EXPECT_EQ("--Disallowed-prefix=foo", builder_.arguments()[default_size]);
+ EXPECT_EQ("--allowed-prefix=foo", builder_.arguments()[default_size + 1]);
+}
+
TEST_F(ChromiumCommandBuilderTest, UserConfigVmodule) {
const char kPrefix[] = "--vmodule=";
@@ -245,7 +280,9 @@
const char kConfig[] = "!--foo\n!--bar";
base::FilePath path(util::GetReparentedPath("/config.txt", base_path_));
ASSERT_EQ(strlen(kConfig), base::WriteFile(path, kConfig, strlen(kConfig)));
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ std::set<std::string> disallowed_prefixes;
+
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
builder_.AddVmodulePattern("b=1");
ASSERT_EQ("--vmodule=a=2,b=1", GetFirstArgWithPrefix(kPrefix));
@@ -253,7 +290,7 @@
const char kConfig2[] = "!--vmodule=";
ASSERT_EQ(strlen(kConfig2),
base::WriteFile(path, kConfig2, strlen(kConfig2)));
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
EXPECT_TRUE(builder_.arguments().empty());
// Now add another vmodule pattern and check that the flag is re-added.
@@ -264,7 +301,7 @@
const char kConfig3[] = "vmodule=a=1\nvmodule=b=2";
ASSERT_EQ(strlen(kConfig3),
base::WriteFile(path, kConfig3, strlen(kConfig3)));
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
ASSERT_EQ("--vmodule=c=1,a=1,b=2", GetFirstArgWithPrefix(kPrefix));
// Also check that literal "vmodule=..." arguments don't get added.
@@ -282,9 +319,11 @@
// Check that we don't get confused when deleting flags surrounding the
// feature flag.
const char kConfig[] = "!--foo\n!--bar";
+ std::set<std::string> disallowed_prefixes;
+
base::FilePath path(util::GetReparentedPath("/config.txt", base_path_));
ASSERT_EQ(strlen(kConfig), base::WriteFile(path, kConfig, strlen(kConfig)));
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
builder_.AddFeatureEnableOverride("b");
ASSERT_EQ("--enable-features=a,b", GetFirstArgWithPrefix(kPrefix));
@@ -292,7 +331,7 @@
const char kConfig2[] = "!--enable-features=";
ASSERT_EQ(strlen(kConfig2),
base::WriteFile(path, kConfig2, strlen(kConfig2)));
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
EXPECT_TRUE(builder_.arguments().empty());
// Now add another feature and check that the flag is re-added.
@@ -303,7 +342,7 @@
const char kConfig3[] = "enable-features=d\nenable-features=e";
ASSERT_EQ(strlen(kConfig3),
base::WriteFile(path, kConfig3, strlen(kConfig3)));
- ASSERT_TRUE(builder_.ApplyUserConfig(path));
+ ASSERT_TRUE(builder_.ApplyUserConfig(path, disallowed_prefixes));
ASSERT_EQ("--enable-features=c,d,e", GetFirstArgWithPrefix(kPrefix));
// Also check that literal "enable-features=..." arguments don't get added.
diff --git a/login_manager/chrome_setup.cc b/login_manager/chrome_setup.cc
index f96a3c5..548cfd7 100644
--- a/login_manager/chrome_setup.cc
+++ b/login_manager/chrome_setup.cc
@@ -7,6 +7,8 @@
#include <sys/stat.h>
#include <unistd.h>
+#include <set>
+
#include <base/bind.h>
#include <base/files/file_path.h>
#include <base/files/file_util.h>
@@ -69,6 +71,23 @@
return true;
}
+// Adds ARC related flags.
+void AddArcFlags(ChromiumCommandBuilder* builder,
+ std::set<std::string>* disallowed_params_out) {
+ if (builder->UseFlagIsSet("arc") ||
+ (builder->UseFlagIsSet("cheets") && builder->is_test_build())) {
+ builder->AddArg("--arc-availability=officially-supported");
+ } else if (builder->UseFlagIsSet("cheets")) {
+ builder->AddArg("--arc-availability=installed");
+ } else {
+ // Don't pass ARC availability related flags in chrome_dev.conf to Chrome if
+ // ARC is not installed at all.
+ disallowed_params_out->insert("--arc-availability");
+ disallowed_params_out->insert("--enable-arc");
+ disallowed_params_out->insert("--arc-available");
+ }
+}
+
// Ensures that necessary directory exist with the correct permissions and sets
// related arguments and environment variables.
void CreateDirectories(ChromiumCommandBuilder* builder) {
@@ -373,13 +392,15 @@
DCHECK(uid_out);
ChromiumCommandBuilder builder;
+ std::set<std::string> disallowed_prefixes;
CHECK(builder.Init());
- builder.SetUpChromium();
+ CHECK(builder.SetUpChromium());
// Please add new code to the most-appropriate helper function instead of
// putting it here. Things that to all Chromium-derived binaries (e.g.
// app_shell, content_shell, etc.) rather than just to Chrome belong in the
// ChromiumCommandBuilder class instead.
+ AddArcFlags(&builder, &disallowed_prefixes);
CreateDirectories(&builder);
InitCrashHandling(&builder);
AddSystemFlags(&builder);
@@ -388,8 +409,10 @@
AddVmodulePatterns(&builder);
// Apply any modifications requested by the developer.
- if (builder.is_developer_end_user())
- builder.ApplyUserConfig(base::FilePath(kChromeDevConfigPath));
+ if (builder.is_developer_end_user()) {
+ builder.ApplyUserConfig(base::FilePath(kChromeDevConfigPath),
+ disallowed_prefixes);
+ }
*is_developer_end_user_out = builder.is_developer_end_user();
*env_vars_out = builder.environment_variables();
