Bug 191566 – Web Inspector: add drag+drop for importing Audits and Recordings

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

We already allow users to import Audits and Recordings, so we should make it even easier by allowing drag and drop.

[Devin Rousso]

Created attachment 354597 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 354597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354597&action=review

> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:26
> +WI.FileUtilities = class FileUtilities {

I don't really like this, but I don't have a good argument against it. The file is already named `FileUtilities` and the methods "saveDataToFile" was clear to me. You can keep it but I'd like to get an idea of when we would want a collection class like this.

> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:85
> +    static getText(file, callback)

How do we not have tests for these? Maybe thats okay, we can get to it later.

> Source/WebInspectorUI/UserInterface/Views/AuditTabContentView.js:126
> +        WI.FileUtilities.getText(event.dataTransfer.files[0], (data, filename) => {

What if someone drops multiple files? We seem to only get the first.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:254
> +    _handleImportButtonNavigationItemClicked(event) {

Style: brace on newline.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:257
> +        WI.FileUtilities.import((data, filename) => {
> +            WI.auditManager.processImportedFile(data, filename);
> +        });

r- the CanvasOverview should not tell the audit manager to import.

Seems this should just be `WI.canvasManager.processImportedFile(...)`.

> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:50
> +        let importButtonNavigationItem = new WI.ButtonNavigationItem("audit-import", WI.UIString("Import"), "Images/Import.svg", 15, 15);

CanvasSidebar with "audit-import" on the button navigation item? Seems wrong.

[Devin Rousso]

Comment on attachment 354597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354597&action=review

>> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:26
>> +WI.FileUtilities = class FileUtilities {
> 
> I don't really like this, but I don't have a good argument against it. The file is already named `FileUtilities` and the methods "saveDataToFile" was clear to me. You can keep it but I'd like to get an idea of when we would want a collection class like this.

I personally find all these "random" functions attached to `WI` to be very confusing, not so much as to what they do, but where I'd find them (e.g. which file).  An example of why this is nice would be to look at Utilities.js, where you have to do a string search for a prototype function because no editor is smart enough to figure out that the additions we add are defined in this file.  Adding `WI.FileUtilities` makes it explicitly clear which file to look in for this (it also matches `WI.ImageUtilities`).

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:257
>> +        });
> 
> r- the CanvasOverview should not tell the audit manager to import.
> 
> Seems this should just be `WI.canvasManager.processImportedFile(...)`.

copypasta :P

>> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:50
>> +        let importButtonNavigationItem = new WI.ButtonNavigationItem("audit-import", WI.UIString("Import"), "Images/Import.svg", 15, 15);
> 
> CanvasSidebar with "audit-import" on the button navigation item? Seems wrong.

copypasta :P

[Devin Rousso]

Created attachment 354783 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 354783 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 354783 [details]
Patch

Clearing flags on attachment: 354783

Committed r238198: <https://trac.webkit.org/changeset/238198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46074573>