Issue 3047 - Change default search engines

mozharness/abb/abb_transform_locales.py

+# This file is part of Adblock Plus

[Felix Dahlke, 2015/09/17 14:57:57]

As I suggested before, this file should be renamed to "transform_locales.py" -
there's no point in the "abb_" prefix, it's already in a module called "abb".

+# 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}(-[A-Z]{2})?$")
+_SEARCH_PROPS_RE = re.compile("^browser\.search\."
+                              "(defaultenginename|order\.).*$")
+_SHORTNAME_RE = re.compile("^<ShortName>.*</ShortName>$")
+
+_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")
+
+_DEFAULT_LOCALE = "en-US"
+
+_SEARCH_ENGINE_ORDER = {
+    "en-US": ["duckduckgo",

[Felix Dahlke, 2015/09/17 14:57:56]

Being our file, I guess we should stick to 2 space indents here. But let's
please do that once all other comments have been addressed, in a separate patch
set.

[René Jeschke, 2015/09/18 09:46:38]

Sure. Wasn't aware that we're not fully pep8 :-D

+              "yahoo",
+              "google",
+              "wikipedia",
+              "amazon"],
+    "zh-CN": ["baidu",
+              "duckduckgo",
+              "yahoo",
+              "google",
+              "wikipedia",
+              "amazon"]
+}
+
+
+def _get_shortname_from_id(needle, engine_ids, engine_map):

[Felix Dahlke, 2015/09/17 14:57:57]

Nit: I would find "engine_names" more fitting than "engine_map" here - took some
scrolling to figure out what this is supposed to do, and the function says it
gets a name.

[René Jeschke, 2015/09/18 09:46:38]

Done.

+    """Fuzzy finds needle in engine_ids and returns ShortName"""
+    regex = re.compile("^%s.*$" % needle)
+    for engine in engine_ids:
+        if regex.match(engine.lower()):
+            return engine_map[engine]
+    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, fns):

[Felix Dahlke, 2015/09/17 14:57:57]

Wouldn't hurt readability to split this big function up into smaller ones (as
evident from the fact that it has sections preceeded by comments), but I
wouldn't insist.

[René Jeschke, 2015/09/18 09:46:38]

Then I prefer leaving it like it is.

[Felix Dahlke, 2015/09/18 16:34:11]

Acknowledged.

+    fns["info"]("Processing locale '%s'..." % locale)
+
+    # Check for list.txt existence
+    list_file = os.path.join(path, _LIST_TXT_PATH)

[Felix Dahlke, 2015/09/17 14:57:57]

Nit: Being a path, not a file object, `list_path`? Likewise `region_path` and
`xml_path` below.

[René Jeschke, 2015/09/18 09:46:38]

Done.

+    if not os.path.exists(list_file):
+        fns["fatal"]("Missing 'list.txt' for locale '%s'" % locale)
+
+    # Check for region.properties existence
+    region_file = os.path.join(path, _REGION_PROPS_PATH)
+    if not os.path.exists(region_file):
+        fns["fatal"]("Missing 'region.properties' for locale '%s'" % locale)
+
+    # Get whitelist and build regex
+    whitelist = _SEARCH_ENGINE_ORDER.get(locale,
+                                         _SEARCH_ENGINE_ORDER[_DEFAULT_LOCALE])
+    white_re = re.compile("^(%s).*$" % ("|".join(whitelist)))

[Felix Dahlke, 2015/09/17 14:57:57]

Nit: No need for the extra parentheses around "|".join(whitelist)

[René Jeschke, 2015/09/18 09:46:37]

Done.

+
+    # Read engine IDs from list.txt, discard engines not on whitelist
+    engine_ids = []
+    for line in open(list_file, "r"):

[Felix Dahlke, 2015/09/17 14:57:57]

This way, the file will stay open, you should use a with block:

with open(list_path, "r") as list_file:
  for line in list_file:
    ...

[René Jeschke, 2015/09/18 09:46:38]

Done.

+        line = line.strip()
+        if len(line) > 0:
+            if white_re.match(line.lower()):
+                engine_ids.append(line)
+            else:
+                fns["info"]("Removing '%s'" % line)
+
+    # Make sure we still have search engines left
+    if len(engine_ids) == 0:
+        fns["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 = os.path.join(path, _SEARCHPLUGINS_PATH, "%s.xml" % eid)
+        if os.path.exists(xml_file):
+            short_name = None
+            for line in open(xml_file, "r"):
+                line = line.strip()
+                if _SHORTNAME_RE.match(line):
+                    short_name = line[11:-12].strip()
+
+            if not short_name:
+                fns["fatal"]("No ShortName defined for '%s' in '%s" %
+                             (eid, locale))
+            engine_names[eid] = short_name
+        else:
+            fns["fatal"]("XML definiton for '%s' in '%s' missing" %
+                         (eid, locale))
+
+    fns["info"]("Remaining 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)
+
+    fns["info"]("Resulting ordered list: %s" % (", ".join(engine_order)))
+
+    # Read region.properties and remove browser.search.* lines
+    props = []
+    for line in open(region_file, "r"):
+        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(3, len(engine_order))):
+        props.append("browser.search.order.%d=%s" % (i + 1, engine_order[i]))
+
+    # Write back list.txt
+    _write_lines(list_file, engine_ids)
+
+    # Write back region.properties
+    _write_lines(region_file, props)
+
+
+def _print_info(obj):
+    """ Wrapper for 'self.info' (to be self-contained) """
+    def fn(s):
+        if obj:
+            obj.info(s)
+        else:
+            print "INFO: %s" % s
+    return fn
+
+
+def _print_error(obj):
+    """ Wrapper for 'self.error' (to be self-contained) """
+    def fn(s):
+        if obj:
+            obj.error(s)
+        else:
+            print "ERROR: %s" % s
+    return fn
+
+
+def _exit_fatal(obj):
+    """ Wrapper for 'self.fatal' (to be self-contained) """
+    def fn(s):
+        if obj:
+            obj.fatal(s)
+        else:
+            print "FATAL: %s" % s
+            exit(1)
+    return fn
+
+
+def transform_locales(obj_dir, obj):

[Felix Dahlke, 2015/09/17 14:57:57]

I found the name "obj" pretty non-obvious here, it is in fact a MultiLocaleBuild
instance, and not really related to `obj_dir` at all. Why not call it
multi_locale_build or build_object or something like that?

Also, being sort of a pseudo method, I would expect it to be the first
parameter, where `self` always is.

[René Jeschke, 2015/09/18 09:46:38]

Done.

+    fns = {"info": _print_info(obj),

[Felix Dahlke, 2015/09/17 14:57:56]

I think this is more complex than it would have to be:

`_print_info` etc. are just wrappers that can deal with `obj` being non-None.
Why not just check for that once, right at the beginning of this function?

If we know info, error and fatal can be called, there is no need for the
wrappers, and also not for this `fns` map here - you could just pass in `obj`
instead of `fns`.

Alternatively, you could turn this into a class that has the build object as a
member. Or, I guess, you could even turn this into a mixin for MultiBuildObject.

[René Jeschke, 2015/09/18 09:46:37]

I thought you wanted to be able to use this script without mozharness.

Those are not simply 'wrappers', those methods use currying to hide the fact
that we need an object to call those functions during a mozharness build. When
this object is not available, it uses built-in functionality to replace it.

If you don't care about this script being used standalone, then I just remove
the closures and use 'build_object' directly.

[Felix Dahlke, 2015/09/18 16:34:11]

Well, I think we should either use the build object directly, or decouple it
completely by passing `fns` into `transform_locales`. Either way is fine by me,
I just don't like this mixed approach :)

+           "error": _print_error(obj),
+           "fatal": _exit_fatal(obj)}
+
+    chrome_path = os.path.join(obj_dir, "dist", "bin", "chrome")
+    if not os.path.exists(chrome_path):
+        fns["fatal"]("'dist/bin/chrome' not existent in '%s'" % obj_dir)

[Felix Dahlke, 2015/09/17 14:57:56]

I guess we shouldn't duplicate "dist/bin/chrome" here. You could just say:
`"'%s' does not exist" % chrome_path`

[René Jeschke, 2015/09/18 09:46:37]

Done.

+
+    locales = []
+    for p in next(os.walk(chrome_path))[1]:
+        if _LOCALE_RE.match(p):
+            locales.append(p)
+    locales.sort()

[Felix Dahlke, 2015/09/17 14:57:56]

Why sort them?

[René Jeschke, 2015/09/18 09:46:38]

Why not? :-)

It makes the log output clearer, as you know that 'en-US' will be before 'kk' in
the output ... nothing more and nothing less.

If you really think, that sorting 66 locales might be a performance bottleneck,
or that we do not have the 19 bytes for this line, then I'll remove it.

[Felix Dahlke, 2015/09/18 16:34:11]

No, no concerns, was just wondering if there's a technical reason for it that I
don't understand. Comment wouldn't hurt, but I guess it's fine anyway.

+
+    fns["info"]("Found %d locales" % len(locales))
+
+    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, fns)
+        else:
+            fns["error"]("Missing 'locale' folder for '%s'" % locale)
+