Summary: | [WinCairo][PlayStation] Add handling for accept failure case | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||
Component: | Platform | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, Basuke.Suzuki, chris.reid, don.olmstead, ews-watchlist, hi, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, saam, stephan.szabo, takashi.komori, tzagallo, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Basuke Suzuki
2020-10-05 16:46:34 PDT
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]. (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. |