[webkit-reviews] review denied: [Bug 191566] Web Inspector: add drag+drop for importing Audits and Recordings : [Attachment 354597] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 191566: Web Inspector: add drag+drop for importing Audits and Recordings
https://bugs.webkit.org/show_bug.cgi?id=191566

Attachment 354597: Patch

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 354597
  --> https://bugs.webkit.org/attachment.cgi?id=354597
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.