[webkit-reviews] review granted: [Bug 208349] Introduce JSRemoteInspectorServerStart API for socket-based RWI. : [Attachment 391997] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 208349: Introduce JSRemoteInspectorServerStart API for socket-based RWI.
https://bugs.webkit.org/show_bug.cgi?id=208349

Attachment 391997: Patch

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




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 391997
  --> https://bugs.webkit.org/attachment.cgi?id=391997
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).