Bug 197432 – [WinCairo] Implement and enable RemoteInspector Server.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

Add platform socket implementation and enable RemoteInspector Server on WinCairo.

[Basuke Suzuki]

(In reply to Ross Kirsling from comment https://bugs.webkit.org/show_bug.cgi?id=197434#c3)
> Comment on attachment 368718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368718&action=review
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:56
> > -#if PLATFORM(PLAYSTATION)
> > +#if USE(RI_SOCKET_SERVER)
> 
> Perhaps RWI would be clearer than just RI? Or maybe we could just say
> USE(INSPECTOR_SOCKET_SERVER) since we know the non-remote inspector doesn't
> need a socket server?

Good. I prefer INSPECTOR_SOCKET_SERVER. It makes sense perfectly.


> > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:63
> > +        LOG_ERROR("Inspector server listens for targets already.");
> 
> Nit: "Inspector server is already listening for targets."


> > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:112
> > +        { "SetTargetList"_s, reinterpret_cast<CallHandler>(&RemoteInspectorServer::setTargetList)},
> 
> Nit: You added a space to this one but not the others. ;)

Oh my.


> (Technically I think all of these pairs should begin and end with a space?
> But either way consistency is the most important...)

I agree with that.

> > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:38
> > +static PlatformSocketType create();
> 
> If the declaration needs to be up here, should we just move the definition
> too?

I will move that to static member of the class.


> > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:88
> > +    if (socket == INVALID_SOCKET) {
> > +        LOG_ERROR("socket() failed, errno = %d", WSAGetLastError());
> > +        return INVALID_SOCKET_VALUE;
> 
> Seems like INVALID_SOCKET_VALUE is the same as INVALID_SOCKET on Windows --
> need we use both?

I intentionally use WinSock definition when it is in the context of WinSock API usage. The value is returned from WinSock's `socket()` function so that I want to compare with INVALID_SOCKET.

> 
> > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:231
> > +    // fcntl(socket, F_SETFD, FD_CLOEXEC);
> > +    // int flags = fcntl(socket, F_GETFL, 0);
> > +    // fcntl(socket, F_SETFL, flags | O_NONBLOCK);
> 
> Can these comments be deleted?

Can I add FIXME comment for future improvement?

[Basuke Suzuki]

(In reply to Basuke Suzuki from comment #1)
> > > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:231
> > > +    // fcntl(socket, F_SETFD, FD_CLOEXEC);
> > > +    // int flags = fcntl(socket, F_GETFL, 0);
> > > +    // fcntl(socket, F_SETFL, flags | O_NONBLOCK);
> > 
> > Can these comments be deleted?
> 
> Can I add FIXME comment for future improvement?

I confirmed these are not required on Windows. I'll remove them.

[Basuke Suzuki]

Created attachment 368809 [details]
patch

[Ross Kirsling]

Comment on attachment 368809 [details]
patch

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

> Source/JavaScriptCore/inspector/remote/socket/posix/RemoteInspectorSocketPOSIX.cpp:72
> -Optional<PlatformSocketType> listen(uint16_t port)
> +Optional<PlatformSocketType> listen(const char* address, uint16_t port)
>  {
> -    struct sockaddr_in address = { };
> +    struct sockaddr_in addr = { };

Instead of abbreviating the local, can we make the parameter more descriptive?
Seems like you're calling this parameter `addressStr` in the Windows implementation (and connect() has `serverAddress`).

[Basuke Suzuki]

Created attachment 368822 [details]
patch

[Basuke Suzuki]

Comment on attachment 368822 [details]
patch

Right. Changed.

[Ross Kirsling]

Comment on attachment 368822 [details]
patch

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

> Source/JavaScriptCore/inspector/remote/socket/posix/RemoteInspectorSocketPOSIX.cpp:46
> +Optional<PlatformSocketType> connect(const char* addressStr, uint16_t serverPort)

I don't think you need to change this parameter name (unless you want to change it in the header and Windows implementation too).
I just was mentioning different ways to avoid having to change `address` to `addr`. :)

[Christopher Reid]

Comment on attachment 368809 [details]
patch

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

Some infromal review comments.

> ChangeLog:8
> +        Add new build flag USE_RI_SOCKET_SERVER to indicate using Socket imprementation for RemoteInspector

USE_RI_SOCKET_SERVER should be updated
Nit: typo in "imprementation"

> Source/JavaScriptCore/ChangeLog:11
> +        Also add listener interface for connection betweeb RemoteInspector and RemoteInspectorServer

Nit: typo in "betweeb"

> Source/JavaScriptCore/inspector/remote/socket/posix/RemoteInspectorSocketPOSIX.cpp:171
> +    memset(&address, 0, len);

This can just be struct sockaddr_in address = { }; https://bugs.webkit.org/show_bug.cgi?id=193806#c40

> Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:210
> +    address.sin_family = AF_INET;

Windows 10 has AF_UNIX now, can we use that for UIProcess to WebProcess communication instead of AF_INET?
It would be nice if we can limit what connections are open to the network if we can.

> Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:259
> +    struct sockaddr_in address;

This can also be `= { }`.

[EWS Watchlist]

Comment on attachment 368822 [details]
patch

Attachment 368822 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12066867

New failing tests:
legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html

[EWS Watchlist]

Created attachment 368839 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Basuke Suzuki]

(In reply to Christopher Reid from comment #8)
> Comment on attachment 368809 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368809&action=review
> 
> Some infromal review comments.
> 
> > ChangeLog:8
> > +        Add new build flag USE_RI_SOCKET_SERVER to indicate using Socket imprementation for RemoteInspector
> 
> USE_RI_SOCKET_SERVER should be updated
> Nit: typo in "imprementation"

Right.

> > Source/JavaScriptCore/ChangeLog:11
> > +        Also add listener interface for connection betweeb RemoteInspector and RemoteInspectorServer
> 
> Nit: typo in "betweeb"

Right.

> > Source/JavaScriptCore/inspector/remote/socket/posix/RemoteInspectorSocketPOSIX.cpp:171
> > +    memset(&address, 0, len);
> 
> This can just be struct sockaddr_in address = { };
> https://bugs.webkit.org/show_bug.cgi?id=193806#c40

Oh, thank you.

> > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:210
> > +    address.sin_family = AF_INET;
> 
> Windows 10 has AF_UNIX now, can we use that for UIProcess to WebProcess
> communication instead of AF_INET?
> It would be nice if we can limit what connections are open to the network if
> we can.

Yeah, I'm also exciting about that, but I want to keep this patch with old-fashioned AF_INET socket for simplicity and keep AF_UNIX support for next ticket. 

> > Source/JavaScriptCore/inspector/remote/socket/win/RemoteInspectorSocketWin.cpp:259
> > +    struct sockaddr_in address;
> 
> This can also be `= { }`.

Yep.

[Basuke Suzuki]

Created attachment 368965 [details]
PATCH

Win's layout tests failure isn't related to this patch. It is red anyway.

[Ross Kirsling]

Comment on attachment 368965 [details]
PATCH

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

r=me with nits

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h:65
> -    void didAccept(ConnectionID, Socket::Domain) override;
> +    void didAccept(ConnectionID, ConnectionID listenerID, Socket::Domain) override;

You should probably label both ConnectID parameters here.

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.h:72
> -Optional<PlatformSocketType> listen(uint16_t port);
> +Optional<PlatformSocketType> listen(const char* address, uint16_t port);

This parameter is now called addressStr, though I suppose the names here could just be omitted.

[Basuke Suzuki]

Created attachment 368978 [details]
Patch for landing :)

Thanks, Ross

[WebKit Commit Bot]

Comment on attachment 368978 [details]
Patch for landing :)

Clearing flags on attachment: 368978

Committed r244919: <https://trac.webkit.org/changeset/244919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50456158>