Bug 176936

Created attachment 320799 [details] Patch

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?

> let resolveWithImage = () => { resolve(image); } Obviously with a semicolon at the end. Never just copy and paste code from me =P

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>

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.

Created attachment 320825 [details] Patch

Comment on attachment 320825 [details] Patch Clearing flags on attachment: 320825 Committed r222057: <http://trac.webkit.org/changeset/222057>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34693338>