WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(19.32 KB, patch)
2012-06-18 21:41 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(19.53 KB, patch)
2012-06-20 19:52 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(19.54 KB, patch)
2012-06-21 00:28 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(19.73 KB, patch)
2012-06-21 23:23 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(13.97 KB, patch)
2012-06-25 22:14 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Screenshot
(43.67 KB, image/png)
2012-06-25 22:15 PDT
,
Taiju Tsuiki
no flags
Details
Patch
(19.44 KB, patch)
2012-06-27 00:18 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(21.92 KB, patch)
2012-06-28 02:12 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(21.43 KB, patch)
2012-06-28 20:57 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Taiju Tsuiki
Comment 1
2011-11-29 03:01:29 PST
Created
attachment 116933
[details]
Patch
Taiju Tsuiki
Comment 2
2012-06-18 21:41:07 PDT
Created
attachment 148242
[details]
Patch
Taiju Tsuiki
Comment 3
2012-06-20 19:52:34 PDT
Created
attachment 148715
[details]
Patch
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
Created
attachment 148740
[details]
Patch
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
Created
attachment 148966
[details]
Patch
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
Created
attachment 149451
[details]
Patch
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
Created
attachment 149695
[details]
Patch
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
Created
attachment 149905
[details]
Patch
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
Created
attachment 150078
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug