[webkit-reviews] review granted: [Bug 169419] Use TCP instead of FIFOs for Simulator/Device communication : [Attachment 305601] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 169419: Use TCP instead of FIFOs for Simulator/Device communication
https://bugs.webkit.org/show_bug.cgi?id=169419

Attachment 305601: Patch

https://bugs.webkit.org/attachment.cgi?id=305601&action=review




--- Comment #15 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 305601
  --> https://bugs.webkit.org/attachment.cgi?id=305601
Patch

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

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1240
> +    setupiOSLayoutTestCommunication();

"Set up" is two words, so the name should be setUpIOSLayoutTestCommunication
(note upper case IOS too).

> Tools/Scripts/webkitpy/port/device.py:41
> +    def setup(self):

I don't think that set_up would be reasonable here, but maybe a more
descriptive name would sidestep the naming problem.

> Tools/Scripts/webkitpy/port/device.py:45
> +	       self.listening_socket.bind(('localhost', 0))

One issue that we had with localhost in the past is that it would flakily
resolve to IPv6 loopback on some ancient OS versions, and cause strange
failures. This is probably not a problem any more. Maybe it's good to be
specific anyway.

> Tools/Scripts/webkitpy/port/ios_simulator.py:69
> +    #FIXME: Ports are recreated in each process.  This is a problem for
IOSSimulatorPort, it means devices are not persistent

Please add a period at the end, and do not use French spacing.

It would be helpful to explain why having he devices not persistent is a
problem.

> Tools/Scripts/webkitpy/port/simulator_process.py:58
> +    # Python 2's implementation of makefile does not return
> +    # a non-blocking file.

No need to wrap this line.

> Tools/Scripts/webkitpy/port/simulator_process.py:108
> +	       # Order matters here!

A better comment would say something like "this order matches what is being
sent in <function_name>".

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:47
> +    if (result < 0)
> +	   abort();
> +    if (connect(result, (struct sockaddr *) &serverAddress,
sizeof(serverAddress)) < 0)
> +	   abort();

Would it be useful to print errors here? Or alternatively to crash. Otherwise,
it will be hard to debug.

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:55
> +	   abort();

Ditto. RELEASE_ASSERT?

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:57
> +    struct hostent* host = gethostbyname("localhost");

localhost or 127.0.0.1?

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:59
> +    memset((char *) &serverAddress, 0, sizeof(serverAddress));

Misplaced star.

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:67
> +    // Order matters!

:-|

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:78
> +void teardowniOSLayoutTestCommunication()

Two words, so "tearDownIOSLayoutTestCommunication" (note upper case IOS too).

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:88
> +void setupiOSLayoutTestCommunication() { }
> +void teardowniOSLayoutTestCommunication() { }

This doesn't seem needed.