[webkit-reviews] review granted: [Bug 201290] Web Inspector: Import file pickers sometimes do not import : [Attachment 377549] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 29 00:14:10 PDT 2019
Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 201290: Web Inspector: Import file pickers sometimes do not import
https://bugs.webkit.org/show_bug.cgi?id=201290
Attachment 377549: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=377549&action=review
--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 377549
--> https://bugs.webkit.org/attachment.cgi?id=377549
[PATCH] Proposed Fix
View in context: https://bugs.webkit.org/attachment.cgi?id=377549&action=review
r=me, nice catch! Really odd that an active input can get GC'd
> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:91
> + fileReader.readAsDataURL(saveData.content);
Nice!
> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:96
> + if (!FileUtilities.importTextInputElement) {
Could we use an "_" prefixed value to make sure callers know it's private?
Just like `WI.ImageUtilities._scratchContext2D`.
> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:111
> + if (!FileUtilities.importJSONInputElement) {
Do we need a separate <input> for text vs json? Theoretically, only one can be
active at a time, so it shouldn't be an issue.
> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:135
> let reader = new FileReader;
You could also move this inside the `Promise` as well, as it's not needed
anywhere outside.
More information about the webkit-reviews
mailing list