Issue 3199 - "Firefox" string is displayed on several error messages

mozharness/abb/transform_locales.py

Index: mozharness/abb/transform_locales.py
===================================================================
--- a/mozharness/abb/transform_locales.py
+++ b/mozharness/abb/transform_locales.py
@@ -11,25 +11,37 @@
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
 import re
 
-_LOCALE_RE = re.compile("^[a-z]{2}(-[A-Z]{2})?$")
+_LOCALE_RE = re.compile("^([a-z]{2,3}(?:-[A-Z]{2})?)$")
+_VALUES_LOCALE_RE = re.compile("^values-([a-z]{2,3}(?:-r[A-Z]{2})?)$")

[Felix Dahlke, 2016/12/13 13:06:05]

What's the "r" for here?

[Felix Dahlke, 2016/12/13 13:11:15]

Whoops, I was planning to remove this comment once I figured this out :P I did,
that seems to be just the format of those values- dirs. Guess we should just
roll with it.

+
 _SEARCH_PROPS_RE = re.compile("^browser\.search\."
                               "(defaultenginename|order\.).*$")
 _SHORTNAME_RE = re.compile("^<ShortName>(.*)</ShortName>$")
 
+_PROPERTY_RE = re.compile("^(([^=]*)=)(.*)$")
+_ENTITY_RE = re.compile("^(\s*<!ENTITY\s*([^\"\s]*)\s*\")(.*)(\">)$")
+_STRING_RE = re.compile("^(\s*<string name=\"([^\"]*)\">)(.*)(</string>)$")
+
+_CHROME_PATH = os.path.join("dist", "bin", "chrome")
+_RES_PATH = os.path.join("mobile", "android", "base", "res")
+
 _SEARCHPLUGINS_PATH = os.path.join("browser", "searchplugins")
 _LIST_TXT_PATH = os.path.join(_SEARCHPLUGINS_PATH, "list.txt")
 _REGION_PROPS_PATH = os.path.join("browser", "region.properties")
 
+_APPSTRINGS_PROPS_PATH = os.path.join("browser", "appstrings.properties")
+_STRINGS_XML_PATH = "strings.xml"
+
 _DEFAULT_LOCALE = "en-US"
 
 _SEARCH_ENGINE_ORDER = {
     "en-US": ["duckduckgo",
               "yahoo",
               "google",
               "wikipedia",
               "amazon"
@@ -38,96 +50,146 @@ import re
               "duckduckgo",
               "yahoo",
               "google",
               "wikipedia",
               "amazon"
               ]
 }
 
+_FIREFOX_REPLACE_STR = "Firefox"
+_ABB_REPLACEMENT_STR = "Adblock Browser"
+
+_ENTITY_EXCEPTIONS = [

[Felix Dahlke, 2016/12/13 13:06:05]

Why do we exclude these? Would be helpful to have in a comment.

[diegocarloslima, 2016/12/21 13:21:48]

These are excluded because they relate to some features that must have Firefox
on it's name like Firefox Sync / Firefox Account. It wouldn't make sense to have
them renamed to Adblock Browser Sync/Account. But I agree, I should add a
comment to make it more clear

+    "overlay_no_synced_devices",
+    "home_remote_tabs_need_to_sign_in",
+    "home_remote_tabs_need_to_finish_migrating",
+    "home_remote_tabs_need_to_verify",
+    "syncBrand.fullName.label",
+    "sync.subtitle.connectlocation2.label",
+    "sync.subtitle.failmultiple.label",
+    "fxaccount_full_label",
+    "fxaccount_create_account_header2",
+    "fxaccount_create_account_policy_text2",
+    "fxaccount_status_header2",
+    "fxaccount_status_needs_finish_migrating",
+    "fxaccount_remove_account_dialog_title",
+    "fxaccount_remove_account_toast",
+    "fxaccount_account_type_label",
+]
+
+
+def _check_path_exists(path, logger):
+    if not os.path.exists(path):
+        logger.fatal("'%s' does not exist" % path)
+
+
+def _get_locales_from_path(path, locale_re):
+    locales = []
+    for dir_name in next(os.walk(path))[1]:
+        match = locale_re.match(dir_name)
+        if match:
+            locales.append(match.group(1))
+    locales.sort
+    return locales
+
 
 def _get_shortname_from_id(needle, engine_ids, engine_names):
     """Fuzzy finds needle in engine_ids and returns ShortName"""
     for engine in engine_ids:
         if engine.startswith(needle):
             return engine_names[engine]
     return None
 
 
+def _replace_str_re(str, old, new, replacement_re, exceptions=[]):

[Felix Dahlke, 2016/12/13 13:06:06]

Took me a little while to wrap my head around what this really does. I have some
ideas for making it easier to understand:

1. Find a name that describes what it does more accurately. My best bet is
`_replace_in_value`, but I'm not fully convinced that's much better.

2. Rename the `replacement_re` parameter to something along the lines of
`format_re`. I would also move this to be the first parameter, since it strongly
affects what the function does. Likewise, I would rename `_PROPERTY_RE` and so
on above to `_PROPERTY_FORMAT_RE` or something to that effect.

3. Assign temp variables for the various matched groups below, e.g. `key` for
`match.group(2)`. But I guess this is least important, not that hard to get
behind.

[diegocarloslima, 2016/12/21 13:21:48]

Acknowledged.

+    match = replacement_re.match(str)
+    if match and match.lastindex > 2:
+        if match.group(2) not in exceptions and old in match.group(3):

[Felix Dahlke, 2016/12/13 13:06:05]

Shouldn't `old` be `in match.group(2)` rather than `in match.group(3)`?

[diegocarloslima, 2016/12/21 13:21:48]

Actually match.group(2) matches the id/key of the property/entity/string while
match.group(3) matches its value. I will extract them as variables as you
suggested to make it more clear

+            new_str = match.group(1) + match.group(3).replace(old, new)
+            if match.lastindex == 4:
+                new_str = new_str + match.group(4)
+            return new_str
+    return None
+
+
 def _write_lines(filename, lines):
     """Writes lines into file appending \\n"""
     with open(filename, "w") as fd:
         for l in lines:
             fd.write("%s\n" % l)
 
 
 def _transform_locale(locale, path, logger):
     logger.info("Processing locale '%s'..." % locale)
 
     # Check for list.txt existence
     list_file_path = os.path.join(path, _LIST_TXT_PATH)
-    if not os.path.exists(list_file_path):
-        logger.fatal("Missing 'list.txt' for locale '%s'" % locale)
+    _check_path_exists(list_file_path, logger)
 
     # Check for region.properties existence
     region_file_path = os.path.join(path, _REGION_PROPS_PATH)
-    if not os.path.exists(region_file_path):
-        logger.fatal("Missing 'region.properties' for locale '%s'" % locale)
+    _check_path_exists(region_file_path, logger)
+
+    # Check for appstrings.properties existence
+    appstrings_file_path = os.path.join(path, _APPSTRINGS_PROPS_PATH)
+    _check_path_exists(appstrings_file_path, logger)
 
     # Get whitelist and build regex
     whitelist = _SEARCH_ENGINE_ORDER.get(locale,
                                          _SEARCH_ENGINE_ORDER[_DEFAULT_LOCALE])
     white_re = re.compile("^(%s).*$" % "|".join(whitelist))
 
     # Read engine IDs from list.txt, discard engines not on whitelist
     engine_ids = []
+    removed_engine_ids = []
     with open(list_file_path, "r") as fd:
         for line in fd:
             line = line.strip()
             if len(line) > 0:
                 if white_re.match(line):
                     engine_ids.append(line)
                 else:
-                    logger.info("Removing '%s'" % line)
+                    removed_engine_ids.append(line)
 
     # Make sure we still have search engines left
     if len(engine_ids) == 0:
         logger.fatal("No search engines left over for '%s'" % locale)
 
     # 'Parse' XML to get matching 'ShortName' for all engine IDs
     engine_names = {}
     for eid in engine_ids:
         xml_file_path = os.path.join(path, _SEARCHPLUGINS_PATH, "%s.xml" % eid)
-        if os.path.exists(xml_file_path):
-            short_name = None
-            with open(xml_file_path, "r") as fd:
-                for line in fd:
-                    line = line.strip()
-                    match = _SHORTNAME_RE.match(line)
-                    if match:
-                        short_name = match.group(1).strip()
+        _check_path_exists(xml_file_path, logger)
+        short_name = None
+        with open(xml_file_path, "r") as fd:
+            for line in fd:
+                line = line.strip()
+                match = _SHORTNAME_RE.match(line)
+                if match:
+                    short_name = match.group(1).strip()
 
-            if not short_name:
-                logger.fatal("No ShortName defined for '%s' in '%s" %
-                             (eid, locale))
-            engine_names[eid] = short_name
-        else:
-            logger.fatal("XML definiton for '%s' in '%s' missing" %
+        if not short_name:
+            logger.fatal("No ShortName defined for '%s' in '%s" %
                          (eid, locale))
+        engine_names[eid] = short_name
 
-    logger.info("Remaining engine IDs: %s" % ", ".join(engine_ids))
+    logger.info("Removed search engine IDs: %s" %
+                ", ".join(removed_engine_ids))
+    logger.info("Remaining search engine IDs: %s" % ", ".join(engine_ids))
 
     # Create search engine order with real engine names
     engine_order = []
     for eid in whitelist:
         sn = _get_shortname_from_id(eid, engine_ids, engine_names)
         if sn:
             engine_order.append(sn)
 
-    logger.info("Resulting ordered list: %s" % (", ".join(engine_order)))
+    logger.info("Resulting search engine ordered list: %s" %
+                (", ".join(engine_order)))
 
     # Read region.properties and remove browser.search.* lines
     props = []
     with open(region_file_path, "r") as fd:
         for line in fd:
             line = line.rstrip("\r\n")
             if not _SEARCH_PROPS_RE.match(line.strip()):
                 props.append(line)
@@ -137,41 +199,100 @@ def _transform_locale(locale, path, logg
 
     # Append search engine order
     for i in range(0, min(5, len(engine_order))):
         props.append("browser.search.order.%d=%s" % (i + 1, engine_order[i]))
 
     # Write back region.properties
     _write_lines(region_file_path, props)
 
+    """Replaces ocurrences of 'Firefox' by 'Adblock Browser'

[Felix Dahlke, 2016/12/13 13:06:06]

Shouldn't this be a normal comment rather than a doc string?

[diegocarloslima, 2016/12/21 13:21:48]

Actually, I had to make it a multiline comment to be in compliance with PEP8
line size constraint. But I just saw that they recommend to use consecutive
single line comments instead. I'll fix that

+        in 'appstrings.properties'"""
+    lines = []
+    replacement_count = 0
+
+    with open(appstrings_file_path, "r") as fd:
+        for line in fd:
+            line = line.rstrip("\r\n")
+            replacement = _replace_str_re(line, _FIREFOX_REPLACE_STR,
+                                          _ABB_REPLACEMENT_STR, _PROPERTY_RE)
+            if replacement:
+                line = replacement
+                replacement_count += 1
+            lines.append(line)
+
+    # Apply changes to appstrings.properties
+    _write_lines(appstrings_file_path, lines)
+    logger.info("Replaced %d ocurrences of %s in 'appstrings.properties'" %
+                (replacement_count, _FIREFOX_REPLACE_STR))
+
+
+def _transform_values_locale(locale, path, logger):
+    logger.info("Processing values-%s..." % locale)
+
+    # Check for strings.xml existence
+    strings_file_path = os.path.join(path, _STRINGS_XML_PATH)
+    _check_path_exists(strings_file_path, logger)
+
+    # Replaces ocurrences of 'Firefox' by 'Adblock Browser' in 'strings.xml'
+    lines = []
+    replacement_count = 0
+
+    with open(strings_file_path, "r") as fd:
+        for line in fd:
+            line = line.rstrip("\r\n")
+            replacement = _replace_str_re(line, _FIREFOX_REPLACE_STR,
+                                          _ABB_REPLACEMENT_STR, _ENTITY_RE,
+                                          _ENTITY_EXCEPTIONS)
+            if replacement:
+                line = replacement
+                replacement_count += 1
+            else:
+                replacement = _replace_str_re(line, _FIREFOX_REPLACE_STR,
+                                              _ABB_REPLACEMENT_STR, _STRING_RE)
+                if replacement:
+                    line = replacement
+                    replacement_count += 1
+            lines.append(line)
+
+    # Apply changes to strings.xml
+    _write_lines(strings_file_path, lines)
+    logger.info("Replaced %d ocurrences of %s in 'strings.xml'" %
+                (replacement_count, _FIREFOX_REPLACE_STR))
+
 
 class MinimalLogger:
     def info(self, s):
         print "INFO: %s" % s
 
     def error(self, s):
         print "ERROR: %s" % s
 
     def fatal(self, s):
         print "FATAL: %s" % s
         exit(1)
 
 
 def transform_locales(obj_dir, logger=MinimalLogger()):
-    chrome_path = os.path.join(obj_dir, "dist", "bin", "chrome")
-    if not os.path.exists(chrome_path):
-        logger.fatal("'%s' does not exist" % obj_dir)
+    chrome_path = os.path.join(obj_dir, _CHROME_PATH)
+    _check_path_exists(chrome_path, logger)
 
-    locales = []
-    for p in next(os.walk(chrome_path))[1]:
-        if _LOCALE_RE.match(p):
-            locales.append(p)
-    locales.sort()
+    res_path = os.path.join(obj_dir, _RES_PATH)
+    _check_path_exists(res_path, logger)
 
-    logger.info("Found %d locales" % len(locales))
+    locales = _get_locales_from_path(chrome_path, _LOCALE_RE)
+    values_locales = _get_locales_from_path(res_path, _VALUES_LOCALE_RE)
+
+    locales_found_msg = "Found %d locales in %s"
+    logger.info(locales_found_msg % (len(locales), chrome_path))
+    logger.info(locales_found_msg % (len(values_locales), res_path))
 
     for locale in locales:
         locale_path = os.path.join(chrome_path, locale, "locale", locale)
         if os.path.exists(locale_path):
             _transform_locale(locale, locale_path, logger)
         else:
-            logger.error("Missing 'locale' folder for '%s'" % locale)
+            logger.error("Missing folder for locale '%s' in path: %s" %
+                         (locale, locale_path))
 
+    for locale in values_locales:
+        locale_path = os.path.join(res_path, "values-" + locale)
+        _transform_values_locale(locale, locale_path, logger)