[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