Bug 231709 – [Cocoa] Web Inspector: handle Promise objects returned from evaluateScriptInExtensionTab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/84224179>

[Blaze Burg]

Created attachment 441164 [details]
Patch v1.0

[Patrick Angle]

Comment on attachment 441164 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:248
> +                // If `result`` is a promise, then it came from a different frame and `instanceof Promise` won't work.

NIT: Extra ` after `result`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:251
> +                    return result.then((resolvedValue) => resolve({result: resolvedValue}), (errorValue) => reject({error: errorValue}));

The `return` and the `result.then` should probably be separate, since returning a value here is a bit confusing.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:259
> +                    `Caught Exception: ${error.name}`,

Should we add `error.message` to the formatted message?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:260
> +                    `at ${error.sourceURL}, Line ${error.line}, Column ${error.column}:`,

I don't think sourceURL will exist for all errors. It doesn't appear to be part of the Error prototype.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:224
> +    // The promise will resolve to one of:
> +    // - an object with a 'result' key and value that is the result of the script evaluation.
> +    // - an object with an 'error' key and value in the case that an exception was thrown while evaluating script.

It's kinda of misleading IMO to say the promise will resolve with an `error` key and value, given that the implementation in the frontend appears to reject with an `error` when something goes wrong.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:246
> +    // The promise will resolve to one of:
> +    // - an object with a 'result' key and value that is the result of the script evaluation.
> +    // - an object with an 'error' key and value in the case that an exception was thrown while evaluating script.

Ditto :222-224

> Tools/ChangeLog:10
> +

NIT: Extra newline

[Blaze Burg]

Comment on attachment 441164 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:251
>> +                    return result.then((resolvedValue) => resolve({result: resolvedValue}), (errorValue) => reject({error: errorValue}));
> 
> The `return` and the `result.then` should probably be separate, since returning a value here is a bit confusing.

I don't follow. Returning the invocation of 'then' on a then-able object is an appropriate way to chain the two promises.

Chaining with .then does not mutate `result` in place, so the two line version would be:

let result = result.then(...)
return result;

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:259
>> +                    `Caught Exception: ${error.name}`,
> 
> Should we add `error.message` to the formatted message?

Oops.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:260
>> +                    `at ${error.sourceURL}, Line ${error.line}, Column ${error.column}:`,
> 
> I don't think sourceURL will exist for all errors. It doesn't appear to be part of the Error prototype.

OK

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:224
>> +    // - an object with an 'error' key and value in the case that an exception was thrown while evaluating script.
> 
> It's kinda of misleading IMO to say the promise will resolve with an `error` key and value, given that the implementation in the frontend appears to reject with an `error` when something goes wrong.

s/resolve/settle/

[Blaze Burg]

Created attachment 441222 [details]
Patch v1.1

[Blaze Burg]

Created attachment 441223 [details]
Patch v1.1.1 (better changelog)

[Blaze Burg]

(In reply to BJ Burg from comment #4)
> Comment on attachment 441164 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441164&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:251
> >> +                    return result.then((resolvedValue) => resolve({result: resolvedValue}), (errorValue) => reject({error: errorValue}));
> > 
> > The `return` and the `result.then` should probably be separate, since returning a value here is a bit confusing.
> 
> I don't follow. Returning the invocation of 'then' on a then-able object is
> an appropriate way to chain the two promises.
> 
> Chaining with .then does not mutate `result` in place, so the two line
> version would be:
> 
> let result = result.then(...)
> return result;

I chatted more with Patrick, and I forgot that this is an inner-outer promise situation, so the return isn't needed.

[Blaze Burg]

Created attachment 441398 [details]
[fast-cq] Patch v1.1.2 (for landing)

[EWS]

Committed r284264 (243072@main): <https://commits.webkit.org/243072@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441398 [details].