Bug 205958

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.