Bug 239495 – IPC testing API should have the ability to test IPC::Connection send and receive through IPC::Connection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Kimmo Kinnunen]

IPC testing API should have the ability to test IPC::Connection send and receive through IPC::Connection

[Kimmo Kinnunen]

Created attachment 458055 [details]
Patch

[Darin Adler]

Comment on attachment 458055 [details]
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.

[Kimmo Kinnunen]

(In reply to Darin Adler from comment #2)
> > 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.

bug 239643 - Add a smart pointer to hold JSValueProtect() objects

> 
> > 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.

I think this is due to the system using the public JS API instead of more normal way of doing JS-C++ integration in WebKit

Bug 239644 - IPC testing API should use the existing IDL bindings instead of manual binding with public JS API

[Kimmo Kinnunen]

Created attachment 458123 [details]
For landing

[Darin Adler]

(In reply to Kimmo Kinnunen from comment #3)
> > 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.
> 
> I think this is due to the system using the public JS API instead of more
> normal way of doing JS-C++ integration in WebKit

Sure but my separate point was that within this file a helper could make the code less repetitive and perhaps less error prone.

There may be reasons it’s not easy to use the IDL bindings here. In WebKitTestRunner we also have a lot of code using the JavaScript C API rather than our bindings system, but since it’s C++ code, I wrote lots of helper functions there for WebKitTestRunner to make the idioms simpler and less error prone.

[Radar WebKit Bug Importer]

<rdar://problem/92331751>

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 458123 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Kimmo Kinnunen]

(In reply to Darin Adler from comment #5)
> (In reply to Kimmo Kinnunen from comment #3)
> > > 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.
> > 
> > I think this is due to the system using the public JS API instead of more
> > normal way of doing JS-C++ integration in WebKit
> 
> Sure but my separate point was that within this file a helper could make the
> code less repetitive and perhaps less error prone.
> 
> There may be reasons it’s not easy to use the IDL bindings here. In
> WebKitTestRunner we also have a lot of code using the JavaScript C API
> rather than our bindings system, but since it’s C++ code, I wrote lots of
> helper functions there for WebKitTestRunner to make the idioms simpler and
> less error prone.

I see what I can do to reduce the repetitiveness. The initial reaction is that the idl system should be the best option here.

[Kimmo Kinnunen]

Created attachment 458445 [details]
For landing

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Kimmo Kinnunen]

Created attachment 458496 [details]
[fast-cq] For landing

[EWS]

Committed r293567 (250081@main): <https://commits.webkit.org/250081@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458496 [details].