Bug 73301

Created attachment 116933 [details] Patch

Created attachment 148242 [details] Patch

Created attachment 148715 [details] Patch

Comment on attachment 148715 [details] Patch Updated for http://webkit.org/b/89555 (Renaming from readDirectory to requestDirectoryContent)

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.

Created attachment 148740 [details] Patch

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

Created attachment 148966 [details] Patch

Could anyone take a look to this?

Ah, I forgot to upload newest version. I'll do it soon.

Created attachment 149451 [details] Patch

Created attachment 149452 [details] Screenshot

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; }

Pavel, Vsevolod or someone in inspector team could you please take a look. FileSystem related logic looks good to my untrained eyes.

Created attachment 149695 [details] Patch

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

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>

Created attachment 149905 [details] Patch

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

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.

> > 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.

Created attachment 150078 [details] Patch

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

Comment on attachment 150078 [details] Patch Clearing flags on attachment: 150078 Committed r121542: <http://trac.webkit.org/changeset/121542>

All reviewed patches have been landed. Closing bug.