Bug 239495

Created attachment 458055 [details]
Patch

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.

(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

Created attachment 458123 [details]
For landing

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

<rdar://problem/92331751>

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

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

Created attachment 458445 [details]
For landing

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

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

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