Bug 208349 – Introduce JSRemoteInspectorServerStart API for socket-based RWI.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Introduce JSRemoteInspectorServerStart APPI for socket-based RWI.

[Ross Kirsling]

Created attachment 391937 [details]
Patch

[Ross Kirsling]

This is needed for embedded applications making use of socket-based RWI.

A couple of possible concerns:

- I've guarded the declaration itself here based on Devin's recommendation, though it does seem a bit more typical to just no-op the implementation instead.

- Our downstream version calls initializeThreading() too; I'd rather omit this as it feels like an orthogonal concern, but it can be added here if we absolutely need it.

[Don Olmstead]

Comment on attachment 391937 [details]
Patch

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

> Source/JavaScriptCore/API/JSRemoteInspector.h:55
> +#if USE(INSPECTOR_SOCKET_SERVER)
> +JS_EXPORT bool JSRemoteInspectorServerStart(uint16_t port) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));
> +#endif
> +

I'm pretty sure having a USE in the API headers is a no go. These are all C headers. I'm going to guess there might need to be a JSRemoteInspectorSocket.h.

[Ross Kirsling]

Created attachment 391996 [details]
Patch

[Ross Kirsling]

Created attachment 391997 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 391997 [details]
Patch

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

Seems fine to me. It is weird exposing an API that does nothing on Apple ports but were they to implement `INSPECTOR_SOCKET_SERVER` then it would just work.

> Source/JavaScriptCore/API/JSRemoteInspector.cpp:92
> +    auto& server = Inspector::RemoteInspectorServer::singleton();
> +    return server.isRunning() || server.start(nullptr, port);

If the server is running it might be running on a different port. Should we return false in such a case, since this was requested to start with a particular port?

Have you considered an option to start without a port, and let the OS pick a random available port? (Not necessarily important, just something to consider).

[Devin Rousso]

Comment on attachment 391997 [details]
Patch

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

> Source/JavaScriptCore/API/JSRemoteInspector.h:53
> +JS_EXPORT bool JSRemoteInspectorServerStart(uint16_t port) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));

I'm not familiar with the "rules" about the JSC C API, but I'd think that we wouldn't want to expose a function that doesn't actually do anything.  If this is done elsewhere (and the JSC folks are fine with it), then I guess it's fine.  I'd really prefer if the function only existed when `USE(INSPECTOR_SOCKET_SERVER)` is guaranteed to be true.

[Joseph Pecoraro]

(In reply to Don Olmstead from comment #3)
> Comment on attachment 391937 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391937&action=review
> 
> > Source/JavaScriptCore/API/JSRemoteInspector.h:55
> > +#if USE(INSPECTOR_SOCKET_SERVER)
> > +JS_EXPORT bool JSRemoteInspectorServerStart(uint16_t port) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));
> > +#endif
> > +
> 
> I'm pretty sure having a USE in the API headers is a no go. 

Correct regarding having `USE` in an API header!

> These are all C headers. I'm going to guess there might need
> to be a JSRemoteInspectorSocket.h.

This isn't a bad idea, I considered it as well. Then ports that don't want to expose this API/Symbol could just not include the header/implementation. I'm not sure if there is a common practice for that.

[Don Olmstead]

Comment on attachment 391997 [details]
Patch

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

>> Source/JavaScriptCore/API/JSRemoteInspector.cpp:92
>> +    return server.isRunning() || server.start(nullptr, port);
> 
> If the server is running it might be running on a different port. Should we return false in such a case, since this was requested to start with a particular port?
> 
> Have you considered an option to start without a port, and let the OS pick a random available port? (Not necessarily important, just something to consider).

I think address should probably be exposed as well on this API.

[Don Olmstead]

Comment on attachment 391997 [details]
Patch

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

>> Source/JavaScriptCore/API/JSRemoteInspector.h:53
>> +JS_EXPORT bool JSRemoteInspectorServerStart(uint16_t port) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));
> 
> I'm not familiar with the "rules" about the JSC C API, but I'd think that we wouldn't want to expose a function that doesn't actually do anything.  If this is done elsewhere (and the JSC folks are fine with it), then I guess it's fine.  I'd really prefer if the function only existed when `USE(INSPECTOR_SOCKET_SERVER)` is guaranteed to be true.

I don't know what the practice is for it but if it was to go into a separate file then on the CMake side the header would need to be appended to JavaScriptCore_PUBLIC_FRAMEWORK_HEADERS on the CMake side.

if (USE_INSPECTOR_SOCKER_SERVER)
    list(APPEND JavaScriptCore_PUBLIC_FRAMEWORK_HEADERS API/JSRemoteInspectorServer.h
endif ()

[Basuke Suzuki]

Comment on attachment 391997 [details]
Patch

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

>> Source/JavaScriptCore/API/JSRemoteInspector.h:53
>> +JS_EXPORT bool JSRemoteInspectorServerStart(uint16_t port) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));
> 
> I'm not familiar with the "rules" about the JSC C API, but I'd think that we wouldn't want to expose a function that doesn't actually do anything.  If this is done elsewhere (and the JSC folks are fine with it), then I guess it's fine.  I'd really prefer if the function only existed when `USE(INSPECTOR_SOCKET_SERVER)` is guaranteed to be true.

We cannot use USE(INSPECTOR_SOCKET_SERVER) here in C API headers. All we can do is using standard definition check for possible platforms to enable this. i.e) #if defined(WIN32) || defined(_WIN32) || defined(__SCE__)

[Ross Kirsling]

Created attachment 392020 [details]
Patch

[Ross Kirsling]

Comment on attachment 392020 [details]
Patch

Here's a version that addresses most of the voiced concerns.

Not going to land this yet though, as Basuke would like to adjust the return type of RemoteInspectorServer::start in bug 208391 first.

[Ross Kirsling]

Created attachment 392335 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392335 [details]
Patch

Clearing flags on attachment: 392335

Committed r257809: <https://trac.webkit.org/changeset/257809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60017043>