[webkit-reviews] review denied: [Bug 58211] Add method to make a Port able to retrieve TestOutputSets from its builders. : [Attachment 88963] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied James Kozianski <koz at chromium.org>'s
request for review:
Bug 58211: Add method to make a Port able to retrieve TestOutputSets from its
builders.
https://bugs.webkit.org/show_bug.cgi?id=58211

Attachment 88963: Patch
https://bugs.webkit.org/attachment.cgi?id=88963&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88963&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:504
> +    def buildbot_testoutputset(self, platforms):

This should be buildbot_test_output_set.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:205
> +	   platform_builders = {
> +	       'chromium-mac': 'Webkit_Mac10_5',
> +	       'chromium-win-vista': 'Webkit_Win',
> +	       'chromium-linux': 'Webkit_Linux__dbg__1_',
> +	   }

This is a generally useful map. Lets move this to a map next to
ALL_BASELINE_VARIANTS and generate ALL_BASELINE_VARIANTS from this map. Lets
talk in person about what the best location for this data is.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:212
> +	       zip_url =
'http://build.chromium.org/f/chromium/layout_test_results/' + \

\ line continuations are so old-school. use parens instead.

> Tools/Scripts/webkitpy/layout_tests/port/factory.py:131
> +def get_all(options=None):

This doesn't look like it's used in this patch. Did you mean to include it?

> Tools/Scripts/webkitpy/layout_tests/port/factory.py:134
> +    return dict([(port_name, get(port_name, options=options))
> +			for port_name in all_port_names()])

This indentation is weird. Also, we don't have a line limit. I think this would
read fine on one line.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:80
> +	   platform_builders = {
> +	       'mac-snowleopard': 'SnowLeopard Intel Release (Tests)',
> +	       'mac-leopard': 'Leopard Intel Debug (Tests)',
> +	       'win': 'Windows 7 Release (WebKit2 Tests)',
> +	       'chromium-linux': 'GTK Linux 32-bit Release',
> +	   }

ditto above

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:86
> +	   bb = BuildBot()

abbreviations are discouraged in webkit code. also, you don't really need this
local variable, do you? it's only used on line 90.