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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 6 06:39:13 PDT 2012


Vsevolod Vlasov <vsevik at chromium.org> has denied Taiju Tsuiki
<tzik at chromium.org>'s request for review:
Bug 72691: [Inspector][FileSystem] Add FileSystem item to storage tree
https://bugs.webkit.org/show_bug.cgi?id=72691

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143709&action=review


Please add new files to WebCore.xcodeproj/project.pbxproj as well.
Could you please upload a patch with a ChangeLog next time?

> Source/WebCore/inspector/front-end/FileSystemModel.js:44
> +    this._pendingReadDirectoryRequests = {};

Unused, please remove.

> Source/WebCore/inspector/front-end/FileSystemModel.js:93
> +	   if (frame.securityOrigin == "null")

Please use ===.

> Source/WebCore/inspector/front-end/FileSystemModel.js:98
> +	   var new_origin = false;

newOrigin

> Source/WebCore/inspector/front-end/FileSystemModel.js:129
> +	   var last_origin = this._frameIdsForOrigin[origin].isEmpty();

lastOrigin

> Source/WebCore/inspector/front-end/FileSystemModel.js:151
> +	   if (!this._fileSystemsForOrigin[origin])

How can this._fileSystemsForOrigin[origin] be already defined here?
This origin was either never added or already removed.

> Source/WebCore/inspector/front-end/FileSystemModel.js:156
> +	       if (!this._fileSystemsForOrigin[origin][types[i]]) {

Again, how can this._fileSystemsForOrigin[origin][types[i]] be already defined
here?

> Source/WebCore/inspector/front-end/FileSystemModel.js:205
> +    readDirectory: function(directory, callback)

Let's move this method and Entry, Directory and File classes to another patch,
they are not really related to adding file system item to storage tree.

> Source/WebCore/inspector/front-end/FileSystemModel.js:216
> +WebInspector.FileSystemModel.nextRequestId = 1;

Unused, please remove.

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

While this feature is not implemented yet it should be hidden behind
experimental flag.
See WebInspector.ExperimentsSettings constructor in Settings.js for usage
examples.
You should then run chromium with --enable-devtools-experiments flag and enable
experiment in settings pane.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1512
> +	   if (this.expanded)

This should be removed since TreeOutline will call onexpand after attaching an
expanded node.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1550
> +    refreshFileSystem: function()

Looks like this method is not used outside of this class, please make it
private.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2004
> +	   if (!this._fileSystemView)

if (this._fileSystemView)

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2012
> +	       if (!WebInspector.FileSystemView) {

How is that possible? Seems redundant.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2023
> +	   if (this.fileSystemView && this._storagePanel.visibleView ==
this.fileSystemView)

You should not need this. When file system node is removed another node will be
selected and its view shown automatically.
You might need to let fileSystemView clear some of its state here later though.


> Source/WebCore/inspector/front-end/Settings.js:48
> +    exposeFileSystemInspection: true

Preferences.exposeFileSystemInspection should be false by default.
You should also enable it for chromium port in
Source/Webkit/chromium/src/js/DevTools.js

> Source/WebCore/inspector/front-end/utilities.js:680
> +(function() {

I don't think you need a function expression here.
Just define a property on Object.prototype (e.g. like keySet is defined on
Array.prototype above)

> Source/WebCore/inspector/front-end/utilities.js:681
> +var objectPrototype = /** @type {!Object} */ Object.prototype;

Please inline.

> Source/WebCore/inspector/front-end/utilities.js:693
> +Object.defineProperty(objectPrototype, "anyOf",

I don't think this is used, please remove.


More information about the webkit-reviews mailing list