[webkit-reviews] review denied: [Bug 45982] Web Inspector: FileSystem integration : [Attachment 71832] patch5

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Kavita Kanetkar
<kkanetkar at chromium.org>'s request for review:
Bug 45982: Web Inspector: FileSystem integration
https://bugs.webkit.org/show_bug.cgi?id=45982

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

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

Overall looks good. Better guard from dupe entries and localizesStrings
modifications and we are good to land. Btw, I'd really like to see some tests!

> WebCore/inspector/InspectorFileSystemAgent.cpp:129
> +    if (!tree)

There always is a tree.

> WebCore/inspector/InspectorFileSystemAgent.cpp:131
> +    while (Frame* frame = tree->traverseNext()) {

We usually We usually use for loop while iterating frame tree.

> WebCore/inspector/front-end/FileSystemView.js:92
> +	  
WebInspector.FileSystem.getFileSystemPathAsync(this.fileSystemType.PERSISTENT,
this._origin);

Given that you make these calls together, it might make sense to merge them.

> WebCore/inspector/front-end/FileSystemView.js:145
> +	   var rootPath = WebInspector.UIString("File System root path not
available.");

I don't see a localizedStrings file in this patch.

> WebCore/inspector/front-end/FileSystemView.js:147
> +	       rootPath = WebInspector.UIString("Error in fetching root path
for file system.");

ditto

> WebCore/inspector/front-end/FileSystemView.js:159
> +	       this.contentElement = document.createElement("div");

Theses all should be variables, not fields of this. (i.e. var contentElement,
var choicesForm, etc.)

> WebCore/inspector/front-end/inspector.css:1914
> +    content: url(Images/applicationCache.png);

Do you have an icon?

> WebCore/inspector/front-end/StoragePanel.js:66
> +	   this.fileSystemListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("File
System"), "file-system-storage-tree-item");

ditto

> WebCore/inspector/front-end/StoragePanel.js:112
> +	   this._fileSystemView = null;

I don't think you need to cache the view. (Others here should not do it as
well)

> WebCore/inspector/front-end/StoragePanel.js:371
> +	   var view = this._fileSystemView;

It is safe to create one each time...

> WebCore/inspector/front-end/inspector.js:1271
> +		   this._addFileSystemOrigin(parsedURL.scheme + "://" +
parsedURL.host + (parsedURL.port ? (":" + parsedURL.port) : ""));

This is fragile. You should either push security origin strings into front-end
explicitly, or at least put a comment here that this string should match
SecurityOrigin::toString. You also might want to add a test for this.

> WebCore/inspector/front-end/inspector.js:1390
> +    this.fileSystemOrigins[origin] = true;

I don't see how setting property to true eliminates the duplicate origin.

> WebCore/inspector/front-end/inspector.js:1394
> +    this.panels.storage.addFileSystem(origin);

I'd rather have storage panel handling the dupe origins with no global
fileSystemOrigins map.