[Webkit-unassigned] [Bug 205958] Web Inspector: awaitPromise option in Runtime.evaluate and Runtime.callFunctionOn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=205958

Devin Rousso <drousso at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #387140|review?                     |review-
              Flags|                            |

--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 387140
  --> https://bugs.webkit.org/attachment.cgi?id=387140
Patch

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

r- due to build failures

Can you explain more what this looks like in the UI?

What happens if I `await new Promise(() => {});`?

What about if I evaluate each of the following separately, but in rapid succession:
```
console.log("before");
await new Promise((resolve, reject) => { setTimeout(resolve, 10 * 1000, "resolve"); });
console.log("after");
```
Would I see "before resolve after" or "before after resolve" in the Console?

(In reply to Joel Einbinder from comment #0)
> I came across this while working with the protocol, and finding `Runtime.awaitPromise` to be inefficient and insufficient for my use case.
How is it insufficient?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:175
> +        const awaitPromise = false;
> +        let val;
> +        const callback = x => val = x;
> +        this._evaluateAndWrap(callFrame.evaluateWithScopeExtension, callFrame, expression, objectGroup, isEvalOnCallFrame, includeCommandLineAPI, returnByValue, generatePreview, saveResult, awaitPromise, callback);
> +        return val;

This is pretty awful.  Why not leave `_evaluateAndWrap` alone and introduce an `_evaluateAndWrapAsync` that the `awaitPromise` functions can call instead?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:208
> +            let value = func.apply(object, resolvedArgs);

Style: missing newline before

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:210
> +                value = await value;

What about if the `Promise `throws?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:211
> +            callback({

Ditto (207)

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:546
> +        }
> +        else {

Style: the `else` should be on the same line as the `}`

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:580
> +                result: RemoteObject.create(await func(), objectGroup, returnByValue, generatePreview)

Ditto (210)

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:122
> +void InspectorRuntimeAgent::evaluate(const String& expression, const String* objectGroup, const bool* includeCommandLineAPI, const bool* doNotPauseOnExceptionsAndMuteConsole, const int* executionContextId, const bool* returnByValue, const bool* generatePreview, const bool* saveResult, const bool * awaitPromise, const bool* /* emulateUserGesture */, Ref<EvaluateCallback>&& callback)

Style: no space between `bool*`

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:124
> +    ErrorString errorString;

This isn't needed.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:127
> +        callback->sendFailure(errorString);

This should really be `callback->sendFailure("Missing injected script"_s);`

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:139
> +    RefPtr<Protocol::Runtime::RemoteObject> result;
> +    Optional<bool> wasThrown;
> +    Optional<int> savedResultIndex;

Ditto (124)

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:189
> +    RefPtr<Protocol::Runtime::RemoteObject> result;
> +    Optional<bool> wasThrown;
> +    Optional<int> savedResultIndex;

Ditto (124)

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:191
> +        ASSERT(!savedResultIndex);

This should either be `ASSERT_UNUSED(savedResultIndex, !savedResultIndex)` or make the parameter `Optional<int>& /* savedResultIndex */`.

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:181
> +void PageRuntimeAgent::evaluate(const String& expression, const String* objectGroup, const bool* includeCommandLineAPI, const bool* doNotPauseOnExceptionsAndMuteConsole, const int* executionContextId, const bool* returnByValue, const bool* generatePreview, const bool* saveResult, const bool * awaitPromise, const bool* emulateUserGesture, Ref<EvaluateCallback>&& callback)

Ditto (InspectorRuntimeAgent.cpp:122)

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.h:60
> +    void callFunctionOn(const String& objectId, const String& expression, const JSON::Array* optionalArguments, const bool* doNotPauseOnExceptionsAndMuteConsole, const bool* returnByValue, const bool* generatePreview, const bool * awaitPromise, const bool* emulateUserGesture, Ref<CallFunctionOnCallback>&&) override;

Ditto (InspectorRuntimeAgent.cpp:122)

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:116
> +        let awaitPromise = false;

Style: this should have a newline before it

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:125
> +            let transformedExpression = this._tryApplyAwaitConvenience(expression);

NIT: I think `asyncExpression` is a better name.

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:270
> +        if (this._supportsEvaluateAwaitPromise()) {

Style: missing newline before

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:275
> +(async function() {
> +    return ${originalExpression};
> +})();`;

Why not just have this on one line?
```
    if (anonymous)
        return `(async function() { return ${originalExpression}; })();`;
```

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:277
> +            return `${declarationKind} ${variableName};

Ditto (275)
```
    return `${declarationKind} ${variableName}; (async function() { ${variableName} = ${awaitPortion}; })();`;
```

> LayoutTests/inspector/runtime/callFunctionOn-awaitPromise.html:11
> +        name: "CallFunctionOnAndDontAwait",

Style: we normally prefix each test case's name with the name of the suite, so "Runtime.callFunctionOn.awaitPromise.False".

> LayoutTests/inspector/runtime/callFunctionOn-awaitPromise.html:16
> +            let {result: remoteObject} = await RuntimeAgent.callFunctionOn.invoke({objectId, functionDeclaration: `function() { return Promise.resolve(123); }`, awaitPromise: false});

Please don't include parameters where the value is falsy, unless the default behavior is truthy.

> LayoutTests/inspector/runtime/callFunctionOn-awaitPromise.html:17
> +            InspectorTest.expectEqual(remoteObject.className, 'Promise', "The className should be Promise");

This is not a good message for the test expectation.  It doesn't really clarify what you're testing, or more importantly why you're testing for it.  How about "Should return a Promise without awaitPromise."?
Style: we use double quotes, not single quotes.

-- 
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/20200109/9cff93cc/attachment-0001.htm>