Bug 201290

Summary: Web Inspector: Import file pickers sometimes do not import
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201289
Attachments:
Description Flags
[PATCH] Proposed Fix
hi: review+
[PATCH] For Landing none

Joseph Pecoraro
Reported 2019-08-29 00:03:20 PDT
Import file pickers sometimes do not import Steps to Reproduce: 1. Inspect webkit.org 2. Show Timeline tab 3. Click "Import" button 4. Select a previously exported recording => Nothing happens Notes: • FileChooser::invalidate happened because the input element got GC'd => "change" handler never fired Lets keep the <input type="file"> around artificially.
Attachments
[PATCH] Proposed Fix (4.23 KB, patch)
2019-08-29 00:05 PDT, Joseph Pecoraro 		hi: review+
DetailsFormatted DiffDiff
[PATCH] For Landing (4.47 KB, patch)
2019-08-29 00:29 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-08-29 00:03:34 PDT
<rdar://problem/54826117>
Joseph Pecoraro
Comment 2 2019-08-29 00:05:25 PDT
Created attachment 377549 [details] [PATCH] Proposed Fix
Devin Rousso
Comment 3 2019-08-29 00:14:10 PDT
Comment on attachment 377549 [details] [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.
Joseph Pecoraro
Comment 4 2019-08-29 00:20:49 PDT
> > 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`. Sure. > > 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. Let's keep it separate. If the other bug gets addressed we can just revert this code! > > > 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. Might as well!
Joseph Pecoraro
Comment 5 2019-08-29 00:29:50 PDT
Created attachment 377554 [details] [PATCH] For Landing
WebKit Commit Bot
Comment 6 2019-08-29 01:23:44 PDT
The commit-queue encountered the following flaky tests while processing attachment 377554 [details]: svg/custom/tabindex-order.html bug 201294 (authors: krit@webkit.org and rniwa@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 7 2019-08-29 01:24:24 PDT
Comment on attachment 377554 [details] [PATCH] For Landing Clearing flags on attachment: 377554 Committed r249248: <https://trac.webkit.org/changeset/249248>
Note You need to log in before you can comment on or make changes to this bug.