Bug 199938 – [WinCairo] Start RemoteInspectorServer

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Fujii Hironori]

[WinCairo] Start RemoteInspectorServer

[Fujii Hironori]

Created attachment 374454 [details]
WIP patch

[Fujii Hironori]

Created attachment 374455 [details]
WIP patch

[Don Olmstead]

Comment on attachment 374455 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374455&action=review

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:154
> +#if ENABLE(REMOTE_INSPECTOR) && PLATFORM(WIN)

Shouldn’t this be USE(INSPECTOR_SOCKET_SERVER) not PLATFORM(WIN)?

[Basuke Suzuki]

Comment on attachment 374455 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374455&action=review

LGTM. Actually this bug is duplicate of mine :) https://bugs.webkit.org/show_bug.cgi?id=197435

> Source/WebKit/Shared/WebProcessCreationParameters.h:193
> +    uint16_t m_inspectorServerPort { 0 };

Should be inspectorServerPort in POD.

[Radar WebKit Bug Importer]

<rdar://problem/53323048>

[Fujii Hironori]

Comment on attachment 374455 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374455&action=review

>> Source/WebKit/Shared/WebProcessCreationParameters.cpp:154
>> +#if ENABLE(REMOTE_INSPECTOR) && PLATFORM(WIN)
> 
> Shouldn’t this be USE(INSPECTOR_SOCKET_SERVER) not PLATFORM(WIN)?

Umm, in Unix, it uses unix domain sockets instead of BSD sockets.

>> Source/WebKit/Shared/WebProcessCreationParameters.h:193
>> +    uint16_t m_inspectorServerPort { 0 };
> 
> Should be inspectorServerPort in POD.

Oops. Thanks.

[Fujii Hironori]

Created attachment 374586 [details]
WIP patch

[Fujii Hironori]

Created attachment 374587 [details]
WIP patch

[Fujii Hironori]

Created attachment 374668 [details]
WIP patch

[Christopher Reid]

Created attachment 375279 [details]
Updated WIP patch

Hey Fujii, what do you think about these changes?
I added some socket fixes to your patch to fix issues with the server disconnecting and replaced the target list page to do DOM manipulation for updating the target list table.
The windows remote inspector is much more stable with these changes.

[Fujii Hironori]

Comment on attachment 375279 [details]
Updated WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375279&action=review

Looks a great improvement. Good job!

The issue of racy initial empty target list should be filed as separate bug ticket.

> Source/WebKit/UIProcess/socket/RemoteInspectorClient.h:88
> +    URL m_url;

This is not used anymore.

> Source/WebKit/UIProcess/socket/RemoteInspectorProtocolHandler.cpp:100
> +    // Re-create the connection to avoid holding on to bad connections

If you re-create RemoteInspectorClient every time inspector:// is load, we don't need to manage RemoteInspectorClient instances by using a HashMap m_inspectorClients anymore.

> Source/WebKit/UIProcess/socket/RemoteInspectorProtocolHandler.cpp:165
> +        "}"

I think we should have just only one JS function.
       "function updateTargets(str) {"
            "let targetDiv = document.getElementById('targetlist');"
            "targetDiv.innerHTML = str"
        "}"

[Christopher Reid]

Created attachment 379488 [details]
Patch

The client should be working now that the weakptr assert issue was fixed in r249158.

[Christopher Reid]

(In reply to Fujii Hironori from comment #11)
> Comment on attachment 375279 [details]
> Updated WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375279&action=review
> 
> Looks a great improvement. Good job!
> 
> The issue of racy initial empty target list should be filed as separate bug
> ticket.

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

[Christopher Reid]

Created attachment 379491 [details]
Patch

Add changelog and fix style issues

[WebKit Commit Bot]

Comment on attachment 379491 [details]
Patch

Clearing flags on attachment: 379491

Committed r250328: <https://trac.webkit.org/changeset/250328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.