Replace webkitpy standalone builders functions with instantiable class.

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
index 63220c727abb0b6d0789800b0953388dc1165e70..e28f114ee7f20ea2b90261718f7be24cc5792ebd 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
@@ -28,70 +28,73 @@
 
 import re
 
-# In this dictionary, each item stores:
-# * port_name -- a fully qualified port name
-# * rebaseline_override_dir -- (optional) directory to put baselines in instead of where you would normally put them.
-#      This is useful when we don't have bots that cover particular configurations; so, e.g., you might
-#      support mac-mountainlion but not have a mac-mountainlion bot yet, so you'd want to put the mac-lion
-#      results into platform/mac temporarily.
-# * specifiers -- TestExpectation specifiers for that config. Valid values are found in
-#      TestExpectationsParser._configuration_tokens_list
 
-_exact_matches = {
-    "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7', 'Release']},
-    "WebKit Win7 (dbg)": {"port_name": "win-win7", "specifiers": ['Win7', 'Debug']},
-    "WebKit Win10": {"port_name": "win-win10", "specifiers": ['Win10', 'Release']},
-    # FIXME: Rename this to 'WebKit Linux Precise'
-    "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
-    "WebKit Linux Trusty": {"port_name": "linux-trusty", "specifiers": ['Trusty', 'Release']},
-    "WebKit Linux (dbg)": {"port_name": "linux-precise", "specifiers": ['Precise', 'Debug']},
-    "WebKit Mac10.9": {"port_name": "mac-mac10.9", "specifiers": ['Mac10.9', 'Release']},
-    "WebKit Mac10.10": {"port_name": "mac-mac10.10", "specifiers": ['Mac10.10', 'Release']},
-    "WebKit Mac10.11": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Release']},
-    "WebKit Mac10.11 (dbg)": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Debug']},
-    "WebKit Mac10.11 (retina)": {"port_name": "mac-retina", "specifiers": ['Retina', 'Release']},
-    "WebKit Android (Nexus4)": {"port_name": "android", "specifiers": ['Android', 'Release']},
-}
-
-
-_ports_without_builders = [
-]
-
-
-def builder_path_from_name(builder_name):
-    return re.sub(r'[\s().]', '_', builder_name)
-
-
-def all_builder_names():
-    return sorted(set(_exact_matches.keys()))
-
-
-def all_port_names():
-    return sorted(set(map(lambda x: x["port_name"], _exact_matches.values()) + _ports_without_builders))
-
-
-def rebaseline_override_dir(builder_name):
-    return _exact_matches[builder_name].get("rebaseline_override_dir", None)
-
-
-def port_name_for_builder_name(builder_name):
-    return _exact_matches[builder_name]["port_name"]
-
-
-def specifiers_for_builder(builder_name):
-    return _exact_matches[builder_name]["specifiers"]
-
-
-def builder_name_for_port_name(target_port_name):
-    debug_builder_name = None
-    for builder_name, builder_info in _exact_matches.items():
-        if builder_info['port_name'] == target_port_name:
-            if 'dbg' in builder_name:
-                debug_builder_name = builder_name
-            else:
+class Builders(object):
+

[qyearsley, 2016/05/02 21:56:36]

Optionally, here (or above) there could be a short description of what an
instance of Builders represents -- does it represent a record of all different
waterfall builders where layout tests are run? And usually the set of builders
is the set that's hard-coded in the constructor, but this can be overridden for
tests?

[bokan, 2016/05/04 11:58:47]

I'm probably not the best person to do this since I don't really know but that's
my understanding. I added a fairly generic comment with a TODO for someone more
knowledgeable to expand. If Dirk knows I can replace this with a better
explanation now.

[Dirk Pranke, 2016/05/04 22:42:36]

Builders is the list of the main chromium builders where the layout tests are
run. There are other
waterfalls that also run them (v8, webrtc), but we don't care about those.

+    def __init__(self):
+
+        # In this dictionary, each item stores:
+        # * port_name -- a fully qualified port name
+        # * rebaseline_override_dir -- (optional) directory to put baselines in instead of where you would normally put them.
+        #      This is useful when we don't have bots that cover particular configurations; so, e.g., you might
+        #      support mac-mountainlion but not have a mac-mountainlion bot yet, so you'd want to put the mac-lion
+        #      results into platform/mac temporarily.
+        # * specifiers -- TestExpectation specifiers for that config. Valid values are found in
+        #      TestExpectationsParser._configuration_tokens_list
+        self._exact_matches = {
+            "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7', 'Release']},
+            "WebKit Win7 (dbg)": {"port_name": "win-win7", "specifiers": ['Win7', 'Debug']},
+            "WebKit Win10": {"port_name": "win-win10", "specifiers": ['Win10', 'Release']},
+            # FIXME: Rename this to 'WebKit Linux Precise'
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+            "WebKit Linux Trusty": {"port_name": "linux-trusty", "specifiers": ['Trusty', 'Release']},
+            "WebKit Linux (dbg)": {"port_name": "linux-precise", "specifiers": ['Precise', 'Debug']},
+            "WebKit Mac10.9": {"port_name": "mac-mac10.9", "specifiers": ['Mac10.9', 'Release']},
+            "WebKit Mac10.10": {"port_name": "mac-mac10.10", "specifiers": ['Mac10.10', 'Release']},
+            "WebKit Mac10.11": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Release']},
+            "WebKit Mac10.11 (dbg)": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Debug']},
+            "WebKit Mac10.11 (retina)": {"port_name": "mac-retina", "specifiers": ['Retina', 'Release']},
+            "WebKit Android (Nexus4)": {"port_name": "android", "specifiers": ['Android', 'Release']},
+        }
+
+        self._ports_without_builders = [
+        ]
+
+    def builder_path_from_name(self, builder_name):
+        return re.sub(r'[\s().]', '_', builder_name)
+
+    def all_builder_names(self):
+        return sorted(set(self._exact_matches.keys()))
+
+    def all_port_names(self):
+        return sorted(set(map(lambda x: x["port_name"], self._exact_matches.values()) + self._ports_without_builders))
+
+    def rebaseline_override_dir(self, builder_name):

[Dirk Pranke, 2016/05/03 23:16:38]

I think we don't use rebaseline_override_dirs any more, so I would get rid of
this.

[bokan, 2016/05/04 11:58:47]

It looks like there's a usage in rebaseline.py that's ultimately used from
RebaselineTest and CopyExistingBaselinesInternal. Or are you saying those are
dead code paths?

[Dirk Pranke, 2016/05/04 22:42:36]

I think they might be dead code, but am not positive. Maybe file a follow-up bug
to check?

[bokan, 2016/05/05 14:20:54]

Done. Filed crbug.com/609472

+        return self._exact_matches[builder_name].get("rebaseline_override_dir", None)
+
+    def port_name_for_builder_name(self, builder_name):
+        return self._exact_matches[builder_name]["port_name"]
+
+    def specifiers_for_builder(self, builder_name):

[Dirk Pranke, 2016/05/03 23:16:38]

Is this function needed/used?

[bokan, 2016/05/04 11:58:47]

In one place, BotTestExpectations uses it in __init__

[Dirk Pranke, 2016/05/04 22:42:36]

Acknowledged.

+        return self._exact_matches[builder_name]["specifiers"]
+
+    def builder_name_for_port_name(self, target_port_name):
+        debug_builder_name = None
+        for builder_name, builder_info in self._exact_matches.items():
+            if builder_info['port_name'] == target_port_name:
+                if 'dbg' in builder_name:
+                    debug_builder_name = builder_name
+                else:
+                    return builder_name
+        return debug_builder_name
+
+    def builder_name_for_specifiers(self, version, build_type):

[Dirk Pranke, 2016/05/03 23:16:38]

Is this function even needed/used? The prior version of the file didn't have it,
right?

[bokan, 2016/05/04 11:58:47]

Ah, yes, this was added and used in my update-test-expectations patch. I've
removed it from here and will land it as part of that work.

+        for builder_name, info in self._exact_matches.items():
+            specifiers = info['specifiers']
+            if specifiers[0].lower() == version.lower() and specifiers[1].lower() == build_type.lower():
                 return builder_name

[qyearsley, 2016/05/02 21:56:36]

Here we're implicitly requiring the specifiers list to be [OS Version, Build
type], but we didn't really make that requirement explicit anywhere. In
rebaseline_unittest, we're using a fake specifier collection
`set(["mock-specifier"])`.

Would it make sense for this function to take a collection of specifiers, and
make sure they're all in the specifiers list for the builder? e.g.

    def builder_name_for_specifiers(self, query_specifiers):
        for builder_name, info in self._exact_matches.items():
            builder_specifiers = [s.lower() for s in info['specifiers']]
            if all(s.lower() in builder_specifiers for s in query_specifiers):
                return builder_name
        return ''

[bokan, 2016/05/04 11:58:47]

Good point. I've removed this method for the purposes of this patch since it's
only needed once my update-test-expectations patch lands but I'll do this on the
other CL.

-    return debug_builder_name
 
+        return ''
 
-def builder_path_for_port_name(port_name):
-    builder_path_from_name(builder_name_for_port_name(port_name))
+    def builder_path_for_port_name(self, port_name):
+        self.builder_path_from_name(self.builder_name_for_port_name(port_name))