Bug 197432 - [WinCairo] Implement and enable RemoteInspector Server.
Summary: [WinCairo] Implement and enable RemoteInspector Server.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 197029 197435
  Show dependency treegraph
 
Reported: 2019-04-30 13:09 PDT by Basuke Suzuki
Modified: 2019-05-03 14:04 PDT (History)
7 users (show)

See Also:


Attachments
patch (37.06 KB, patch)
2019-05-02 12:39 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
patch (37.68 KB, patch)
2019-05-02 14:34 PDT, Basuke Suzuki
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (14.13 MB, application/zip)
2019-05-02 16:18 PDT, EWS Watchlist
no flags Details
PATCH (36.72 KB, patch)
2019-05-03 12:37 PDT, Basuke Suzuki
ross.kirsling: review+
Details | Formatted Diff | Diff
Patch for landing :) (36.81 KB, patch)
2019-05-03 13:41 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2019-04-30 13:09:11 PDT
Add platform socket implementation and enable RemoteInspector Server on WinCairo.
Comment 1 Basuke Suzuki 2019-05-02 11:12:13 PDT
(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?
Comment 2 Basuke Suzuki 2019-05-02 11:19:19 PDT
(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.
Comment 3 Basuke Suzuki 2019-05-02 12:39:46 PDT
Created attachment 368809 [details]
patch
Comment 4 Ross Kirsling 2019-05-02 13:31:49 PDT
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`).
Comment 5 Basuke Suzuki 2019-05-02 14:34:14 PDT
Created attachment 368822 [details]
patch
Comment 6 Basuke Suzuki 2019-05-02 14:34:45 PDT
Comment on attachment 368822 [details]
patch

Right. Changed.
Comment 7 Ross Kirsling 2019-05-02 15:05:54 PDT
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 8 Christopher Reid 2019-05-02 15:27:53 PDT
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 9 EWS Watchlist 2019-05-02 16:18:51 PDT
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
Comment 10 EWS Watchlist 2019-05-02 16:18:53 PDT
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
Comment 11 Basuke Suzuki 2019-05-03 12:30:18 PDT
(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.
Comment 12 Basuke Suzuki 2019-05-03 12:37:53 PDT
Created attachment 368965 [details]
PATCH

Win's layout tests failure isn't related to this patch. It is red anyway.
Comment 13 Ross Kirsling 2019-05-03 12:56:16 PDT
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.
Comment 14 Basuke Suzuki 2019-05-03 13:41:11 PDT
Created attachment 368978 [details]
Patch for landing :)

Thanks, Ross
Comment 15 WebKit Commit Bot 2019-05-03 14:03:39 PDT
Comment on attachment 368978 [details]
Patch for landing :)

Clearing flags on attachment: 368978

Committed r244919: <https://trac.webkit.org/changeset/244919>
Comment 16 WebKit Commit Bot 2019-05-03 14:03:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2019-05-03 14:04:32 PDT
<rdar://problem/50456158>