[webkit-reviews] review granted: [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 : [Attachment 430392] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
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
https://bugs.webkit.org/show_bug.cgi?id=226539

Attachment 430392: Patch v1.0

https://bugs.webkit.org/attachment.cgi?id=430392&action=review




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 430392
  --> https://bugs.webkit.org/attachment.cgi?id=430392
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