[webkit-reviews] review granted: [Bug 45801] new-run-webkit-tests: add port-specific mechanism for finding list of tests to run and the ability to run w/o needing tests in the filesystem : [Attachment 67993] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 45801: new-run-webkit-tests: add port-specific mechanism for finding list
of tests to run and the ability to run w/o needing tests in the filesystem
https://bugs.webkit.org/show_bug.cgi?id=45801

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

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

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:379
> +	       return self.relative_test_filename(test)
> +	   elif uri.startswith("http://127.0.0.1:8880/"):
> +	       # websocket tests
> +	       test = test.replace('http://127.0.0.1:8880/',
> +				   '')
> +	       return test
> +	   elif uri.startswith("http://"):
> +	       # regular HTTP test
> +	       test = test.replace('http://127.0.0.1:8000/',
> +				   'http/tests/')
> +	       return test
> +	   elif uri.startswith("https://"):

Per webkit style, these should not be elif since they early return in each
block. Once they're made if statements, i'd also prefer to see a newline after
each return.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:151
>	   try:
> -	       if self._executive.run_command(cmd, return_exit_code=True) == 0:

> -		   return False
> -	   except OSError, e:
> -	       if e.errno == errno.ENOENT or e.errno == errno.EACCES:
> -		   _compare_available = False
> -	       else:
> -		   raise e
> +	       try:
> +		   if self._executive.run_command(cmd, return_exit_code=True)
== 0:
> +		       return False
> +	       except OSError, e:
> +		   if e.errno == errno.ENOENT or e.errno == errno.EACCES:
> +		       _compare_available = False
> +		   else:
> +		       raise e
> +	   finally:

Can't you write this as a single try/except/finally instead of a try/except
nested inside a try/finally?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:108
> +	   if not self._have_read_expected_checksum:

Do you really need the boolean? Can't you just check "if not
self._image_checksum:"?