Protocol changes: Runtime.evaluate and Runtime.callFunctionOn both get an awaitPromise option, which will `await` the result. Frontend Change: Results of level await now appear as any other console evaluation, instead of coming in separately through console.info. This means they can be used via $_, $1, etc. when paused, top level awaits now return their promise. This looks related to: https://bugs.webkit.org/show_bug.cgi?id=174006 I came across this while working with the protocol, and finding `Runtime.awaitPromise` to be inefficient and insufficient for my use case. I'm happy to land just the protocol changes, if the frontend changes are controversial. But they seemed to mostly come for free.
Created attachment 387140 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 387140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387140&action=review > Source/JavaScriptCore/inspector/InjectedScriptSource.js:186 > + callback("Could not find object with given id"); > + return; Don't you want to reject the promise here? > Source/JavaScriptCore/inspector/InjectedScriptSource.js:197 > + callback(String(e)); > + return; And here?
Comment on attachment 387140 [details] 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.