[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 #25 from Christopher Reid <chris.reid at sony.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'm still working on an updated patch addressing all the feedback, just adding some input.

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

I agree they aren't completely generic, I was using that term since we do eventually want to also share these files with WinCario. I'll move them over to playstation for now.

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

I think it's currently needed for this case since the TargetListing type is used in shared code and glib/cocoa both have their own aliases to their own types.

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:137
>> +    std::lock_guard<Lock> lock(m_mutex);
> 
> Any reason not to use `LockHolder`?

Steph mentioned this above for an earlier patch, std::lock_guard is consistent with how that m_mutex is acquired in the shared RemoteInspector.cpp and cocoa/glib code. I agree LockHolder makes sense here but updating cocoa/glib seems a bit out of scope for this patch.

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

FWIU the `TargetInfo`/`TargetListing` objects are created and maintained without necessarily having a remote debugger connected yet. I believe this is the case so when a remote inspector does connect, only m_targetListingMap needs sending instead of having to query WebProcesses/UIProcess for inspectable targets.

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:155
>> +bool RemoteInspectorServer::start(const char* /* address */, unsigned port)
> 
> Drop `address` from the signature?

Oops, I forgot to do that for this patch.

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:100
>> +    Lock m_lock;
> 
> What is m_lock guarding specifically?

Yeah, it's meant to guard that m_connections object. I'll make the name clearer and ensure it's guarding all accesses.

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:44
>> +constexpr size_t SocketBufferSize = 65536;
> 
> How did you arrive at this value (64kb)? The vast majority of inspector messages will fit into that size. However there are some inspector messages that will certainly end up larger (for example image resources >64kb that have been base64 encoded). It may be useful to increase this if resources allow. Just something worth considering.

Our UIProcess is currently pretty memory limited so having smaller receiving buffers seems safer for now but it's something we should look at.

-- 
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/20190213/ad7a380d/attachment-0001.html>