Bug 168143 - [Cocoa] Web Inspector: clean up names of out-vars in generated ObjC backend dispatcher code
Summary: [Cocoa] Web Inspector: clean up names of out-vars in generated ObjC backend d...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-10 12:38 PST by BJ Burg
Modified: 2017-03-23 13:33 PDT (History)
8 users (show)

See Also:


Attachments
Proposed Fix (15.67 KB, patch)
2017-02-10 12:43 PST, BJ Burg
joepeck: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-02-10 12:38:51 PST
We currently have in parameters named in_foo, and out-vars names o_in_foo instead of out_foo.
Comment 1 BJ Burg 2017-02-10 12:43:38 PST
Created attachment 301194 [details]
Proposed Fix
Comment 2 WebKit Commit Bot 2017-02-10 12:45:41 PST
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 3 Joseph Pecoraro 2017-02-10 15:21:32 PST
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 4 BJ Burg 2017-02-10 15:47:08 PST
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.
Comment 5 Joseph Pecoraro 2017-02-10 15:54:36 PST
> 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.
Comment 6 Joseph Pecoraro 2017-02-10 16:00:04 PST
> 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).
Comment 7 Joseph Pecoraro 2017-02-10 16:03:23 PST
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);
        ...
    }
Comment 8 Joseph Pecoraro 2017-03-23 13:27:34 PDT
Can we close this?