Add implementation of Remote Web Inspector client for WinCairo.
Created attachment 368716 [details] PATCH
Created attachment 368718 [details] Patch
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? > 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. ;) (Technically I think all of these pairs should begin and end with a space? But either way consistency is the most important...) > 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? > 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? > 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?
Comment on attachment 368718 [details] Patch Sorry, I sent a wrong patch to this bug!
Created attachment 370083 [details] Patch
Comment on attachment 370083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370083&action=review Some informal comments > Source/WebKit/UIProcess/win/RemoteInspectorClient.cpp:93 > + RELEASE_LOG("Failed"); Is this release log needed? > Source/WebKit/UIProcess/win/WebView.cpp:241 > + m_page->setURLSchemeHandlerForScheme(RemoteInspectorProtocolHandler::create(*m_page), "inspector"); It would be nice if this scheme was just accessible from URL bar navigations and not links in pages. It's consistent with what GTK does currently so it might be okay.
Created attachment 370131 [details] Patch
Created attachment 370258 [details] Patch
Comment on attachment 370258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370258&action=review It looks like Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp is the only real Windows file specific file. I would probably put the rest into a socket directory to match whats happening in JavaScriptCore. > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173 > +#if !ENABLE(REMOTE_INSPECTOR) || (!PLATFORM(MAC) && !PLATFORM(GTK) && !PLATFORM(WIN_CAIRO)) It seems like this is basically asking if its inspectable from itself. Are there other places where this is present? If so I would say to make this into its own ENABLE or USE. If not I would do || PLATFORM(IOS) || PLATFORM(WPE) and maybe even add a comment about it.
Created attachment 370267 [details] Patch
Comment on attachment 370267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370267&action=review > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173 > +#if !ENABLE(REMOTE_INSPECTOR) && !PLATFORM(MAC) You're implementing this below. This is incorrect. > Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp:101 > +WebPageProxy* RemoteWebInspectorProxy::platformCreateFrontendPageAndWindow() Here's an implementation.
(In reply to Don Olmstead from comment #11) > Comment on attachment 370267 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370267&action=review > > > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173 > > +#if !ENABLE(REMOTE_INSPECTOR) && !PLATFORM(MAC) > > You're implementing this below. This is incorrect. > > > Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp:101 > > +WebPageProxy* RemoteWebInspectorProxy::platformCreateFrontendPageAndWindow() > > Here's an implementation. It's not incorrect. RWIProxyMac is the only implementation that's not conditioned on ENABLE(REMOTE_INSPECTOR).
(In reply to Ross Kirsling from comment #12) > (In reply to Don Olmstead from comment #11) > > Comment on attachment 370267 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=370267&action=review > > > > > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173 > > > +#if !ENABLE(REMOTE_INSPECTOR) && !PLATFORM(MAC) > > > > You're implementing this below. This is incorrect. > > > > > Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp:101 > > > +WebPageProxy* RemoteWebInspectorProxy::platformCreateFrontendPageAndWindow() > > > > Here's an implementation. > > It's not incorrect. RWIProxyMac is the only implementation that's not > conditioned on ENABLE(REMOTE_INSPECTOR). My bad. I misread it as an || like the old one.
Comment on attachment 370267 [details] Patch Clearing flags on attachment: 370267 Committed r245536: <https://trac.webkit.org/changeset/245536>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50960229>