Move matching Tagalog and Filipino to ResourceTypes.cpp

Previously, if a mix of "fil" and "tl" resources existed in Resources
(from mixing resources from libraries for example), only resources
from one or the other would be chosen, resulting in default resources
getting surprisingly used. Now, we resolve the equivalent languages
at a per-resource levels (breaking ties for the identical code).

Also, previously if both "tl" and "fil" resources were present in
assets, getLocales() could return a list with duplicate locales.
This change removes Filipino duplicates in the return value of
AssetManager::getLocales().

Finally, there was a bug in the replacement of "tl" with "fil" that
considered any locale starting with the letter "tl" to be Tagalog.
This failed in case of various languages, including Klingon ("tlh")
and Tlingit ("tli"). It's now fixed.

Bug: 29073000
Change-Id: I0e8b9ae337ced2e640a2575897948c4c5ca307d3

4 files changed, 124 insertions, 97 deletions

diff --git a/include/androidfw/ResourceTypes.h b/include/androidfw/ResourceTypes.h
index 12a6b0f9a4ec..6349e86dae94 100644
--- a/include/androidfw/ResourceTypes.h
+++ b/include/androidfw/ResourceTypes.h
@@ -1227,7 +1227,10 @@ struct ResTable_config
     // |RESTABLE_MAX_LOCALE_LEN| (including a terminating '\0').
     //
     // Example: en-US, en-Latn-US, en-POSIX.
-    void getBcp47Locale(char* out) const;
+    //
+    // If canonicalize is set, Tagalog (tl) locales get converted
+    // to Filipino (fil).
+    void getBcp47Locale(char* out, bool canonicalize=false) const;
 
     // Append to str the resource-qualifer string representation of the
     // locale component of this Config. If the locale is only country
@@ -1849,7 +1852,8 @@ public:
     void getConfigurations(Vector<ResTable_config>* configs, bool ignoreMipmap=false,
             bool ignoreAndroidPackage=false, bool includeSystemConfigs=true) const;
 
-    void getLocales(Vector<String8>* locales, bool includeSystemLocales=true) const;
+    void getLocales(Vector<String8>* locales, bool includeSystemLocales=true,
+            bool mergeEquivalentLangs=false) const;
 
     // Generate an idmap.
     //
diff --git a/libs/androidfw/AssetManager.cpp b/libs/androidfw/AssetManager.cpp
index f50cff4387d2..641a7ffc2a78 100644
--- a/libs/androidfw/AssetManager.cpp
+++ b/libs/androidfw/AssetManager.cpp
@@ -355,14 +355,6 @@ void AssetManager::setLocale(const char* locale)
 }
 
 
-static const char kFilPrefix[] = "fil";
-static const char kTlPrefix[] = "tl";
-
-// The sizes of the prefixes, excluding the 0 suffix.
-// char.
-static const int kFilPrefixLen = sizeof(kFilPrefix) - 1;
-static const int kTlPrefixLen = sizeof(kTlPrefix) - 1;
-
 void AssetManager::setLocaleLocked(const char* locale)
 {
     if (mLocale != NULL) {
@@ -372,44 +364,6 @@ void AssetManager::setLocaleLocked(const char* locale)
         delete[] mLocale;
     }
 
-    // If we're attempting to set a locale that starts with "fil",
-    // we should convert it to "tl" for backwards compatibility since
-    // we've been using "tl" instead of "fil" prior to L.
-    //
-    // If the resource table already has entries for "fil", we use that
-    // instead of attempting a fallback.
-    if (strncmp(locale, kFilPrefix, kFilPrefixLen) == 0) {
-        Vector<String8> locales;
-        ResTable* res = mResources;
-        if (res != NULL) {
-            res->getLocales(&locales);
-        }
-        const size_t localesSize = locales.size();
-        bool hasFil = false;
-        for (size_t i = 0; i < localesSize; ++i) {
-            if (locales[i].find(kFilPrefix) == 0) {
-                hasFil = true;
-                break;
-            }
-        }
-
-
-        if (!hasFil) {
-            const size_t newLocaleLen = strlen(locale);
-            // This isn't a bug. We really do want mLocale to be 1 byte
-            // shorter than locale, because we're replacing "fil-" with
-            // "tl-".
-            mLocale = new char[newLocaleLen];
-            // Copy over "tl".
-            memcpy(mLocale, kTlPrefix, kTlPrefixLen);
-            // Copy the rest of |locale|, including the terminating '\0'.
-            memcpy(mLocale + kTlPrefixLen, locale + kFilPrefixLen,
-                   newLocaleLen - kFilPrefixLen + 1);
-            updateResourceParamsLocked();
-            return;
-        }
-    }
-
     mLocale = strdupNew(locale);
     updateResourceParamsLocked();
 }
@@ -816,17 +770,7 @@ void AssetManager::getLocales(Vector<String8>* locales, bool includeSystemLocale
 {
     ResTable* res = mResources;
     if (res != NULL) {
-        res->getLocales(locales, includeSystemLocales);
-    }
-
-    const size_t numLocales = locales->size();
-    for (size_t i = 0; i < numLocales; ++i) {
-        const String8& localeStr = locales->itemAt(i);
-        if (localeStr.find(kTlPrefix) == 0) {
-            String8 replaced("fil");
-            replaced += (localeStr.string() + kTlPrefixLen);
-            locales->editItemAt(i) = replaced;
-        }
+        res->getLocales(locales, includeSystemLocales, true /* mergeEquivalentLangs */);
     }
 }
 
diff --git a/libs/androidfw/ResourceTypes.cpp b/libs/androidfw/ResourceTypes.cpp
index b127912678ac..aee93dc78222 100644
--- a/libs/androidfw/ResourceTypes.cpp
+++ b/libs/androidfw/ResourceTypes.cpp
@@ -2024,7 +2024,6 @@ int ResTable_config::isLocaleMoreSpecificThan(const ResTable_config& o) const {
         ((o.localeVariant[0] != '\0') ? 2 : 0);
 
     return score - oScore;
-
 }
 
 bool ResTable_config::isMoreSpecificThan(const ResTable_config& o) const {
@@ -2170,6 +2169,23 @@ bool ResTable_config::isMoreSpecificThan(const ResTable_config& o) const {
     return false;
 }
 
+// Codes for specially handled languages and regions
+static const char kEnglish[2] = {'e', 'n'};  // packed version of "en"
+static const char kUnitedStates[2] = {'U', 'S'};  // packed version of "US"
+static const char kFilipino[2] = {'\xAD', '\x05'};  // packed version of "fil"
+static const char kTagalog[2] = {'t', 'l'};  // packed version of "tl"
+
+// Checks if two language or region codes are identical
+inline bool areIdentical(const char code1[2], const char code2[2]) {
+    return code1[0] == code2[0] && code1[1] == code2[1];
+}
+
+inline bool langsAreEquivalent(const char lang1[2], const char lang2[2]) {
+    return areIdentical(lang1, lang2) ||
+            (areIdentical(lang1, kTagalog) && areIdentical(lang2, kFilipino)) ||
+            (areIdentical(lang1, kFilipino) && areIdentical(lang2, kTagalog));
+}
+
 bool ResTable_config::isLocaleBetterThan(const ResTable_config& o,
         const ResTable_config* requested) const {
     if (requested->locale == 0) {
@@ -2179,7 +2195,7 @@ bool ResTable_config::isLocaleBetterThan(const ResTable_config& o,
     }
 
     if (locale == 0 && o.locale == 0) {
-        // The locales parts of both resources are empty, so no one is better
+        // The locale part of both resources is empty, so none is better
         // than the other.
         return false;
     }
@@ -2194,10 +2210,11 @@ bool ResTable_config::isLocaleBetterThan(const ResTable_config& o,
     // 2) If the request's script is known, the resource scripts are either
     //    unknown or match the request.
 
-    if (language[0] != o.language[0]) {
-        // The languages of the two resources are not the same. We can only
-        // assume that one of the two resources matched the request because one
-        // doesn't have a language and the other has a matching language.
+    if (!langsAreEquivalent(language, o.language)) {
+        // The languages of the two resources are not equivalent. If we are
+        // here, we can only assume that the two resources matched the request
+        // because one doesn't have a language and the other has a matching
+        // language.
         //
         // We consider the one that has the language specified a better match.
         //
@@ -2205,15 +2222,15 @@ bool ResTable_config::isLocaleBetterThan(const ResTable_config& o,
         // for US English and similar locales than locales that are a descendant
         // of Internatinal English (en-001), since no-language resources are
         // where the US English resource have traditionally lived for most apps.
-        if (requested->language[0] == 'e' && requested->language[1] == 'n') {
-            if (requested->country[0] == 'U' && requested->country[1] == 'S') {
+        if (areIdentical(requested->language, kEnglish)) {
+            if (areIdentical(requested->country, kUnitedStates)) {
                 // For US English itself, we consider a no-locale resource a
                 // better match if the other resource has a country other than
                 // US specified.
                 if (language[0] != '\0') {
-                    return country[0] == '\0' || (country[0] == 'U' && country[1] == 'S');
+                    return country[0] == '\0' || areIdentical(country, kUnitedStates);
                 } else {
-                    return !(o.country[0] == '\0' || (o.country[0] == 'U' && o.country[1] == 'S'));
+                    return !(o.country[0] == '\0' || areIdentical(o.country, kUnitedStates));
                 }
             } else if (localeDataIsCloseToUsEnglish(requested->country)) {
                 if (language[0] != '\0') {
@@ -2226,27 +2243,38 @@ bool ResTable_config::isLocaleBetterThan(const ResTable_config& o,
         return (language[0] != '\0');
     }
 
-    // If we are here, both the resources have the same non-empty language as
-    // the request.
+    // If we are here, both the resources have an equivalent non-empty language
+    // to the request.
     //
-    // Because the languages are the same, computeScript() always
-    // returns a non-empty script for languages it knows about, and we have passed
-    // the script checks in match(), the scripts are either all unknown or are
-    // all the same. So we can't gain anything by checking the scripts. We need
-    // to check the region and variant.
+    // Because the languages are equivalent, computeScript() always returns a
+    // non-empty script for languages it knows about, and we have passed the
+    // script checks in match(), the scripts are either all unknown or are all
+    // the same. So we can't gain anything by checking the scripts. We need to
+    // check the region and variant.
 
-    // See if any of the regions is better than the other
+    // See if any of the regions is better than the other.
     const int region_comparison = localeDataCompareRegions(
             country, o.country,
-            language, requested->localeScript, requested->country);
+            requested->language, requested->localeScript, requested->country);
     if (region_comparison != 0) {
         return (region_comparison > 0);
     }
 
     // The regions are the same. Try the variant.
-    if (requested->localeVariant[0] != '\0'
-            && strncmp(localeVariant, requested->localeVariant, sizeof(localeVariant)) == 0) {
-        return (strncmp(o.localeVariant, requested->localeVariant, sizeof(localeVariant)) != 0);
+    const bool localeMatches = strncmp(
+            localeVariant, requested->localeVariant, sizeof(localeVariant)) == 0;
+    const bool otherMatches = strncmp(
+            o.localeVariant, requested->localeVariant, sizeof(localeVariant)) == 0;
+    if (localeMatches != otherMatches) {
+        return localeMatches;
+    }
+
+    // Finally, the languages, although equivalent, may still be different
+    // (like for Tagalog and Filipino). Identical is better than just
+    // equivalent.
+    if (areIdentical(language, requested->language)
+            && !areIdentical(o.language, requested->language)) {
+        return true;
     }
 
     return false;
@@ -2522,7 +2550,7 @@ bool ResTable_config::match(const ResTable_config& settings) const {
         //
         // If two configs differ only in their country and variant,
         // they can be weeded out in the isMoreSpecificThan test.
-        if (language[0] != settings.language[0] || language[1] != settings.language[1]) {
+        if (!langsAreEquivalent(language, settings.language)) {
             return false;
         }
 
@@ -2550,9 +2578,7 @@ bool ResTable_config::match(const ResTable_config& settings) const {
         }
 
         if (countriesMustMatch) {
-            if (country[0] != '\0'
-                && (country[0] != settings.country[0]
-                    || country[1] != settings.country[1])) {
+            if (country[0] != '\0' && !areIdentical(country, settings.country)) {
                 return false;
             }
         } else {
@@ -2734,37 +2760,43 @@ void ResTable_config::appendDirLocale(String8& out) const {
     }
 }
 
-void ResTable_config::getBcp47Locale(char str[RESTABLE_MAX_LOCALE_LEN]) const {
+void ResTable_config::getBcp47Locale(char str[RESTABLE_MAX_LOCALE_LEN], bool canonicalize) const {
     memset(str, 0, RESTABLE_MAX_LOCALE_LEN);
 
     // This represents the "any" locale value, which has traditionally been
     // represented by the empty string.
-    if (!language[0] && !country[0]) {
+    if (language[0] == '\0' && country[0] == '\0') {
         return;
     }
 
     size_t charsWritten = 0;
-    if (language[0]) {
-        charsWritten += unpackLanguage(str);
+    if (language[0] != '\0') {
+        if (canonicalize && areIdentical(language, kTagalog)) {
+            // Replace Tagalog with Filipino if we are canonicalizing
+            str[0] = 'f'; str[1] = 'i'; str[2] = 'l'; str[3] = '\0';  // 3-letter code for Filipino
+            charsWritten += 3;
+        } else {
+            charsWritten += unpackLanguage(str);
+        }
     }
 
-    if (localeScript[0] && !localeScriptWasComputed) {
-        if (charsWritten) {
+    if (localeScript[0] != '\0' && !localeScriptWasComputed) {
+        if (charsWritten > 0) {
             str[charsWritten++] = '-';
         }
         memcpy(str + charsWritten, localeScript, sizeof(localeScript));
         charsWritten += sizeof(localeScript);
     }
 
-    if (country[0]) {
-        if (charsWritten) {
+    if (country[0] != '\0') {
+        if (charsWritten > 0) {
             str[charsWritten++] = '-';
         }
         charsWritten += unpackRegion(str + charsWritten);
     }
 
-    if (localeVariant[0]) {
-        if (charsWritten) {
+    if (localeVariant[0] != '\0') {
+        if (charsWritten > 0) {
             str[charsWritten++] = '-';
         }
         memcpy(str + charsWritten, localeVariant, sizeof(localeVariant));
@@ -5879,12 +5911,13 @@ static bool compareString8AndCString(const String8& str, const char* cStr) {
     return strcmp(str.string(), cStr) < 0;
 }
 
-void ResTable::getLocales(Vector<String8>* locales, bool includeSystemLocales) const {
+void ResTable::getLocales(Vector<String8>* locales, bool includeSystemLocales,
+                          bool mergeEquivalentLangs) const {
     char locale[RESTABLE_MAX_LOCALE_LEN];
 
     forEachConfiguration(false, false, includeSystemLocales, [&](const ResTable_config& cfg) {
         if (cfg.locale != 0) {
-            cfg.getBcp47Locale(locale);
+            cfg.getBcp47Locale(locale, mergeEquivalentLangs /* canonicalize if merging */);
 
             const auto beginIter = locales->begin();
             const auto endIter = locales->end();
diff --git a/libs/androidfw/tests/ConfigLocale_test.cpp b/libs/androidfw/tests/ConfigLocale_test.cpp
index 2bf9b12b6ce5..6e998159e554 100644
--- a/libs/androidfw/tests/ConfigLocale_test.cpp
+++ b/libs/androidfw/tests/ConfigLocale_test.cpp
@@ -279,6 +279,23 @@ TEST(ConfigLocaleTest, getBcp47Locale_script) {
     EXPECT_EQ(0, strcmp("en", out));
 }
 
+TEST(ConfigLocaleTest, getBcp47Locale_canonicalize) {
+    ResTable_config config;
+    char out[RESTABLE_MAX_LOCALE_LEN];
+
+    fillIn("tl", NULL, NULL, NULL, &config);
+    config.getBcp47Locale(out);
+    EXPECT_EQ(0, strcmp("tl", out));
+    config.getBcp47Locale(out, true /* canonicalize */);
+    EXPECT_EQ(0, strcmp("fil", out));
+
+    fillIn("tl", "PH", NULL, NULL, &config);
+    config.getBcp47Locale(out);
+    EXPECT_EQ(0, strcmp("tl-PH", out));
+    config.getBcp47Locale(out, true /* canonicalize */);
+    EXPECT_EQ(0, strcmp("fil-PH", out));
+}
+
 TEST(ConfigLocaleTest, match) {
     ResTable_config supported, requested;
 
@@ -292,6 +309,11 @@ TEST(ConfigLocaleTest, match) {
     // Different languages don't match.
     EXPECT_FALSE(supported.match(requested));
 
+    fillIn("tl", "PH", NULL, NULL, &supported);
+    fillIn("fil", "PH", NULL, NULL, &requested);
+    // Equivalent languages match.
+    EXPECT_TRUE(supported.match(requested));
+
     fillIn("qaa", "FR", NULL, NULL, &supported);
     fillIn("qaa", "CA", NULL, NULL, &requested);
     // If we can't infer the scripts, different regions don't match.
@@ -406,6 +428,12 @@ TEST(ConfigLocaleTest, isLocaleBetterThan_basics) {
     EXPECT_FALSE(config2.isLocaleBetterThan(config1, &request));
 
     fillIn("de", "DE", NULL, NULL, &request);
+    fillIn("de", "DE", NULL, NULL, &config1);
+    fillIn("de", "DE", NULL, "1901", &config2);
+    EXPECT_TRUE(config1.isLocaleBetterThan(config2, &request));
+    EXPECT_FALSE(config2.isLocaleBetterThan(config1, &request));
+
+    fillIn("de", "DE", NULL, NULL, &request);
     fillIn("de", "DE", NULL, "1901", &config1);
     fillIn("de", "DE", NULL, "1996", &config2);
     EXPECT_FALSE(config1.isLocaleBetterThan(config2, &request));
@@ -422,6 +450,24 @@ TEST(ConfigLocaleTest, isLocaleBetterThan_basics) {
     fillIn("de", "DE", NULL, NULL, &config2);
     EXPECT_FALSE(config1.isLocaleBetterThan(config2, &request));
     EXPECT_FALSE(config2.isLocaleBetterThan(config1, &request));
+
+    fillIn("fil", "PH", NULL, NULL, &request);
+    fillIn("tl", "PH", NULL, NULL, &config1);
+    fillIn("fil", "US", NULL, NULL, &config2);
+    EXPECT_TRUE(config1.isLocaleBetterThan(config2, &request));
+    EXPECT_FALSE(config2.isLocaleBetterThan(config1, &request));
+
+    fillIn("fil", "PH", NULL, "fonipa", &request);
+    fillIn("tl", "PH", NULL, "fonipa", &config1);
+    fillIn("fil", "PH", NULL, NULL, &config2);
+    EXPECT_TRUE(config1.isLocaleBetterThan(config2, &request));
+    EXPECT_FALSE(config2.isLocaleBetterThan(config1, &request));
+
+    fillIn("fil", "PH", NULL, NULL, &request);
+    fillIn("fil", "PH", NULL, NULL, &config1);
+    fillIn("tl", "PH", NULL, NULL, &config2);
+    EXPECT_TRUE(config1.isLocaleBetterThan(config2, &request));
+    EXPECT_FALSE(config2.isLocaleBetterThan(config1, &request));
 }
 
 TEST(ConfigLocaleTest, isLocaleBetterThan_regionComparison) {