[webkit-reviews] review denied: [Bug 57640] [GTK] overlapping drag&drop tests fail on NRWT : [Attachment 96434] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Xan Lopez <xan.lopez at gmail.com>'s
request for review:
Bug 57640: [GTK] overlapping drag&drop tests fail on NRWT
https://bugs.webkit.org/show_bug.cgi?id=57640

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=96434) [details] [details]
> > Patch
> 
> A couple of comments:
> 
> - I had to copy the start method from the WebKit driver because I need to add
things to the environment and the port method to set it up runs to soon (I
don't know the worker number). I guess it can be fixed with some refactoring.
>

Overriding the start method is exactly what I'd expect you to do. I don't think
I'd want to change or refactor anything, but maybe I'm not understanding your
concern?
 
> - The Xvfb processes are not killed if the test run is stopped with Ctrl-C,
not sure how to do this.

This is a problem. Is driver.stop() not getting called after the Ctrl-C is
received? It should be. You can try adding a _log.debug() message and then
running with --verbose to see what's going on one way or another; feel free to
post the log or email it to me for help debugging it.


> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:37
> +from webkitpy.layout_tests.port.webkit import WebKitPort, WebKitDriver

Nit: I usually prefer to just import the module, and prefix the symbols with
the module name (so, webkit.WebKitPort below instead of just WebKitPort); I
think Eric wrote this code originally before we converged on that style. You
can change this or not as you like for now.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:49
> +	   run_xvfb = ["Xvfb", ":%d" % (self._worker_number + 1)]

Nit: I'd probably do something like 'display_id = self._worker_number + 1' here
to make it clear and avoid having to do the computation twice.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:50
> +	   self._process = subprocess.Popen(run_xvfb)

Can you rename this to self._xvfb_process or something like that? _process is a
little too generic.

>>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:52
>>> +	     environment['DYLD_FRAMEWORK_PATH'] = self._port._build_path()
>> 
>> Is this actually need for GTK+? This environment variable controls Framework
loading on OS X.
> 
> I don't think we use any of the variables I copied, this one we don't for
sure. I guess I just left it there in case we want to make this "more portable"
and to keep the exact behavior we have now, but probably we should just make it
GTK+-only for now.

Definitely remove the DYLD_* and DUMPRENDERTREE_TEMP variables if you don't
need them.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:72
> +	   self._worker_number = worker_number

Do you need self._worker_number? I don't see it being used anywhere.