Bug 229061 - Web Inspector: refactor `WI.ImageUtilities.scratchCanvasContext2D` to not use a callback
Summary: Web Inspector: refactor `WI.ImageUtilities.scratchCanvasContext2D` to not use...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-12 17:22 PDT by Devin Rousso
Modified: 2022-03-28 09:14 PDT (History)
5 users (show)

See Also:

Attachments
Patch (7.03 KB, patch)
2021-08-12 17:29 PDT, Devin Rousso 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-08-12 17:22:41 PDT 
Having a callback makes it unclear as to whether the function is async (it's not).  All of the callers expect it to be sync.
Comment 1 Devin Rousso 2021-08-12 17:29:37 PDT 
Created attachment 435458 [details]
Patch
Comment 2 Patrick Angle 2021-08-13 09:45:59 PDT 
Comment on attachment 435458 [details]
Patch

I thought about this some more, and I'm concerned it trades one confusing construct for another, less common, confusing construct. The call sites at first glance now read as loops, which to me invokes the question "Why does this work need to be done more than once", a question that much like the sync/async situation can only really be answered by a trip to the definition of `WI.ImageUtilities.scratchCanvasContext2D`. 

I enjoy this solution from a technical perspective, but I'm not sure it meets the stated goal in the changelog of making these call sites clearer. It's just unclear in a different way.
Comment 3 Devin Rousso 2021-08-13 11:14:19 PDT 
Yeah FWIW this is kinda a "hack" of sorts 😅

I personally found that the loop looked better than a callback, but that's just me.
Comment 4 Radar WebKit Bug Importer 2021-08-19 17:23:24 PDT 
<rdar://problem/82148919>