Issue 6077 - Create script to modify the list of search engines dynamically

mozharness/abb/transform_locales.py

Index: mozharness/abb/transform_locales.py
===================================================================
--- a/mozharness/abb/transform_locales.py
+++ b/mozharness/abb/transform_locales.py
@@ -8,39 +8,86 @@
 # 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 json
 import os
 import re
 import sys
 
 _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})?)$")
 
+_SEARCH_PROPS_RE = re.compile("^browser\.search\."
+                              "(defaultenginename|order\.).*$")
+_SHORTNAME_RE = re.compile("^<ShortName>(.*)</ShortName>$")
+
 _PROPERTY_FORMAT_RE = re.compile("^(([^=]*)=)(.*)$")
 _ENTITY_FORMAT_RE = re.compile("^(\s*<!ENTITY\s*([^\"\s]*)\s*\")(.*)(\">)$")
 _STRING_FORMAT_RE = re.compile(
     "^(\s*<string name=\"([^\"]*)\">)(.*)(</string>)$")
 
 _MOZBUILD_PATH = os.path.join("python", "mozbuild")
 
 _CHROME_PATH = os.path.join("dist", "bin", "chrome")
 _RES_PATH = os.path.join("mobile", "android", "base", "res")
+_I10N_PATH = os.path.join("abb-build", "l10n")

[diegocarloslima, 2017/12/13 10:44:42]

This var should be named L10N_PATH :)

+_LISTJSON_PATH = os.path.join("mobile", "locales", "search")
+_SEARCHPLUGINS_PATH = os.path.join("mobile", "locales", "searchplugins")

[diegocarloslima, 2017/12/13 10:44:41]

Maybe the ("mobile","locales") part could be extracted to another variable to
avoid writing twice the same string constants, e.g. _LOCALES_PATH =
os.path.join("mobile", "locales").

Also, its not really clear to me why do we need the _LISTJSON_PATH + _LIST_JSON.
Couldn't we have just one var (e.g. _LIST_JSON_PATH) that would also include the
`list.json` filename?

 
 _BROWSER_DIR = "browser"
+_REGION_PROPS_PATH = os.path.join(_BROWSER_DIR, "region.properties")
 
 _APPSTRINGS_PROPS_PATH = os.path.join(_BROWSER_DIR, "appstrings.properties")
 _STRINGS_XML_PATH = "strings.xml"
+_LIST_JSON = "list.json"
 
 _DEFAULT_LOCALE = "en-US"
+_KEY_VDF = "visibleDefaultEngines"
+_KEY_LOC = "locales"
+_KEY_DEF = "default"

[diegocarloslima, 2017/12/13 10:44:42]

I know that having short variable names helps to be in compliance with pep8, but
if I look into those keys in the code below, they are not clear to me, so I have
to come here to see what they stand for, which is kinda of undesired

+
+# Add Ecosia as secondary search engine.
+# See https://issues.adblockplus.org/ticket/5518
+_ECOSIA_ID = "ecosia"
+
+_SEARCH_ENGINE_ORDER_DEFAULT = [
+    "duckduckgo",
+    "yahoo",
+    "google",
+    "wikipedia",
+    "amazondotcom"]
+
+_SEARCH_ENGINE_ORDER_ECOSIA = [
+    "duckduckgo",
+    "yahoo",
+    "google",
+    "ecosia",
+    "wikipedia",
+    "amazon"]
+
+_SEARCH_ENGINE_ORDER = {
+    "de": _SEARCH_ENGINE_ORDER_ECOSIA,
+    "en-GB": _SEARCH_ENGINE_ORDER_ECOSIA,
+    "en-US": _SEARCH_ENGINE_ORDER_ECOSIA,
+    "fr": _SEARCH_ENGINE_ORDER_ECOSIA,
+    "nl": _SEARCH_ENGINE_ORDER_ECOSIA,
+    "zh-CN": ["baidu",
+              "duckduckgo",
+              "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 = [
     "overlay_no_synced_devices",
@@ -104,23 +151,108 @@ def _replace_in_value(format_re, str, ol
 
 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):
+def _transform_locale(locale, build_dir, locale_path, logger):
     logger.info("Processing locale '%s'..." % locale)
+    search_list_path = os.path.join(build_dir, _LISTJSON_PATH)
+    _check_path_exists(search_list_path, logger)

[diegocarloslima, 2017/12/13 10:44:41]

I think it makes more sense to check for the existence of the `list.json` file
(_LISTJSON_PATH + _LIST_JSON) instead of the `search` folder. Also, since this
check isn't locale dependent, it should be only made once (before actually
calling _transform_locale)

+
+    # Check for region.properties existence
+    region_file_path = os.path.join(locale_path, _REGION_PROPS_PATH)
+    _check_path_exists(region_file_path, logger)
 
     # Check for appstrings.properties existence
-    appstrings_file_path = os.path.join(path, _APPSTRINGS_PROPS_PATH)
+    appstrings_file_path = os.path.join(locale_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)
+    white_re = re.compile("^(%s).*$" % "|".join(whitelist))
+
+    all_engine_ids = []
+    engine_ids = []
+    removed_engine_ids = []
+
+    jsonFile = open(os.path.join(search_list_path, _LIST_JSON), 'r')
+    data = json.load(jsonFile)
+    jsonFile.close()

[diegocarloslima, 2017/12/13 10:44:42]

We could use the `with open` syntax here, to avoid the close statement

+
+    for item in data[_KEY_LOC][locale][_KEY_DEF][_KEY_VDF]:
+        all_engine_ids.append(item)
+        if white_re.match(item):
+            engine_ids.append(item)
+        else:
+            removed_engine_ids.append(item)
+
+    # 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 = {}
+    search_plugins_path = os.path.join(build_dir, _SEARCHPLUGINS_PATH)
+    for eid in engine_ids[:]:
+        xml_file_path = os.path.join(search_plugins_path, "%s.xml" % eid)
+        if not os.path.exists(xml_file_path):
+            logger.info("Missing xml file for plugin %s. Searched in path %s" %
+                        (eid, xml_file_path))
+            engine_ids.remove(eid)
+            continue
+        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
+
+    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 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)
+
+    # 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))):

[diegocarloslima, 2017/12/13 10:44:42]

The condition here should be `for i in range(0, len(engine_order)):`.  We used
to have this limitation of 5 search engines, but we don't want this limitation
anymore, so we would append whatever we have.

Also, since we're not adding Ecosia into this piece of logic (it will be never
present in engine_order), it seems that Ecosia will be added for the correct
locales, but it won't be ordered properly, and it will be placed in last
position

+        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' in
     # 'appstrings.properties'
     lines = []
     replacement_count = 0
 
     with open(appstrings_file_path, "r") as fd:
         for line in fd:
             line = line.rstrip("\r\n")
@@ -215,17 +347,78 @@ def transform_locales(build_dir, obj_dir
 
     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)
+            _transform_locale(locale, build_dir, locale_path, logger)
             _generate_browser_search(locale, locale_path, res_path, build_dir)
         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)
+
+
+def transform_search_engines_list(abs_mozilla_dir, obj_dir,

[diegocarloslima, 2017/12/13 10:44:42]

I see that you're using here the `abs_mozilla_dir` nomenclature. On other
funcions we name it `build_dir`, which now I don't think it was a good name
choice :( . Anyways, I think we should  name it consistently, so it can be more
easily perceived that it means the same directory. I would suggest naming it
just mozilla_dir, or abb_dir or project_dir (and also replacing `build_dir`
occurrences). What do you think?

+                                  logger=MinimalLogger()):
+    # open the Mozilla list of search engines, put it into a buffer and
+    # close the JSON file after reading
+    search_list_path = os.path.join(abs_mozilla_dir, _LISTJSON_PATH)
+    jsonFile = open(os.path.join(search_list_path, _LIST_JSON), 'r')
+    data = json.load(jsonFile)
+    jsonFile.close()

[diegocarloslima, 2017/12/13 10:44:41]

We could use the `with open` syntax here, to avoid the close statement. Also,
this block is pretty much the same that we have in _transform_locale. It might
be better to have it as an utility function to avoid duplication of code

+
+    chrome_path = os.path.join(abs_mozilla_dir, _I10N_PATH)

[diegocarloslima, 2017/12/13 10:44:42]

Didn't really get why are you using a different chrome path here (in comparison
to chrome_path = os.path.join(obj_dir, _CHROME_PATH) )

+    _check_path_exists(chrome_path, logger)
+    # get all locales we want to transform
+    locales = _get_locales_from_path(chrome_path, _LOCALE_RE)
+
+    # set default search engine order
+    data[_KEY_DEF][_KEY_VDF] = _SEARCH_ENGINE_ORDER_DEFAULT
+    for loc in locales:

[diegocarloslima, 2017/12/13 10:44:42]

If we moved all the logic that is currently being added also in
_transform_locale to here, we could avoid lots of code duplications (e.g.
creation of whitelist , whitelist_re, all_engine_ids, engine_ids,
removed_engine_ids, and so on)

[diegocarloslima, 2017/12/13 10:55:29]

Also, I think that this should be an internal function, in order to avoid
creating a further step in multi_locale_build. So the execution would be
something like this:

transform_locales() would have the main logic for reading the json file and it
would pass to _transform_search_engines() which would reorder the search engines
for each locale by rewriting the region.properties and would modify the json
data structure that would be later been written back in list.json, inside
transform_locales(), once _transform_search_engines() has ended

+        whitelist = _SEARCH_ENGINE_ORDER.get(loc,
+                                             _SEARCH_ENGINE_ORDER_DEFAULT)
+        white_re = re.compile("^(%s).*$" % "|".join(whitelist))
+
+        # Read engine IDs from list.json, discard engines not on whitelist
+        all_engine_ids = []
+        engine_ids = []
+        removed_engine_ids = []
+
+        # Mozilla default list does not contain locale bn-BD, so we create it
+        # and use the values from locale bn-IN
+        if loc == "bn-BD":
+            data[_KEY_LOC].update({loc: {_KEY_DEF: {_KEY_VDF: data[_KEY_LOC]
+                                   ["bn-IN"][_KEY_DEF][_KEY_VDF]}}})
+        # Mozilla default list does not contain locale wo, so we use the
+        # default order. In case they will not support any other locales in
+        # the future, we want the build to fail, to decide which order to use
+        elif loc == "wo":
+            data[_KEY_LOC].update({loc: {_KEY_DEF:
+                                   {_KEY_VDF: _SEARCH_ENGINE_ORDER_DEFAULT}}})
+
+        for item in data[_KEY_LOC][loc][_KEY_DEF][_KEY_VDF]:
+            all_engine_ids.append(item)
+            if len(item) > 0:
+                if white_re.match(item):
+                    engine_ids.append(item)
+                else:
+                    removed_engine_ids.append(item)
+
+        if _ECOSIA_ID in whitelist and _ECOSIA_ID not in all_engine_ids:
+            all_engine_ids.append(_ECOSIA_ID)
+            engine_ids.append(_ECOSIA_ID)
+
+        # Make sure we still have search engines left
+        if len(engine_ids) == 0:
+            logger.fatal("No search engines left over for '%s'" % loc)
+
+        data[_KEY_LOC][loc][_KEY_DEF][_KEY_VDF] = all_engine_ids
+
+    # Save our changes to list.json
+    with open(os.path.join(search_list_path, _LIST_JSON), 'w') as outfile:
+        json.dump(data, outfile, indent=4, sort_keys=True)