[webkit-reviews] review denied: [Bug 63501] Simplify skipped file finding in preparation for adding wk2 skipped list fallback : [Attachment 98992] Slightly smaller patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 63501: Simplify skipped file finding in preparation for adding wk2 skipped
list fallback
https://bugs.webkit.org/show_bug.cgi?id=63501

Attachment 98992: Slightly smaller patch
https://bugs.webkit.org/attachment.cgi?id=98992&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
The patch generally looks pretty good. My comments mostly relate to various
FIXMEs, docstrings, function names, and other comments ...

View in context: https://bugs.webkit.org/attachment.cgi?id=98992&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base.py:541
> +	   """The base name of the port, not including version information."""

We should probably provide examples here ... should it be "apple" or "mac"?
"chromium" or "chromium-mac", etc.? How is this different from "name"?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:546
> +	   # FIXME: This should be replaced by "return "%s-%s"
(self.port_name(), self.version())"

As noted in other comments, this FIXME is wrong and the doc string is still
simplistic. I'd replace this by something along the lines of 

"""Returns a name that uniquely identifies this particular type of port (e.g.,
"mac-snowleopard" or "chromium-gpu-linux-x86_x64" and can be passed back to
factory.get() or the constructor"".

> Tools/Scripts/webkitpy/layout_tests/port/base.py:551
> +	   """Returns the actual name of the port, not the delegate's."""

"""Returns the name of the port as passed to the --platform command line
argument.

This can be different from port.name() for ports like the MockDRT port."""

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:71
> +	   self._parse_port_name(port_name)

I'm always a little uncomfortable with calling other routines from inside
constructors. Maybe we should add a comment to _parse_port_name() that this may
be called from inside a constructor and the object may not be fully
initialized? Alternatively, we could make this a static member function, and
pass a tuple back or something like that ...

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:70
> +    # FIXME: version, name, etc. should be passed separately and we should
not have to parse port_name.

I don't understand this comment. Why would a routine called _parse_port_name()
not have to parse the port name? Obviously you're trying to get at something
else ...

> Tools/Scripts/webkitpy/layout_tests/port/config.py:121
> +	       config_module_path =
self._filesystem.path_to_module(self.__module__)

I suggest you change filesystem.path_to_module() to return an abspath and be
clear about it.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:54
> +    # FIXME: There is no mac-future directory, I don't understand why this
code is here.

Remove this per the comments in the bug? There will never be a mac-future
directory, but port names do not necessarily map directly to baseline dirs.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:71
>	   'wk2': ['mac-wk2', 'mac'],

As discussed, "-wk2" isn't really a version, and probably should represent a
different axis of the test configuration, just like -gpu- and -x86 do in
chromium. I would update the FIXME accordingly.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:69
> +    # port which requires platform-specfic results. We should replace this
with a smarter

Typo: "specific", not "specfic". Also, do you mean os-version-specific results,
or os-specific results? I'm not sure what "platform" refers to in this context.


> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:296
> +    def wk2_port_name(self):

Is this supposed to be a public function? Seems like it should only be
_wk2_port_name() instead?

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:330
> +    def _skipped_list_as_expectations_file(self):

Nit: Technically, you're returning a string here ... I'd call this
"_skipped_list_as_expectations()" instead.

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:49
> +	   WebKitPort.__init__(self, port_name="test", executive=executive,
filesystem=filesystem, user=MockUser(), **kwargs)

Given that "test" refers to the "test" port in port/test.py, this might be
confusing. "webkit-test" or "test-webkit" maybe?