[webkit-reviews] review granted: [Bug 176936] Web Inspector: make recording swizzle async : [Attachment 320799] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 176936: Web Inspector: make recording swizzle async
https://bugs.webkit.org/show_bug.cgi?id=176936

Attachment 320799: Patch

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




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

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

r=me

> Source/WebCore/ChangeLog:8
> +	   Modified existing tests.  No change in functionality.

Two spaces after a period is so 90's.  See what I did there? That said I don't
find these kinds of lines particularly useful and can just be dropped.

> Source/WebInspectorUI/ChangeLog:10
> +	   necessary primarily for swizzling Image values, as it is not able to
be used (such as for

Replace "it" with "the Image"

> Source/WebInspectorUI/UserInterface/Models/Recording.js:231
> +		       var loadPromise = new WI.WrappedPromise;
> +		       function resolveWithImage(event) {
> +			   loadPromise.resolve(image);
> +		       }
> +		       image.addEventListener("load", resolveWithImage);
> +		       image.addEventListener("error", resolveWithImage);
> +
> +		       image.src = data;
> +
> +		       this._swizzle[index][type] = await loadPromise.promise;

Lets avoid WI.WrappedPromise here. It is only useful when someone else wants to
resolve the promise. Here everything is self contained and fits on a few lines:

    this._swizzle[index][type] = new Promise((resolve, reject) => {
	let image = new Image;
	let resolveWithImage = () => { resolve(image); }
	image.addEventListener("load", resolveWithImage);
	image.addEventListener("error", resolveWithImage);
	image.src = data;
    });
    break;

> Source/WebInspectorUI/UserInterface/Models/Recording.js:256
> +		       var image = await this.swizzle(data[0],
WI.Recording.Swizzle.Image);
> +		       var repeat = await this.swizzle(data[1],
WI.Recording.Swizzle.String);

While there is no issue right now, we have to be careful we don't fall into a
trap here.

If a future swizzle takes _multiple_ images / or simply has _multiple_ swizzles
which we expect to do work asynchronously then we should be sure to start both
operations before awaiting.

For example this:

    // serial
    var image1 = await this.swizzle(data[0], WI.Recording.Swizzle.Image);
    var image2 = await this.swizzle(data[1], WI.Recording.Swizzle.Image);

Will start image1 load => wait for image1 => start image2 load => wait for
image2. Effectively it loads both images in serial.

While this:

    // parallel
    var [image1, image2] = await Promise.all([
	this.swizzle(data[0], WI.Recording.Swizzle.Image),
	this.swizzle(data[1], WI.Recording.Swizzle.Image),
    ]);

Will start image1 load => start image 2 load => wait for both to complete.
Effectively it loads both images in parallel.

I searched around and I think may cover it:
https://ponyfoo.com/articles/understanding-javascript-async-await#fork-in-the-r
oad

---

So it might be worthwhile to switch to the parallel syntax now to avoid a
potential mishap in the future.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:120
> +	   this._name = await recording.swizzle(this._payloadName,
WI.Recording.Swizzle.String);

FWIW I think all of these are fine. Because where it really counts is parallel,
and where it doesn't really matter (None / String) its serial. And it reads
nicely.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:313
>		       case "fillStyle":
>		       case "strokeStyle":

This one may be a small concern since it is doing things serially instead of in
parallel, however maybe we can assume the swizzle here will be an immediate
result?