[webkit-reviews] review denied: [Bug 84008] Add public page loading performance tests using web-page-replay : [Attachment 144957] Initial implementation

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 84008: Add public page loading performance tests using web-page-replay
https://bugs.webkit.org/show_bug.cgi?id=84008

Attachment 144957: Initial implementation
https://bugs.webkit.org/attachment.cgi?id=144957&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144957&action=review


Patch generally looks good, just some minor comments

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:533
> +	       command = driver_input.test_name

Seems like this should just go in is_http_test()? I understand that you might
be thinking 'that's just for tests in the http/ directory', but it's probably
more confusing this way.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:72
> +	   if not output or output.text == None or output.error:

when would output be None here? driver.run_test() should always return
something; can you call run_failed() outside of run()?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:176
> +    def run(self, port, driver, time_out_ms):

Does it make more sense to pass the port to __init__()?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:210
> +	   return driver.run_test(DriverInput(self.path_or_url(), time_out_ms,
None, False))

Maybe this should go up into PerfTest() and then PerfTest.run() can just call
this to avoid the repetition?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:219
> +	   replay_path =
os.path.join(os.path.dirname(webkitpy.thirdparty.autoinstalled.webpagereplay.__
file__), 'replay.py')

can you get this from just
webkitpy.thirdparty.autoinstalled.webpagereplay.replay.__file__ ?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:230
> +		   connection = socket.create_connection(('localhost', '8080'),
timeout=100)

timeouts are in seconds, are you really trying to wait for 100 seconds and
retrying 100 times, or did you mean for this to be msecs?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:239
> +	       self._process.send_signal(signal.SIGINT)

curious why this is SIGINT instead of TERM or KILL?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:311
> +	       filesystem = driver._port.host.filesystem

You can just access port.host.filesystem directly.