Bug 217353 – [WinCairo][PlayStation] Add handling for accept failure case

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

When listening RemoteInspectorServer fails to accept new connection, the listening socket goes to invalid situation and need to be recreated from the scrach.

[Basuke Suzuki]

Created attachment 410695 [details]
PATCH

[Ross Kirsling]

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`?

[Basuke Suzuki]

(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!

[Alex Christensen]

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

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 412468 [details]
PATCH

[Alex Christensen]

Comment on attachment 412468 [details]
PATCH

I wish there were a way to make this more straightforward.

[EWS]

Committed r269128: <https://trac.webkit.org/changeset/269128>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412468 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70786493>

[Basuke Suzuki]

(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?

[Alex Christensen]

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.