Bug 197434 – [WinCairo] Implement Remote Web Inspector Client.

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

[Basuke Suzuki]

Add implementation of Remote Web Inspector client for WinCairo.

[Basuke Suzuki]

Created attachment 368716 [details]
PATCH

[Basuke Suzuki]

Created attachment 368718 [details]
Patch

[Ross Kirsling]

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?

[Basuke Suzuki]

Comment on attachment 368718 [details]
Patch

Sorry, I sent a wrong patch to this bug!

[Ross Kirsling]

Created attachment 370083 [details]
Patch

[Christopher Reid]

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.

[Ross Kirsling]

Created attachment 370131 [details]
Patch

[Ross Kirsling]

Created attachment 370258 [details]
Patch

[Don Olmstead]

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.

[Ross Kirsling]

Created attachment 370267 [details]
Patch

[Don Olmstead]

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.

[Ross Kirsling]

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

[Don Olmstead]

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

[WebKit Commit Bot]

Comment on attachment 370267 [details]
Patch

Clearing flags on attachment: 370267

Committed r245536: <https://trac.webkit.org/changeset/245536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50960229>