[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 #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 360938
  --> https://bugs.webkit.org/attachment.cgi?id=360938
[WIP] Inspector Server

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

I did a quick once over and this looks good. I didn't dig into any of the threading complexity, but everything seems good! Let me know when this needs a more complete review.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:188
> +    info->type = (target.type() == RemoteInspectionTarget::Type::Web)? "Web" : "JavaScript";

Nit: "Web"_s and "JavaScript"_s to avoid unnecessary string copies.

Technically there are 3 types you may want to support:

    RemoteInspectionTarget::Type::JavaScript
    RemoteInspectionTarget::Type::ServiceWorker
    RemoteInspectionTarget::Type::Web

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:156
> +bool RemoteInspectorServer::start(const char* /* address */, unsigned port)

Address is never used, and probably not something you want anyways. Seems like it can be removed.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:51
> +    +--------------+----------------------------------+--------------
> +    |    size      |               data               | (next message)
> +    |    4byte     |          variable length         |
> +    +--------------+----------------------------------+--------------

Given you are expecting 4 bytes, you may want to use int32_t or uint32_t below. The Cocoa implementations which communicate over TCP (which has a similar ASCII art!) uses a uint32_t. Of course in practice messages don't really come close to that full size.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:419
> +    saServer.sin_family = AF_INET;

So you want to also support IPv6 (AF_INET6)?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:454
> +Vector<uint8_t> MessageParser::createMessage(const void* data, size_t size)

Given you are interpreting the data to `const uint8_t*`, why not specify that in the signatures (MessageParser::createMessage, RemoteInspectorSocket::send). I think that is a more common type for arbitrary data.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:464
> +    auto messageBuffer = Vector<uint8_t>(size + sizeof(int));
> +    int intSize = static_cast<int>(size);
> +    //messageBuffer.append(&intSize, sizeof(int));
> +    //messageBuffer.append(reinterpret_cast<const uint8_t*>(data), intSize);
> +    memcpy(&messageBuffer[0], &intSize, sizeof(int));
> +    memcpy(&messageBuffer[sizeof(int)], data, intSize);

These `int` could be `int32_t`

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:488
> +        if (m_buffer.size() < sizeof(int)) {

Likewise everywhere in here.

-- 
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/20190208/8f7e5c59/attachment.html>