Bug 191566

Created attachment 354597 [details] Patch

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.

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

Created attachment 354783 [details] Patch

Comment on attachment 354783 [details] Patch r=me

Comment on attachment 354783 [details] Patch Clearing flags on attachment: 354783 Committed r238198: <https://trac.webkit.org/changeset/238198>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46074573>