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

mozharness/abb/transform_locales.py

 # This file is part of Adblock Plus
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # 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,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>)$")
+_PROPERTY_FORMAT_RE = re.compile("^(([^=]*)=)(.*)$")
+_ENTITY_FORMAT_RE = re.compile("^(\s*<!ENTITY\s*([^\"\s]*)\s*\")(.*)(\">)$")
+_STRING_FORMAT_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"
@@ skipping 12 matching lines @@
               "yahoo",
               "google",
               "wikipedia",
               "amazon"
               ]
 }
 
 _FIREFOX_REPLACE_STR = "Firefox"
 _ABB_REPLACEMENT_STR = "Adblock Browser"
 
+# Some string values that contain Firefox such as 'Firefox Sync' shouldn't be
+# replaced, so we keep a list of ids that are exceptions
 _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",
@@ skipping 21 matching lines @@
 
 
 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)
+def _replace_in_value(format_re, str, old, new, exceptions=[]):
+    match = format_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)
+        # The prefix contains all characters that precedes the value, including
+        # the id/key
+        str_value_prefix = match.group(1)
+        str_id = match.group(2)
+        str_value = match.group(3)
+        if str_id not in exceptions and old in str_value:
+            new_str = str_value_prefix + str_value.replace(old, new)
             if match.lastindex == 4:
-                new_str = new_str + match.group(4)
+                # The suffix contains all characters that succeeds the value
+                str_value_suffix = match.group(4)
+                new_str = new_str + str_value_suffix
             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)
 
@@ skipping 77 matching lines @@
     # Append default search engine name
     props.append("browser.search.defaultenginename=%s" % engine_order[0])
 
     # 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'"""
+    # Replaces ocurrences of 'Firefox' by 'Adblock Browser' 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)
+            replacement = _replace_in_value(_PROPERTY_FORMAT_RE, line,
+                                            _FIREFOX_REPLACE_STR,
+                                            _ABB_REPLACEMENT_STR)
             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)
+            replacement = _replace_in_value(_ENTITY_FORMAT_RE, line,
+                                            _FIREFOX_REPLACE_STR,
+                                            _ABB_REPLACEMENT_STR,
+                                            _ENTITY_EXCEPTIONS)
             if replacement:
                 line = replacement
                 replacement_count += 1
             else:
-                replacement = _replace_str_re(line, _FIREFOX_REPLACE_STR,
-                                              _ABB_REPLACEMENT_STR, _STRING_RE)
+                replacement = _replace_in_value(_STRING_FORMAT_RE, line,
+                                                _FIREFOX_REPLACE_STR,
+                                                _ABB_REPLACEMENT_STR)
                 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))
 
@@ skipping 28 matching lines @@
         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 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)