When listening RemoteInspectorServer fails to accept new connection, the listening socket goes to invalid situation and need to be recreated from the scrach.
Created attachment 410695 [details] PATCH
Comment on attachment 410695 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=410695&action=review Just some naming comments from me. > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h:50 > - void didClose(RemoteInspectorSocketEndpoint&, ConnectionID) final { }; > + void didStatusChanged(RemoteInspectorSocketEndpoint&, ConnectionID, RemoteInspectorSocketEndpoint::Listener::Status) final { }; This should be `didChangeStatus`. > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:168 > + bool isTimeToListen() I think `shouldListen` would be better. > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:176 > + MonotonicTime postponeRetryUntil { MonotonicTime::nan() }; How about something like `nextRetryTime`?
(In reply to Ross Kirsling from comment #2) > Comment on attachment 410695 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410695&action=review > > Just some naming comments from me. > > > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h:50 > > - void didClose(RemoteInspectorSocketEndpoint&, ConnectionID) final { }; > > + void didStatusChanged(RemoteInspectorSocketEndpoint&, ConnectionID, RemoteInspectorSocketEndpoint::Listener::Status) final { }; > > This should be `didChangeStatus`. > > > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:168 > > + bool isTimeToListen() > > I think `shouldListen` would be better. > > > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:176 > > + MonotonicTime postponeRetryUntil { MonotonicTime::nan() }; > > How about something like `nextRetryTime`? Thanks!
Comment on attachment 410695 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=410695&action=review > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp:113 > + MonotonicTime mostRecentWakeup = MonotonicTime::nan(); Let's not use nan as a sentinel. Please use Optional<MonotonicTime> instead. > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:55 > + enum class Status { : uint8_t
Comment on attachment 410695 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=410695&action=review Thanks for reviewing! >> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp:113 >> + MonotonicTime mostRecentWakeup = MonotonicTime::nan(); > > Let's not use nan as a sentinel. Please use Optional<MonotonicTime> instead. Ah, that's better. Thanks >> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:55 >> + enum class Status { > > : uint8_t Oh my. I don't know why but I misunderstand the behavior of enum class's underlying type decision. For unscoped enum, it is always int when it is unspecified, but for scoped, I believed it is calculated to be the minimum to hold the values. I might be confusing with other specification. thanks.
Created attachment 412468 [details] PATCH
Comment on attachment 412468 [details] PATCH I wish there were a way to make this more straightforward.
Committed r269128: <https://trac.webkit.org/changeset/269128> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412468 [details].
<rdar://problem/70786493>
(In reply to Alex Christensen from comment #7) > Comment on attachment 412468 [details] > PATCH > > I wish there were a way to make this more straightforward. Thanks for reviewing! Do you mean something like socket abstraction layer?
I'm not completely sure what I mean. I think there should be a better abstraction somewhere, but I'm not sure what would make this more straightforward.