[webkit-reviews] review granted: [Bug 34511] refactor run-chromium-webkit-tests, make work for webkit mac platform : [Attachment 48161] resubmitted patch 3 of 4; not sure why the bots didn't like the last one

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 34511: refactor run-chromium-webkit-tests, make work for webkit mac
platform
https://bugs.webkit.org/show_bug.cgi?id=34511

Attachment 48161: resubmitted patch 3 of 4; not sure why the bots didn't like
the last one
https://bugs.webkit.org/attachment.cgi?id=48161&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
expected_baselines could have just always returned a list, and the callers that
only care about the first, could have only looked at the first?  But perhaps
all_baselines is a performance optimization.  Either way, not something we need
to change before landing this patch. :)

We should probably hold the ports as contants somewhere, but again, doesn't
matter for this patch:
 210	     if relative_path.startswith(LAYOUTTEST_HTTP_DIR):
 211		 # http/tests/ run off port 8000 and ssl/ off 8443
 212		 relative_path = relative_path[len(LAYOUTTEST_HTTP_DIR):]
 213		 port = 8000
 214	     elif relative_path.startswith(LAYOUTTEST_WEBSOCKET_DIR):
 215		 # websocket/tests/ run off port 8880 and 9323
 216		 # Note: the root is /, not websocket/tests/
 217		 port = 8880

Interesting.  Will this re-raise e?:
 251	     except OSError, e:
 252		 if e.errno != errno.EEXIST:
 253		     raise

Looks functional, and better than what we have.

It's hard to give an in-depth review of a change this large.  But I'm willing
to rubber-stamp this for sure!

I looked at every line.  I'm just not sure that my little brain could actually
hold it all in at once. :)

Thanks for the patch!