Bug 199938 - [WinCairo] Start RemoteInspectorServer
Summary: [WinCairo] Start RemoteInspectorServer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 197029
  Show dependency treegraph
 
Reported: 2019-07-19 03:58 PDT by Fujii Hironori
Modified: 2019-09-24 18:46 PDT (History)
14 users (show)

See Also:


Attachments
WIP patch (6.92 KB, patch)
2019-07-19 03:58 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (6.48 KB, patch)
2019-07-19 05:06 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (6.84 KB, patch)
2019-07-21 20:17 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (3.34 KB, patch)
2019-07-21 21:16 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (9.39 KB, patch)
2019-07-22 20:15 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Updated WIP patch (16.31 KB, patch)
2019-07-31 19:12 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (13.03 KB, patch)
2019-09-24 13:53 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (15.03 KB, patch)
2019-09-24 14:21 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-07-19 03:58:21 PDT
[WinCairo] Start RemoteInspectorServer
Comment 1 Fujii Hironori 2019-07-19 03:58:48 PDT
Created attachment 374454 [details]
WIP patch
Comment 2 Fujii Hironori 2019-07-19 05:06:20 PDT
Created attachment 374455 [details]
WIP patch
Comment 3 Don Olmstead 2019-07-19 09:21:16 PDT
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)?
Comment 4 Basuke Suzuki 2019-07-19 14:11:24 PDT
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.
Comment 5 Radar WebKit Bug Importer 2019-07-19 14:11:39 PDT
<rdar://problem/53323048>
Comment 6 Fujii Hironori 2019-07-21 20:17:12 PDT
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.
Comment 7 Fujii Hironori 2019-07-21 20:17:46 PDT
Created attachment 374586 [details]
WIP patch
Comment 8 Fujii Hironori 2019-07-21 21:16:29 PDT
Created attachment 374587 [details]
WIP patch
Comment 9 Fujii Hironori 2019-07-22 20:15:15 PDT
Created attachment 374668 [details]
WIP patch
Comment 10 Christopher Reid 2019-07-31 19:12:52 PDT
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.
Comment 11 Fujii Hironori 2019-07-31 20:56:50 PDT
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"
        "}"
Comment 12 Christopher Reid 2019-09-24 13:53:03 PDT
Created attachment 379488 [details]
Patch

The client should be working now that the weakptr assert issue was fixed in r249158.
Comment 13 Christopher Reid 2019-09-24 13:58:23 PDT
(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
Comment 14 Christopher Reid 2019-09-24 14:21:02 PDT
Created attachment 379491 [details]
Patch

Add changelog and fix style issues
Comment 15 WebKit Commit Bot 2019-09-24 18:46:53 PDT
Comment on attachment 379491 [details]
Patch

Clearing flags on attachment: 379491

Committed r250328: <https://trac.webkit.org/changeset/250328>
Comment 16 WebKit Commit Bot 2019-09-24 18:46:55 PDT
All reviewed patches have been landed.  Closing bug.