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

[bugzilla-daemon at webkit.org]

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

--- Comment #22 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 361707
  --> https://bugs.webkit.org/attachment.cgi?id=361707
Inspector Server

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

I haven't had time to review the whole patch, so I'm adding only a few comments for now

> Source/JavaScriptCore/PlatformPlayStation.cmake:13
> +    inspector/remote/generic/RemoteInspectorServer.h

This depends on RemoteInspectorSocket which is under playstation directory, so I would move this there too for now, because it's not really generic.

> Source/JavaScriptCore/PlatformPlayStation.cmake:28
> +    inspector/remote/generic/RemoteInspectorGeneric.cpp
> +    inspector/remote/generic/RemoteInspectorServerGeneric.cpp

Ditto.

> Source/JavaScriptCore/SourcesGTK.txt:31
> -inspector/remote/glib/RemoteConnectionToTargetGlib.cpp
> +inspector/remote/generic/RemoteConnectionToTargetGeneric.cpp

I'm not a fan of the name 'generic', why not simply add RemoteConnectionToTarget.cpp if it's really generic? Apple ports can still use its own platform specific implementation in their platform specific file.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:72
> +class InspectorEvent {

This could probably be just a struct.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:165
> +    void setup(unsigned targetIdentifier);
> +    void sendMessageToTarget(unsigned targetIdentifier, const char* message);

You can split the GLIB ifdef to share the common signatures.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:222
> +    using RemoteInspectorCall = void (RemoteInspector::*)(const InspectorEvent&);

I would use MessageHandler or MethodHandler, or even CallHandler here instead of just Call

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:258
> +    static PlatformSocketType connectionIdentifier;

s_connectionIdentifier

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:90
> +    const CString& message = event.utf8();

String::utf8() doesn't return a const reference, but a new CString.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:98
> +    auto jsonData = String::adopt(WTFMove(data));

You are converting to utf8 before sending, I think you should convert back using String::fromUTF8() when receiving, no?

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:114
> +    InspectorEvent event;
> +    uint64_t connectionID;
> +    uint64_t targetID;
> +    String message;

Move these where they are first needed.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:117
> +        event.connectionID = Optional<uint64_t>(connectionID);

I don't think you need the explicit Optional<uint64_t>()

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:120
> +        event.targetID = Optional<uint64_t>(targetID);

Ditto.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:123
> +        event.message = Optional<String>(WTFMove(message));

Ditto.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:151
> +    m_socketConnection->setDidReceiveDataListener([this](ClientID, Vector<uint8_t>&& data) {
> +        didReceiveData(WTFMove(data));
> +    });
> +    m_socketConnection->setDidCloseListener([this](ClientID) {
> +        didClose();
> +    });

I would add a Client class that RemoteInspector class can derive from, received as argument by RemoteInspectorSocketClient constructor instead of using these listeners approach.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:187
> +    TargetInfo* info = new TargetInfo();

Use RefPtr directly here, it could be Ref<> instead of RefPtr

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:224
> +    auto targetListJSON = JSON::Array::create();
> +    for (auto listing : m_targetListingMap.values()) {
> +        auto target = JSON::Object::create();
> +        target->setInteger("targetID"_s, listing->targetIdentifier);
> +        target->setString("type"_s, listing->type);
> +        target->setString("name"_s, listing->name);
> +        target->setString("url"_s, listing->url);
> +        target->setBoolean("hasLocalDebugger"_s, listing->hasLocalDebugger);
> +        targetListJSON->pushObject(WTFMove(target));
> +    }

Why don't you use JSON::Array directly as TargetListing? I don't think we need the TargetInfo class is really needed.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:38
> +    if (m_server) {

Use an early return here.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:102
> +    const CString& message = event.utf8();

CString message

-- 
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/20190212/b933e3c4/attachment.html>