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

Tue Feb 12 14:55:07 PST 2019

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

--- Comment #23 from Devin Rousso <drousso at apple.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

>> Source/JavaScriptCore/SourcesGTK.txt:31
>> +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.

I agree.  Our usual style is to have the "generic"/"common"/"base" class in the "top-level" folder (which is inspector/remote/ in this case), and have any platform specific code in subfolders named after that platform (e.g. inspector/remote/PlayStation/).

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:70
> +using TargetListing = RefPtr<TargetInfo>;

I personally don't like these types of "simple" aliases, as it can make code harder to follow (unless you have a smarter IDE).

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

Inspector's generated backend dispatcher code uses `CallHandler`.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:223
> +    HashMap<String, RemoteInspectorCall>& methodTable();

Inspector's generated backend dispatcher code calls this the `dispatchMap`.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:91
> +    m_socketConnection->send(*m_clientID, reinterpret_cast<const uint8_t*>(message.data()), message.length());

When using `Optional`s, I prefer calling `.value()` over `*` to retrieve the value.  This makes it clearer that the value is an optional, not a pointer.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:137
> +    std::lock_guard<Lock> lock(m_mutex);

Any reason not to use `LockHolder`?

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:187
>> +    TargetInfo* info = new TargetInfo();
> 
> Use RefPtr directly here, it could be Ref<> instead of RefPtr

Many classes have a `::create` that follows this pattern.  I'd recommend copying that (specifically with a `Ref`).

    adoptRef(*new TargetInfo())

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:191
> +    info->hasLocalDebugger = target.hasLocalDebugger();

Just to make sure, this means that if `hasLocalDebugger` is false, but we have `TargetInfo`, then there must be a remote debugger?

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:243
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:259
> +    std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:274
> +    std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:283
> +        setup(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:295
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:296
> +        connectionToTarget = m_targetConnectionMap.get(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:301
> +    connectionToTarget->sendMessageToTarget(*event.message);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:313
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:314
> +        RemoteControllableTarget* target = m_targetMap.get(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:318
> +        connectionToTarget = m_targetConnectionMap.take(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:330
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:343
> +    std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:190
> +    targetListEvent->setString("message"_s, *event.message);