[Webkit-unassigned] [Bug 197432] [WinCairo] Implement and enable RemoteInspector Server.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=197432

--- Comment #1 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
(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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190502/a761a6e7/attachment-0001.html>