[webkit-reviews] review denied: [Bug 73301] Web Inspector: Add FileSystemView : [Attachment 149695] Patch

Wed Jun 27 07:27:09 PDT 2012

Vsevolod Vlasov <vsevik at chromium.org> has denied Taiju Tsuiki
<tzik at chromium.org>'s request for review:
Bug 73301: Web Inspector: Add FileSystemView
https://bugs.webkit.org/show_bug.cgi?id=73301

Attachment 149695: Patch
https://bugs.webkit.org/attachment.cgi?id=149695&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
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>