Bug 199938

Summary: [WinCairo] Start RemoteInspectorServer
Product: WebKit Reporter: Fujii Hironori <fujii.hironori>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: basuke, chris.reid, commit-queue, don.olmstead, ews-watchlist, hi, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 197029    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
Updated WIP patch
none
Patch
none
Patch none

Fujii Hironori
Reported 2019-07-19 03:58:21 PDT
[WinCairo] Start RemoteInspectorServer
Attachments
WIP patch (6.92 KB, patch)
2019-07-19 03:58 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
WIP patch (6.48 KB, patch)
2019-07-19 05:06 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
WIP patch (6.84 KB, patch)
2019-07-21 20:17 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
WIP patch (3.34 KB, patch)
2019-07-21 21:16 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
WIP patch (9.39 KB, patch)
2019-07-22 20:15 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Updated WIP patch (16.31 KB, patch)
2019-07-31 19:12 PDT, Christopher Reid 		no flags
DetailsFormatted DiffDiff
Patch (13.03 KB, patch)
2019-09-24 13:53 PDT, Christopher Reid 		no flags
DetailsFormatted DiffDiff
Patch (15.03 KB, patch)
2019-09-24 14:21 PDT, Christopher Reid 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Fujii Hironori
Comment 1 2019-07-19 03:58:48 PDT
Created attachment 374454 [details] WIP patch
Fujii Hironori
Comment 2 2019-07-19 05:06:20 PDT
Created attachment 374455 [details] WIP patch
Don Olmstead
Comment 3 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)?
Basuke Suzuki
Comment 4 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.
Radar WebKit Bug Importer
Comment 5 2019-07-19 14:11:39 PDT
<rdar://problem/53323048>
Fujii Hironori
Comment 6 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.
Fujii Hironori
Comment 7 2019-07-21 20:17:46 PDT
Created attachment 374586 [details] WIP patch
Fujii Hironori
Comment 8 2019-07-21 21:16:29 PDT
Created attachment 374587 [details] WIP patch
Fujii Hironori
Comment 9 2019-07-22 20:15:15 PDT
Created attachment 374668 [details] WIP patch
Christopher Reid
Comment 10 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.
Fujii Hironori
Comment 11 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" "}"
Christopher Reid
Comment 12 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.
Christopher Reid
Comment 13 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
Christopher Reid
Comment 14 2019-09-24 14:21:02 PDT
Created attachment 379491 [details] Patch Add changelog and fix style issues
WebKit Commit Bot
Comment 15 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>
WebKit Commit Bot
Comment 16 2019-09-24 18:46:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.