RESOLVED FIXED 73301
Web Inspector: Add FileSystemView
https://bugs.webkit.org/show_bug.cgi?id=73301
Summary Web Inspector: Add FileSystemView
Taiju Tsuiki
Reported 2011-11-29 02:59:53 PST
This is a splitted patch of https://bugs.webkit.org/show_bug.cgi?id=68203, part3. - Adding split-view on FileSystemView and putting directory tree on the view.
Attachments
Patch (17.04 KB, patch)
2011-11-29 03:01 PST, Taiju Tsuiki
no flags
Patch (19.32 KB, patch)
2012-06-18 21:41 PDT, Taiju Tsuiki
no flags
Patch (19.53 KB, patch)
2012-06-20 19:52 PDT, Taiju Tsuiki
no flags
Patch (19.54 KB, patch)
2012-06-21 00:28 PDT, Taiju Tsuiki
no flags
Patch (19.73 KB, patch)
2012-06-21 23:23 PDT, Taiju Tsuiki
no flags
Patch (13.97 KB, patch)
2012-06-25 22:14 PDT, Taiju Tsuiki
no flags
Screenshot (43.67 KB, image/png)
2012-06-25 22:15 PDT, Taiju Tsuiki
no flags
Patch (19.44 KB, patch)
2012-06-27 00:18 PDT, Taiju Tsuiki
no flags
Patch (21.92 KB, patch)
2012-06-28 02:12 PDT, Taiju Tsuiki
no flags
Patch (21.43 KB, patch)
2012-06-28 20:57 PDT, Taiju Tsuiki
no flags
Taiju Tsuiki
Comment 1 2011-11-29 03:01:29 PST
Taiju Tsuiki
Comment 2 2012-06-18 21:41:07 PDT
Taiju Tsuiki
Comment 3 2012-06-20 19:52:34 PDT
Taiju Tsuiki
Comment 4 2012-06-20 19:56:28 PDT
Comment on attachment 148715 [details] Patch Updated for http://webkit.org/b/89555 (Renaming from readDirectory to requestDirectoryContent)
Taiju Tsuiki
Comment 5 2012-06-21 00:19:39 PDT
Comment on attachment 148715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148715&action=review > Source/WebCore/inspector/front-end/FileSystemView.js:133 > + this.insertChild(new WebInspector.FileSystemView.Entry(this._fileSystemView, entries[0]), currentTreeItem); entries[0] should be entries[newEntryIndex]. I'll fix it in next version of this patch.
Taiju Tsuiki
Comment 6 2012-06-21 00:28:00 PDT
Taiju Tsuiki
Comment 7 2012-06-21 23:03:36 PDT
Since some part of this patch is depended by http://webkit.org/b/87856 (Web Inspector: Add requestMetadata command and metadataReceived event to FileSystem), let me split out that (FileSystemModel.{Entry,Directory,File}) to https://bugs.webkit.org/show_bug.cgi?id=89739
Taiju Tsuiki
Comment 8 2012-06-21 23:23:33 PDT
Taiju Tsuiki
Comment 9 2012-06-25 21:57:41 PDT
Could anyone take a look to this?
Taiju Tsuiki
Comment 10 2012-06-25 22:00:35 PDT
Ah, I forgot to upload newest version. I'll do it soon.
Taiju Tsuiki
Comment 11 2012-06-25 22:14:56 PDT
Taiju Tsuiki
Comment 12 2012-06-25 22:15:54 PDT
Created attachment 149452 [details] Screenshot
Kinuko Yasuda
Comment 13 2012-06-25 22:44:15 PDT
Comment on attachment 149451 [details] Patch I'm not familiar with the frontend code but the logic looks ok to me. View in context: https://bugs.webkit.org/attachment.cgi?id=149451&action=review > Source/WebCore/inspector/front-end/FileSystemModel.js:302 > + } Is this same as following? if (x.isDirectory != y.isDirectory) { if (!y.isDirectory) return -1; else return 1; }
Kinuko Yasuda
Comment 14 2012-06-26 20:25:05 PDT
Pavel, Vsevolod or someone in inspector team could you please take a look. FileSystem related logic looks good to my untrained eyes.
Taiju Tsuiki
Comment 15 2012-06-27 00:18:37 PDT
Taiju Tsuiki
Comment 16 2012-06-27 00:19:40 PDT
Comment on attachment 149451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149451&action=review Updated. +added a test. >> Source/WebCore/inspector/front-end/FileSystemModel.js:302 >> + } > > Is this same as following? > > if (x.isDirectory != y.isDirectory) { > if (!y.isDirectory) > return -1; > else > return 1; > } Done
Vsevolod Vlasov
Comment 17 2012-06-27 07:27:08 PDT
Comment on attachment 149695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149695&action=review > Source/WebCore/inspector/front-end/FileSystemModel.js:304 > + if (x.name < y.name) We prefer localeCompare(). See WebInspector.NavigatorTreeOutline._treeElementsCompare in NavigatorView.js for an example. > Source/WebCore/inspector/front-end/FileSystemView.js:38 > + WebInspector.SplitView.call(this, WebInspector.SplitView.SidebarPosition.Left, "FileSystemViewSidebarWidth"); We have never used nested SplitViews with sidebars on the same side. Does it work nicely (Resize, restoring size after inspector reload, etc.)? > Source/WebCore/inspector/front-end/FileSystemView.js:47 > + this.directoryTree.appendChild(new WebInspector.FileSystemView.Entry(this, fileSystem.root)); Root entry should be expanded by default. > Source/WebCore/inspector/front-end/FileSystemView.js:48 > + this.visibleView = null; This is never used outside from the FileSystemView, should be private. > Source/WebCore/inspector/front-end/FileSystemView.js:93 > + this.refresh(); This should be in onpopulate() method. > Source/WebCore/inspector/front-end/FileSystemView.js:99 > + contextMenu.appendItem(WebInspector.UIString("Refresh"), this.refresh.bind(this)); We should not show "Refresh" action it context menu when it does nothing (e.g. selected entry is not a directory). > Source/WebCore/inspector/front-end/FileSystemView.js:103 > + _directoryContentReceived: function(errorCode, entries) Please add compiler annotations. > Source/WebCore/inspector/front-end/FileSystemView.js:124 > + oldChildren[oldChildIndex].refresh(); I am not sure I understand what you are trying to do here. Why do you call refresh on all old children? I would expect that you call it on expanded old children instead. > Source/WebCore/inspector/front-end/ResourcesPanel.js:2005 > + if (!this._fileSystemView) if (this._fileSystemView) ? This still looks weird (Why context menu click does nothing in some cases?). I think context menu is always shown after onselect, so this check is probably redundant, it could be replaced with console.assert(). I would use refresh button in the status bar instead though. This is how we did it for other views in reosurces panel (IndexedDB, Cookies). It is much more discoverable. We might end up having two refresh button for both FileSystemView main view and sidebar. Pavel what do you think about such UI decision? > Source/WebCore/inspector/front-end/ResourcesPanel.js:2016 > + clear: function() Shouldn't this method be called from ResourcesPanel._fileSystemRemoved? > LayoutTests/http/tests/inspector/filesystem/directory-tree.html:1 > +<!DOCTYPE html><meta charset="UTF-8"> Please add <head> <body> and closing </html>
Taiju Tsuiki
Comment 18 2012-06-28 02:12:12 PDT
Taiju Tsuiki
Comment 19 2012-06-28 02:22:38 PDT
Comment on attachment 149695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149695&action=review >> Source/WebCore/inspector/front-end/FileSystemModel.js:304 >> + if (x.name < y.name) > > We prefer localeCompare(). See WebInspector.NavigatorTreeOutline._treeElementsCompare in NavigatorView.js for an example. I see. Done. >> Source/WebCore/inspector/front-end/FileSystemView.js:38 >> + WebInspector.SplitView.call(this, WebInspector.SplitView.SidebarPosition.Left, "FileSystemViewSidebarWidth"); > > We have never used nested SplitViews with sidebars on the same side. > Does it work nicely (Resize, restoring size after inspector reload, etc.)? Yes, it looks working well to me. >> Source/WebCore/inspector/front-end/FileSystemView.js:47 >> + this.directoryTree.appendChild(new WebInspector.FileSystemView.Entry(this, fileSystem.root)); > > Root entry should be expanded by default. Done. >> Source/WebCore/inspector/front-end/FileSystemView.js:48 >> + this.visibleView = null; > > This is never used outside from the FileSystemView, should be private. Done. >> Source/WebCore/inspector/front-end/FileSystemView.js:93 >> + this.refresh(); > > This should be in onpopulate() method. Done. >> Source/WebCore/inspector/front-end/FileSystemView.js:99 >> + contextMenu.appendItem(WebInspector.UIString("Refresh"), this.refresh.bind(this)); > > We should not show "Refresh" action it context menu when it does nothing (e.g. selected entry is not a directory). Removed for files. >> Source/WebCore/inspector/front-end/FileSystemView.js:103 >> + _directoryContentReceived: function(errorCode, entries) > > Please add compiler annotations. OK, I added. >> Source/WebCore/inspector/front-end/FileSystemView.js:124 >> + oldChildren[oldChildIndex].refresh(); > > I am not sure I understand what you are trying to do here. > Why do you call refresh on all old children? I would expect that you call it on expanded old children instead. Ah, that's right. I should refresh only expanded directory. >> Source/WebCore/inspector/front-end/ResourcesPanel.js:2005 >> + if (!this._fileSystemView) > > if (this._fileSystemView) ? > > This still looks weird (Why context menu click does nothing in some cases?). > I think context menu is always shown after onselect, so this check is probably redundant, it could be replaced with console.assert(). > > I would use refresh button in the status bar instead though. This is how we did it for other views in reosurces panel (IndexedDB, Cookies). > It is much more discoverable. > > We might end up having two refresh button for both FileSystemView main view and sidebar. > Pavel what do you think about such UI decision? > if (this._fileSystemView) ? Right. I might fail to rebase. > I would use refresh button in the status bar instead though. This is how we did it for other views in reosurces panel (IndexedDB, Cookies). > It is much more discoverable. OK, so we don't need context menu on each filesystem to refresh its view. How about placing just one button to refresh active item or view? >> Source/WebCore/inspector/front-end/ResourcesPanel.js:2016 >> + clear: function() > > Shouldn't this method be called from ResourcesPanel._fileSystemRemoved? Done >> LayoutTests/http/tests/inspector/filesystem/directory-tree.html:1 >> +<!DOCTYPE html><meta charset="UTF-8"> > > Please add <head> <body> and closing </html> Done
Vsevolod Vlasov
Comment 20 2012-06-28 04:47:14 PDT
Comment on attachment 149905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149905&action=review Please fix how the root element is expanded. Other than that looks good. > Source/WebCore/inspector/front-end/FileSystemView.js:48 > + rootItem.expand(); I don't think this will work because expand() should not be called until TreeElement is attached. You should set expanded to true instead: rootItem.expanded = true; expand() will be called from TreeElement._attach automatically then. > Source/WebCore/inspector/front-end/FileSystemView.js:76 > +WebInspector.FileSystemView.Entry = function(fileSystemView, entry) nit: I would rename it to EntryTreeElement to avoid confusion with FileSystemModel.Entry.
Vsevolod Vlasov
Comment 21 2012-06-28 04:50:43 PDT
> > I would use refresh button in the status bar instead though. This is how we did it for other views in reosurces panel (IndexedDB, Cookies). > > It is much more discoverable. > OK, so we don't need context menu on each filesystem to refresh its view. > > How about placing just one button to refresh active item or view? I think we can have one button that both refreshes the files list and active file view. This should be fine since your entry list update procedure would work smoothly when there are no changes.
Taiju Tsuiki
Comment 22 2012-06-28 20:57:43 PDT
Taiju Tsuiki
Comment 23 2012-06-28 21:00:01 PDT
Comment on attachment 149905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149905&action=review >> Source/WebCore/inspector/front-end/FileSystemView.js:48 >> + rootItem.expand(); > > I don't think this will work because expand() should not be called until TreeElement is attached. > You should set expanded to true instead: rootItem.expanded = true; > expand() will be called from TreeElement._attach automatically then. I see. Done >> Source/WebCore/inspector/front-end/FileSystemView.js:76 >> +WebInspector.FileSystemView.Entry = function(fileSystemView, entry) > > nit: I would rename it to EntryTreeElement to avoid confusion with FileSystemModel.Entry. Done
WebKit Review Bot
Comment 24 2012-06-29 02:54:15 PDT
Comment on attachment 150078 [details] Patch Clearing flags on attachment: 150078 Committed r121542: <http://trac.webkit.org/changeset/121542>
WebKit Review Bot
Comment 25 2012-06-29 02:54:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.