[webkit-reviews] review granted: [Bug 239495] IPC testing API should have the ability to test IPC::Connection send and receive through IPC::Connection : [Attachment 458055] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 239495: IPC testing API should have the ability to test IPC::Connection
send and receive through IPC::Connection
https://bugs.webkit.org/show_bug.cgi?id=239495

Attachment 458055: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 458055
  --> https://bugs.webkit.org/attachment.cgi?id=458055
Patch

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

> Source/WebKit/Shared/IPCConnectionTester.cpp:48
> +#if USE(UNIX_DOMAIN_SOCKETS)
> +    IPC::Connection::Identifier connectionHandle {
connectionIdentifier.release().release() };
> +#elif OS(DARWIN)
> +    IPC::Connection::Identifier connectionHandle {
connectionIdentifier.port() };
> +#elif OS(WINDOWS)
> +    IPC::Connection::Identifier connectionHandle {
connectionIdentifier.handle() };
> +#else
> +    notImplemented();
> +    IPC::Connection::Identifier connectionHandle { };
> +#endif
> +    return connectionHandle;

Why the local variable? Why not just use "return" statements?

> Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:414
> +	   JSGlobalContextRetain(JSContextGetGlobalContext(context));
> +	   JSValueProtect(context, resolve);
> +	   JSValueProtect(context, reject);

I strongly suggest we use smart pointers in cases like this rather than
hand-written function call pairs. But maybe the need is not quite as strong for
test code? And maybe we don’t already have the correct smart pointer classes
already handy to use? I thought we did, but not sure.

> Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:508
> +	   if (scope.exception()) {
> +	       *exception = toRef(globalObject, scope.exception());
> +	       scope.clearException();

Maybe we should consider a helper function that does this repeated pattern. The
coding idioms here are repetitive enough that I am craving some helper
functions.