[Webkit-unassigned] [Bug 193806] [PlayStation] Upstream playstation's remote inspector server

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193806

--- Comment #28 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 362388
  --> https://bugs.webkit.org/attachment.cgi?id=362388
Inspector Server

View in context: https://bugs.webkit.org/attachment.cgi?id=362388&action=review

This looks good to me. One remaining threading issue you may want to address.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorConnectionClient.h:44
> +    struct InspectorEvent {

I suppose this could just be named `Event` and not `InspectorEvent` given this is inside RemoteInspectorConnectionClient.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorConnectionClient.h:51
> +
> +

Style: Remove accidental empty line.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorMessageParser.h:39
> +    /*
> +    | <--- one message for send / didReceiveData ---> |
> +    +--------------+----------------------------------+--------------
> +    |    size      |               data               | (next message)
> +    |    4byte     |          variable length         |
> +    +--------------+----------------------------------+--------------
> +                   | <------------ size ------------> |
> +    */

This is already in the implementation file. It is probably only needed in one place.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorMessageParser.h:48
> +    void setDidParseMessageListener(Function<void(ClientID, Vector<uint8_t>)>&& listener) { m_didParseMessageListener =
> +WTFMove(listener); }

Style: Remove accidental newline

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorMessageParser.h:54
> +    bool parse();
> +    Function<void(ClientID, Vector<uint8_t>&&)> m_didParseMessageListener;

Style: Add a newline between the methods and members.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorServerPlayStation.cpp:56
> +        m_inspectorConnections.append(clientID);

`m_inspectorConnections` still looks like it has multi-threaded mutation / access issues. There are functions with ASSERT(isMainThread()) and ASSERT(!isMainThread()) that mutate or access this map without a lock. This is very likely to be rare, especially if there is only 1 client in the Inet case, but given it is possible we should probably address that now if we can.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:71
> +    struct connection {

Style: Normally struct/class names are capitalized. I'd have expected `struct Connection` here.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:79
> +    Lock m_connectionsLock;
> +    HashMap<ClientID, std::unique_ptr<struct connection>> m_connections;

Nice! This looks cleaner, I don't see any threading issues here now.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:194
> +        if (m_inspectorClient)
> +            m_inspectorClient->didClose(clientID);

Nit: You could call the client without the lock.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:271
> +        if (newID && m_inspectorClient)
> +            m_inspectorClient->didAccept(newID.value(), DomainType::Inet);

Nit: You could call the client without the lock.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190219/fbceefb1/attachment-0001.html>