[Webkit-unassigned] [Bug 193806] [PlayStaton] Upstream playstation's remote inspector server
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
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);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:212
> + m_inspectionTargets.add(std::make_pair(*event.connectionID, *event.targetID));
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:216
> + setupEvent->setInteger("targetID"_s, *event.targetID);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:236
> + sendCloseEvent(*event.connectionID, *event.targetID);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:237
> + m_inspectionTargets.remove(std::make_pair(*event.connectionID, *event.targetID));
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:261
> + sendWebInspectorEvent(*m_clientConnection, closedEvent->toJSONString());
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:274
> + sendEvent->setInteger("targetID"_s, *event.targetID);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:275
> + sendEvent->setString("message"_s, *event.message);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:276
> + sendWebInspectorEvent(*event.connectionID, sendEvent->toJSONString());
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:292
> + sendEvent->setInteger("targetID"_s, *event.targetID);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:294
> + sendEvent->setString("message"_s, *event.message);
Ditto (>RemoteInspectorGeneric.cpp:91).
> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:295
> + sendWebInspectorEvent(*m_clientConnection, sendEvent->toJSONString());
Ditto (>RemoteInspectorGeneric.cpp:91).
>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:275
>> + return writeSize;
>
> And this `return false`.
It's possible that `writeSize` is not equal to `0` at this point, so this one might be valid.
--
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/44e2c79f/attachment-0001.html>
More information about the webkit-unassigned
mailing list