Summary: | Web Inspector: [Cocoa] `RemoteInspector` won't connect to a new relay if it hasn't yet failed to communicate with a previously connected relay | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||
Component: | Web Inspector | Assignee: | Patrick Angle <pangle> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, pangle, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 228303 | ||||||||||
Attachments: |
|
Description
Patrick Angle
2021-06-02 09:46:20 PDT
Created attachment 430392 [details]
Patch v1.0
Correct radar is <rdar://78237764> Comment on attachment 430392 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=430392&action=review r=me, nice fix with a great explanation :) > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:263 > + bool m_allowRetryRelayConnection { false }; NIT: How about something a bit more specific/explanatory like `m_shouldReconnectToRelayOnFailure`? > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:281 > + m_relayConnection->sendMessage(@"syn", nil); NIT: I wonder if maybe we should give this a different name so it shows up differently in logging. If we do keep the same name, perhaps we should create a static constant instead of hardcoding in both spots? > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:376 > + if (m_allowRetryRelayConnection) { Style: I'd make this into an early return > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:377 > + WTFLogAlways("RemoteInspector XPC connection to relay failed, reattempting connection..."); NIT: s/reattempting connection/reconnecting/ > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:380 > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ Should we maybe wait a second or two before attempting to reconnect? > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:384 > + m_allowRetryRelayConnection = false; NIT: I'd put this as the first thing in the `if` so that if someone adds an early return in the future it's less likely to be missed/forgotten Created attachment 430484 [details]
Patch v1.1 - Review nits
Created attachment 430491 [details]
Patch v1.1.1 - Add reviewer to changelog
Committed r278413 (238438@main): <https://commits.webkit.org/238438@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430491 [details]. |