Bug 176936 – Web Inspector: make recording swizzle async

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

In the case that we need to swizzle a CanvasPattern, we need to wait for the required Image (first argument) to have loaded before we attempt to create the pattern.  The current swizzle logic is such that even though we use a dataURL, it is still possible for the Image to not have loaded by the time we want to create the CanvasPattern.  As such, we should make the entire the swizzle system async so that we can wait to create the CanvasPattern until the required Image has loaded.

[Devin Rousso]

Created attachment 320799 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 320799 [details]
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-road

---

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?

[Joseph Pecoraro]

>         let resolveWithImage = () => { resolve(image); }

Obviously with a semicolon at the end. Never just copy and paste code from me =P

[Matt Baker]

Comment on attachment 320799 [details]
Patch

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

>> 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.

As in the 1890s!

With the advent of the computer age, typographers began deprecating double spacing, even in monospaced text. In 1989, Desktop Publishing by Design stated that "typesetting requires only one space after periods, question marks, exclamation points, and colons", and identified single sentence spacing as a typographic convention.

- <https://en.wikipedia.org/wiki/Sentence_spacing>

[Devin Rousso]

Comment on attachment 320799 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:313
>>                      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?

Everything that doesn't involve an Image should be an immediate result.  Also, this will only happen once per recording (initial state), and its a fairly small amount of data, so I think it's fine.

[Devin Rousso]

Created attachment 320825 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320825 [details]
Patch

Clearing flags on attachment: 320825

Committed r222057: <http://trac.webkit.org/changeset/222057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693338>