[webkit-reviews] review denied: [Bug 72691] [Inspector][FileSystem] Add FileSystem item to storage tree : [Attachment 142628] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied  review:
Bug 72691: [Inspector][FileSystem] Add FileSystem item to storage tree
https://bugs.webkit.org/show_bug.cgi?id=72691

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142628&action=review


> Source/WebCore/inspector/front-end/FileSystemModel.js:42
> +   
WebInspector.resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.
EventTypes.FrameNavigated, this._frameNavigated, this);

You should traverse existing frames here. I'm interested to learn why you would
like to track frames. If you are only interested in origins, you could maintain
listeners for FrameAdded, Navigated and Detached and every time event comes,
model would traverse all frames recursively and collect remaining origins. That
would be 10 lines of code in total and you would be operating only filesystems
in the model. r- for while we are discussing it.

> Source/WebCore/inspector/front-end/FileSystemModel.js:107
> +    _isEmpty: function(obj)

This should go to the utilities.js and be declared on Object (i.e.
Object.isEmpty(obj)).

> Source/WebCore/inspector/front-end/FileSystemModel.js:212
> + * @param {Page.Frame} frame

This is WebInspector.ResourceTreeFrame according to the call site. Try
compiling this code using Source/WebCore/inspector/compile-frontend.sh with the
recent closure compiler. Ignore the __proto__ assignment errors that started
popping up in the latest compiler version.

> Source/WebCore/inspector/front-end/FileSystemModel.js:222
> +    this._filesystemModel.addFrameForOrigin(this, this._securityOrigin);

Why do you need to keep track of frames at all? Could you track only origins
instead?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:70
> +    if (Capabilities.filesystemInspection) {

I'd call it Capabilities.filesystem

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1647
> +	   this._filesystemModel = new WebInspector.FileSystemModel();

Since you create filesystem model lazily (which is good), you should traverse
all existing frames in the tree in FileSystemModel constructor. Although I'd
remove tracking of frames and left origins only.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1672
> +	       if(this.children[i].filesystemName == filesystemName)

Space before ( is missing. We also use === for comparison.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2098
> +    var displayName = filesystem.type + " - " + filesystem.origin;

This should be WebInspector.UIString("%s - $s", filesystem.type,
filesystem.origin);
You should also be consistent with naming: filesystem vs fileSystem.

> Source/WebKit/chromium/src/js/DevTools.js:47
> +    Capabilities.filesystemInspection = false;

Preferences are about enabling / disabling features when they are not yet
ready.
Capabilities are for querying whether backend supports this feature or not. See
other capabilities usages.

You start with Preferences when it is not ready at all. When it is already
somewhat useful, you migrate to Capabilities and query backend for filesystem
existence. You can then add "experiment" (see Settings.js) so that you could
enable it at runtime.