AAPT2: Disable locale domination for deduping
Locale deduping isn't straightforward, as parenting rules
change between platform versions and the selection
preference of a specific locale variant over the default
configuration lead to incorrect results at runtime.
Bug: 62409213
Test: make aapt2_tests
Change-Id: Iec8f1cfba7ae43c847d163529891fdc15f3db826
diff --git a/tools/aapt2/ConfigDescription.cpp b/tools/aapt2/ConfigDescription.cpp
index 46098cb..7ff0c72 100644
--- a/tools/aapt2/ConfigDescription.cpp
+++ b/tools/aapt2/ConfigDescription.cpp
@@ -877,7 +877,16 @@
}
bool ConfigDescription::Dominates(const ConfigDescription& o) const {
- if (*this == DefaultConfig() || *this == o) {
+ if (*this == o) {
+ return true;
+ }
+
+ // Locale de-duping is not-trivial, disable for now (b/62409213).
+ if (diff(o) & CONFIG_LOCALE) {
+ return false;
+ }
+
+ if (*this == DefaultConfig()) {
return true;
}
return MatchWithDensity(o) && !o.MatchWithDensity(*this) &&
diff --git a/tools/aapt2/DominatorTree_test.cpp b/tools/aapt2/DominatorTree_test.cpp
index e89c6be..efc523f 100644
--- a/tools/aapt2/DominatorTree_test.cpp
+++ b/tools/aapt2/DominatorTree_test.cpp
@@ -69,14 +69,12 @@
TEST(DominatorTreeTest, DefaultDominatesEverything) {
const ConfigDescription default_config = {};
const ConfigDescription land_config = test::ParseConfigOrDie("land");
- const ConfigDescription sw600dp_land_config =
- test::ParseConfigOrDie("sw600dp-land-v13");
+ const ConfigDescription sw600dp_land_config = test::ParseConfigOrDie("sw600dp-land-v13");
std::vector<std::unique_ptr<ResourceConfigValue>> configs;
configs.push_back(util::make_unique<ResourceConfigValue>(default_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(land_config, ""));
- configs.push_back(
- util::make_unique<ResourceConfigValue>(sw600dp_land_config, ""));
+ configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_land_config, ""));
DominatorTree tree(configs);
PrettyPrinter printer;
@@ -91,16 +89,13 @@
TEST(DominatorTreeTest, ProductsAreDominatedSeparately) {
const ConfigDescription default_config = {};
const ConfigDescription land_config = test::ParseConfigOrDie("land");
- const ConfigDescription sw600dp_land_config =
- test::ParseConfigOrDie("sw600dp-land-v13");
+ const ConfigDescription sw600dp_land_config = test::ParseConfigOrDie("sw600dp-land-v13");
std::vector<std::unique_ptr<ResourceConfigValue>> configs;
configs.push_back(util::make_unique<ResourceConfigValue>(default_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(land_config, ""));
- configs.push_back(
- util::make_unique<ResourceConfigValue>(default_config, "phablet"));
- configs.push_back(
- util::make_unique<ResourceConfigValue>(sw600dp_land_config, "phablet"));
+ configs.push_back(util::make_unique<ResourceConfigValue>(default_config, "phablet"));
+ configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_land_config, "phablet"));
DominatorTree tree(configs);
PrettyPrinter printer;
@@ -118,16 +113,11 @@
const ConfigDescription en_config = test::ParseConfigOrDie("en");
const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");
const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl-v4");
- const ConfigDescription ldrtl_xhdpi_config =
- test::ParseConfigOrDie("ldrtl-xhdpi-v4");
- const ConfigDescription sw300dp_config =
- test::ParseConfigOrDie("sw300dp-v13");
- const ConfigDescription sw540dp_config =
- test::ParseConfigOrDie("sw540dp-v14");
- const ConfigDescription sw600dp_config =
- test::ParseConfigOrDie("sw600dp-v14");
- const ConfigDescription sw720dp_config =
- test::ParseConfigOrDie("sw720dp-v13");
+ const ConfigDescription ldrtl_xhdpi_config = test::ParseConfigOrDie("ldrtl-xhdpi-v4");
+ const ConfigDescription sw300dp_config = test::ParseConfigOrDie("sw300dp-v13");
+ const ConfigDescription sw540dp_config = test::ParseConfigOrDie("sw540dp-v14");
+ const ConfigDescription sw600dp_config = test::ParseConfigOrDie("sw600dp-v14");
+ const ConfigDescription sw720dp_config = test::ParseConfigOrDie("sw720dp-v13");
const ConfigDescription v20_config = test::ParseConfigOrDie("v20");
std::vector<std::unique_ptr<ResourceConfigValue>> configs;
@@ -135,8 +125,7 @@
configs.push_back(util::make_unique<ResourceConfigValue>(en_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(en_v21_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(ldrtl_config, ""));
- configs.push_back(
- util::make_unique<ResourceConfigValue>(ldrtl_xhdpi_config, ""));
+ configs.push_back(util::make_unique<ResourceConfigValue>(ldrtl_xhdpi_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(sw300dp_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(sw540dp_config, ""));
configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_config, ""));
@@ -148,15 +137,37 @@
std::string expected =
"<default>\n"
- " en\n"
- " en-v21\n"
" ldrtl-v4\n"
" ldrtl-xhdpi-v4\n"
" sw300dp-v13\n"
" sw540dp-v14\n"
" sw600dp-v14\n"
" sw720dp-v13\n"
- " v20\n";
+ " v20\n"
+ "en\n"
+ " en-v21\n";
+ EXPECT_EQ(expected, printer.ToString(&tree));
+}
+
+TEST(DominatorTreeTest, LocalesAreNeverDominated) {
+ const ConfigDescription fr_config = test::ParseConfigOrDie("fr");
+ const ConfigDescription fr_rCA_config = test::ParseConfigOrDie("fr-rCA");
+ const ConfigDescription fr_rFR_config = test::ParseConfigOrDie("fr-rFR");
+
+ std::vector<std::unique_ptr<ResourceConfigValue>> configs;
+ configs.push_back(util::make_unique<ResourceConfigValue>(ConfigDescription::DefaultConfig(), ""));
+ configs.push_back(util::make_unique<ResourceConfigValue>(fr_config, ""));
+ configs.push_back(util::make_unique<ResourceConfigValue>(fr_rCA_config, ""));
+ configs.push_back(util::make_unique<ResourceConfigValue>(fr_rFR_config, ""));
+
+ DominatorTree tree(configs);
+ PrettyPrinter printer;
+
+ std::string expected =
+ "<default>\n"
+ "fr\n"
+ "fr-rCA\n"
+ "fr-rFR\n";
EXPECT_EQ(expected, printer.ToString(&tree));
}
diff --git a/tools/aapt2/optimize/ResourceDeduper_test.cpp b/tools/aapt2/optimize/ResourceDeduper_test.cpp
index 4d00fa6..d9f384c0 100644
--- a/tools/aapt2/optimize/ResourceDeduper_test.cpp
+++ b/tools/aapt2/optimize/ResourceDeduper_test.cpp
@@ -19,69 +19,88 @@
#include "ResourceTable.h"
#include "test/Test.h"
+using ::aapt::test::HasValue;
+using ::testing::Not;
+
namespace aapt {
TEST(ResourceDeduperTest, SameValuesAreDeduped) {
std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
const ConfigDescription default_config = {};
+ const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl");
+ const ConfigDescription ldrtl_v21_config = test::ParseConfigOrDie("ldrtl-v21");
const ConfigDescription en_config = test::ParseConfigOrDie("en");
const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");
- // Chosen because this configuration is compatible with en.
+ // Chosen because this configuration is compatible with ldrtl/en.
const ConfigDescription land_config = test::ParseConfigOrDie("land");
std::unique_ptr<ResourceTable> table =
test::ResourceTableBuilder()
- .AddString("android:string/dedupe", ResourceId{}, default_config,
- "dedupe")
- .AddString("android:string/dedupe", ResourceId{}, en_config, "dedupe")
- .AddString("android:string/dedupe", ResourceId{}, land_config,
- "dedupe")
- .AddString("android:string/dedupe2", ResourceId{}, default_config,
- "dedupe")
- .AddString("android:string/dedupe2", ResourceId{}, en_config,
- "dedupe")
- .AddString("android:string/dedupe2", ResourceId{}, en_v21_config,
- "keep")
- .AddString("android:string/dedupe2", ResourceId{}, land_config,
- "dedupe")
+ .AddString("android:string/dedupe", ResourceId{}, default_config, "dedupe")
+ .AddString("android:string/dedupe", ResourceId{}, ldrtl_config, "dedupe")
+ .AddString("android:string/dedupe", ResourceId{}, land_config, "dedupe")
+
+ .AddString("android:string/dedupe2", ResourceId{}, default_config, "dedupe")
+ .AddString("android:string/dedupe2", ResourceId{}, ldrtl_config, "dedupe")
+ .AddString("android:string/dedupe2", ResourceId{}, ldrtl_v21_config, "keep")
+ .AddString("android:string/dedupe2", ResourceId{}, land_config, "dedupe")
+
+ .AddString("android:string/dedupe3", ResourceId{}, default_config, "dedupe")
+ .AddString("android:string/dedupe3", ResourceId{}, en_config, "dedupe")
+ .AddString("android:string/dedupe3", ResourceId{}, en_v21_config, "dedupe")
.Build();
ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
- EXPECT_EQ(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/dedupe", en_config));
- EXPECT_EQ(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/dedupe", land_config));
- EXPECT_EQ(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/dedupe2", en_config));
- EXPECT_NE(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/dedupe2", en_v21_config));
+ EXPECT_THAT(table, Not(HasValue("android:string/dedupe", ldrtl_config)));
+ EXPECT_THAT(table, Not(HasValue("android:string/dedupe", land_config)));
+
+ EXPECT_THAT(table, HasValue("android:string/dedupe2", ldrtl_v21_config));
+ EXPECT_THAT(table, Not(HasValue("android:string/dedupe2", ldrtl_config)));
+
+ EXPECT_THAT(table, HasValue("android:string/dedupe3", default_config));
+ EXPECT_THAT(table, HasValue("android:string/dedupe3", en_config));
+ EXPECT_THAT(table, Not(HasValue("android:string/dedupe3", en_v21_config)));
}
TEST(ResourceDeduperTest, DifferentValuesAreKept) {
std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
const ConfigDescription default_config = {};
- const ConfigDescription en_config = test::ParseConfigOrDie("en");
- const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");
- // Chosen because this configuration is compatible with en.
+ const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl");
+ const ConfigDescription ldrtl_v21_config = test::ParseConfigOrDie("ldrtl-v21");
+ // Chosen because this configuration is compatible with ldrtl.
const ConfigDescription land_config = test::ParseConfigOrDie("land");
std::unique_ptr<ResourceTable> table =
test::ResourceTableBuilder()
- .AddString("android:string/keep", ResourceId{}, default_config,
- "keep")
- .AddString("android:string/keep", ResourceId{}, en_config, "keep")
- .AddString("android:string/keep", ResourceId{}, en_v21_config,
- "keep2")
+ .AddString("android:string/keep", ResourceId{}, default_config, "keep")
+ .AddString("android:string/keep", ResourceId{}, ldrtl_config, "keep")
+ .AddString("android:string/keep", ResourceId{}, ldrtl_v21_config, "keep2")
.AddString("android:string/keep", ResourceId{}, land_config, "keep2")
.Build();
ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
- EXPECT_NE(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/keep", en_config));
- EXPECT_NE(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/keep", en_v21_config));
- EXPECT_NE(nullptr, test::GetValueForConfig<String>(
- table.get(), "android:string/keep", land_config));
+ EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_config));
+ EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_v21_config));
+ EXPECT_THAT(table, HasValue("android:string/keep", land_config));
+}
+
+TEST(ResourceDeduperTest, LocalesValuesAreKept) {
+ std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+ const ConfigDescription default_config = {};
+ const ConfigDescription fr_config = test::ParseConfigOrDie("fr");
+ const ConfigDescription fr_rCA_config = test::ParseConfigOrDie("fr-rCA");
+
+ std::unique_ptr<ResourceTable> table =
+ test::ResourceTableBuilder()
+ .AddString("android:string/keep", ResourceId{}, default_config, "keep")
+ .AddString("android:string/keep", ResourceId{}, fr_config, "keep")
+ .AddString("android:string/keep", ResourceId{}, fr_rCA_config, "keep")
+ .Build();
+
+ ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
+ EXPECT_THAT(table, HasValue("android:string/keep", default_config));
+ EXPECT_THAT(table, HasValue("android:string/keep", fr_config));
+ EXPECT_THAT(table, HasValue("android:string/keep", fr_rCA_config));
}
} // namespace aapt
diff --git a/tools/aapt2/test/Common.h b/tools/aapt2/test/Common.h
index 01b2d14..8efd56a 100644
--- a/tools/aapt2/test/Common.h
+++ b/tools/aapt2/test/Common.h
@@ -156,6 +156,23 @@
return arg.Equals(&a);
}
+MATCHER_P(StrValueEq, a,
+ std::string(negation ? "isn't" : "is") + " equal to " + ::testing::PrintToString(a)) {
+ return *(arg.value) == a;
+}
+
+MATCHER_P(HasValue, name,
+ std::string(negation ? "does not have" : "has") + " value " +
+ ::testing::PrintToString(name)) {
+ return GetValueForConfig<Value>(&(*arg), name, {}) != nullptr;
+}
+
+MATCHER_P2(HasValue, name, config,
+ std::string(negation ? "does not have" : "has") + " value " +
+ ::testing::PrintToString(name) + " for config " + ::testing::PrintToString(config)) {
+ return GetValueForConfig<Value>(&(*arg), name, config) != nullptr;
+}
+
} // namespace test
} // namespace aapt