Socket RWI client should acquire backend commands from server
Created attachment 379935 [details] Patch
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`)
*** Bug 199016 has been marked as a duplicate of this bug. ***
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
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?
(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.
Created attachment 379964 [details] Patch
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. :(
Created attachment 379976 [details] Patch
Created attachment 379978 [details] Patch
Created attachment 379979 [details] Patch
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.
Created attachment 380054 [details] Patch
Here's the patch without any special support for PlayStation port -- we can deal with that stuff at a later date.
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; ```
Unfortunately it doesn't seem like String::adopt can handle a Vector of char, which is the type used by FileSystem::readFromFile.
(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...)
Created attachment 380159 [details] Patch for landing
Comment on attachment 380159 [details] Patch for landing Clearing flags on attachment: 380159 Committed r250679: <https://trac.webkit.org/changeset/250679>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55961622>