Bug 202421 – Socket RWI client should acquire backend commands from server

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Socket RWI client should acquire backend commands from server

[Ross Kirsling]

Created attachment 379935 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Basuke Suzuki]

*** Bug 199016 has been marked as a duplicate of this bug. ***

[Basuke Suzuki]

Comment on attachment 379935 [details]
Patch

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

This is informal review.

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:134
> +bool RemoteInspectorServer::start(const char* address, uint16_t port, String&& backendCommandsPath)

PlayStation port doesn't require this argument. I think it is better to add another method to set m_backendCommandsPath. It shouldn't be too late if the path is set just after start().

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h:43
> +    JS_EXPORT_PRIVATE bool start(const char* address, uint16_t port, String&& backendCommandsPath = String());

Ditto.

> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:68
> +    Inspector::RemoteInspectorServer::singleton().start(host.utf8().data(), port.value(), backendCommandsPath());

Ditto

[Devin Rousso]

Comment on attachment 379935 [details]
Patch

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

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:268
> +    if (!m_backendCommandsPath)

Should this be `isEmpty()`?

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:269
> +        return String();

You can use `{ }` instead.

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:273
> +        return String();

Ditto

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:278
> +        return String();

Ditto

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:283
> +    FileSystem::readFromFile(handle, buffer.data(), size);

Should we be early-returning with a `String()` if this returns `-1`?

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h:43
> +    JS_EXPORT_PRIVATE bool start(const char* address, uint16_t port, String&& backendCommandsPath = String());

I don't think we should have a fallback for this.  I'd expect any callers to provide a `backendCommandsPath` (which is what it looks like is done now already).  Are there other callers of this that don't provide a `backendCommandsPath`?

> Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:158
> +class CppBackendCommandsGenerator(JSBackendCommandsGenerator):

I think we'd prefer if you moved this into a separate file 'generate_cpp_backend_commands.py'.

> Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:166
> +        generators.append(CppBackendCommandsGenerator(*generator_arguments))

Please include this for "Frameworks.Test" too.  We should also probably have some sort of testing for this, especially since you do some character replacements for the C++ string.

> Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:191
> +void RemoteInspectorClient::setBackendCommands(const Event& event)

I think this signature is missing from the header 'RemoateInspectorClient.h'.

> Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:196
> +    m_backendCommandsURL = makeString("data:text/javascript;base64,", *event.message);

Style: I think we prefer using `.value()` for `Optional` instead of `*`.

> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:45
> +        return String();

Ditto

> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:49
> +        return String();

Ditto

> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:51
> +    return String(path);

Do you need to explicitly construct the `String`, or can it be implicit?

[Ross Kirsling]

(In reply to Devin Rousso from comment #5)
> > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h:43
> > +    JS_EXPORT_PRIVATE bool start(const char* address, uint16_t port, String&& backendCommandsPath = String());
> 
> I don't think we should have a fallback for this.  I'd expect any callers to
> provide a `backendCommandsPath` (which is what it looks like is done now
> already).  Are there other callers of this that don't provide a
> `backendCommandsPath`?

It's just PS that uses the C++ copy instead of the path to the JS resource, so yeah, I'll split that out as Basuke also suggested.

> > Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:166
> > +        generators.append(CppBackendCommandsGenerator(*generator_arguments))
> 
> Please include this for "Frameworks.Test" too.  We should also probably have
> some sort of testing for this, especially since you do some character
> replacements for the C++ string.

Er, now that you mention it, we shouldn't be doing these string replaces at all, since C++11 has raw strings.

> > Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:191
> > +void RemoteInspectorClient::setBackendCommands(const Event& event)
> 
> I think this signature is missing from the header 'RemoateInspectorClient.h'.

Oddly enough, it's already there and just not implemented.

[Ross Kirsling]

Created attachment 379964 [details]
Patch

[Ross Kirsling]

Comment on attachment 379964 [details]
Patch

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

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:265
> +    return s_inspectorBackendCommandsString ? s_inspectorBackendCommandsString : String();

Style note: Apparently initializer lists aren't syntactically valid inside ternary branches. :(

[Ross Kirsling]

Created attachment 379976 [details]
Patch

[Ross Kirsling]

Created attachment 379978 [details]
Patch

[Ross Kirsling]

Created attachment 379979 [details]
Patch

[Ross Kirsling]

Sigh...no matter how many files I add, it doesn't please iOS.

I'm sure that in itself is fixable, but this all feels like a ton of Xcode wiring for something that we have no particular expectation of ever using outside of PlayStation port.

[Ross Kirsling]

Created attachment 380054 [details]
Patch

[Ross Kirsling]

Here's the patch without any special support for PlayStation port -- we can deal with that stuff at a later date.

[Devin Rousso]

Comment on attachment 380054 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.cpp:263
> +    auto handle = FileSystem::openFile(m_backendCommandsPath, FileSystem::FileOpenMode::Read);

NIT: I would reorganize this function a bit so you can better ensure that `FileSystem::closeFile` gets called:
```
    if (m_backendCommandsPath.isEmpty())
        return { };

    auto handle = FileSystem::openFile(m_backendCommandsPath, FileSystem::FileOpenMode::Read);
    if (!FileSystem::isHandleValid(handle))
        return { };

    String result;
    long long size;
    if (FileSystem::getFileSize(handle, size)) {
        Vector<char> buffer(size);
        auto bytesRead = FileSystem::readFromFile(handle, buffer.data(), size);
        result = String::adopt(WTFMove(buffer));
    }
    FileSystem::closeFile(handle);
    return result;
```

[Ross Kirsling]

Unfortunately it doesn't seem like String::adopt can handle a Vector of char, which is the type used by FileSystem::readFromFile.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #16)
> Unfortunately it doesn't seem like String::adopt can handle a Vector of
> char, which is the type used by FileSystem::readFromFile.

(Nevermind, I can reinterpret_cast from LChar...)

[Ross Kirsling]

Created attachment 380159 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 380159 [details]
Patch for landing

Clearing flags on attachment: 380159

Committed r250679: <https://trac.webkit.org/changeset/250679>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55961622>