[webkit-reviews] review granted: [Bug 192171] Web Inspector: Audit: tests should support async operations : [Attachment 356286] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 192171: Web Inspector: Audit: tests should support async operations
https://bugs.webkit.org/show_bug.cgi?id=192171

Attachment 356286: Patch

https://bugs.webkit.org/attachment.cgi?id=356286&action=review




--- Comment #37 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 356286
  --> https://bugs.webkit.org/attachment.cgi?id=356286
Patch

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

r=me

> Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:138
> +	   // Since `callback` is moved above, we can't call it if there's an
execption while trying to

Typo: "execption"

> Source/JavaScriptCore/inspector/protocol/Runtime.json:230
> +	       "description": "Evaluates promise on global object.",

Might use a better description. This calls the callback when the given objectId
(which must be a Promise) resolves / rejects.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:239
> +	       let response = null;
> +
> +	       metadata.startTimestamp = new Date;
> +	       response = await
RuntimeAgent.evaluate.invoke(evaluateArguments);

Merge the `let response = ...` here?

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:244
> +		       metadata.asyncTimestamp = metadata.endTimestamp;

Note to Devin: Add `asyncTimestamp` to `fromPayload` (so export/import uses
it).

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:249
> +		       addError(WI.UIString("Async audits are not supported on
this device."));

Probably better to just say "are not support". The "on this device" thing is a
little weird.

> LayoutTests/inspector/runtime/awaitPromise.html:49
> +	   let expression = `new Promise((resolve, reject) =>
setTimeout(reject, 100, ${JSON.stringify(value)}))`;

If this is triggering onunhandledrejection console messages we should provide a
way in tests to mute those so that this doesn't end up being flakey.

> LayoutTests/inspector/runtime/awaitPromise.html:77
> +    addRejectTest("Runtime.awaitPromise.Reject.Object", {a: 1, b: 2});

I'd also want to see a Promise chain.

    • Promise (1) that resolves to
	-> Promise (2) that resolves to
	    -> Promise (3) that resolves to
		-> a value

Should end up with the final value and not object (2).