Bug 197432

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

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

Created attachment 368809 [details] patch

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

Created attachment 368822 [details] patch

Comment on attachment 368822 [details] patch Right. Changed.

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`. :)

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 `= { }`.

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

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

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

Created attachment 368965 [details] PATCH Win's layout tests failure isn't related to this patch. It is red anyway.

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.

Created attachment 368978 [details] Patch for landing :) Thanks, Ross

Comment on attachment 368978 [details] Patch for landing :) Clearing flags on attachment: 368978 Committed r244919: <https://trac.webkit.org/changeset/244919>

All reviewed patches have been landed. Closing bug.

<rdar://problem/50456158>