Bug 226539 – Web Inspector: [Cocoa] `RemoteInspector` won't connect to a new relay if it hasn't yet failed to communicate with a previously connected relay

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

<rdar://78237764>

[Radar WebKit Bug Importer]

<rdar://problem/78768633>

[Patrick Angle]

Created attachment 430392 [details]
Patch v1.0

[Patrick Angle]

Correct radar is <rdar://78237764>

[Ryan Haddad]

<rdar://78237764>

[Devin Rousso]

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

[Patrick Angle]

Created attachment 430484 [details]
Patch v1.1 - Review nits

[Patrick Angle]

Created attachment 430491 [details]
Patch v1.1.1 - Add reviewer to changelog

[EWS]

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