We currently have in parameters named in_foo, and out-vars names o_in_foo instead of out_foo.
Created attachment 301194 [details] Proposed Fix
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`)
Comment on attachment 301194 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=301194&action=review > Source/JavaScriptCore/ChangeLog:13 > + The 'o_' prefix is probably supposed to mean 'ObjC'-versions of the in parameters, but looking > + at this code with fresh eyes, I was really confused by the prefix. Just use 'out_' like we do > + in the equivalent C++ generated code. But these are not out parameters, they are in parameters converted to their ObjectiveC type. Naming them "out" is confusing. Maybe you can convince me of the merit of this with an explanation!
Comment on attachment 301194 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=301194&action=review >> Source/JavaScriptCore/ChangeLog:13 >> + in the equivalent C++ generated code. > > But these are not out parameters, they are in parameters converted to their ObjectiveC type. Naming them "out" is confusing. Maybe you can convince me of the merit of this with an explanation! In both language bindings, the pattern is: - take some JSON ~in~ via parameters - convert and validate it (if applicable) - pass it ~out~ to the actual command For example, out_printColor = fromProtocolString<TestProtocolDatabaseExecuteAllOptionalParametersPrintColor>(*in_printColor); We should never use in_ variables when calling the command, only out_ variables. I'm open to other prefixes, but it's important to distinguish it from unconverted parameters of the same name.
> In both language bindings, the pattern is: > > - take some JSON ~in~ via parameters > - convert and validate it (if applicable) > - pass it ~out~ to the actual command I don't think that is correct. This is how I remember it: ~in~ values are command parameters, they are validated, and passed to the C++ command. ~out~ values are command return values, they are setup, and passed to the C++ command to fill in as needed. For synchronous commands, they are all passed to the C++ command. The in/out prefix just corresponds to whether they are "parameters" or "return" values. In the ObjC case, we force all commands to be async. So there are never any ~out~ prefixed names. Instead, each command gets a success callback block that the ObjC must call asynchronously. The parameters to that block contains all of the traditional "out" parameters.
> For example, > > out_printColor = > fromProtocolString<TestProtocolDatabaseExecuteAllOptionalParametersPrintColor > >(*in_printColor); Lets look at this deeper. The JSON is: "commands": [ { "name": "executeAllOptionalParameters", "parameters": [ ... { "name": "printColor", "type": "string", "enum": ["cyan", "magenta", "yellow", "black"], "optional": true } ], "returns": [ ... { "name": "printColor", "type": "string", "enum": ["cyan", "magenta", "yellow", "black"], "optional": true } ] }, In the generated ObjC we are taking the incoming printColor, converting it to an ObjC type (if needed), and sending it on to the ObjC handler: if (in_printColor) o_in_printColor = fromProtocolString<TestProtocolDatabaseExecuteAllOptionalParametersPrintColor>(*in_printColor); [m_delegate executeAllOptionalParametersWithErrorCallback:errorCallback successCallback:successCallback ... printColor:(in_printColor ? &o_in_printColor : nil)]; The out parameter of the same name here is part of the successCallback: id successCallback = ^(... TestProtocolDatabaseExecuteAllOptionalParametersPrintColor *printColor) { Ref<InspectorObject> resultObject = InspectorObject::create(); ... if (printColor) resultObject->setString(ASCIILiteral("printColor"), toProtocolString(*printColor)); backendDispatcher()->sendResponse(requestId, WTFMove(resultObject)); }; And here we don't need to prefix anything because we are guaranteed not to have any collisions. (Except "resultObject" which we could probably prefix).
Oh, and the C++ dispatch shows we ~in~ prefix the incoming parameter and ~out~ prefix the return value of the same name: void DatabaseBackendDispatcher::executeAllOptionalParameters(long requestId, RefPtr<InspectorObject>&& parameters) { ... bool opt_in_printColor_valueFound = false; String opt_in_printColor = m_backendDispatcher->getString(parameters.get(), ASCIILiteral("printColor"), &opt_in_printColor_valueFound); ... DatabaseBackendDispatcherHandler::PrintColor out_printColor; m_agent->executeAllOptionalParameters(error, ..., opt_in_printColor_valueFound ? &opt_in_printColor : nullptr, ..., &out_printColor); ... }
Can we close this?