[Webkit-unassigned] [Bug 45982] Web Inspector: FileSystem integration

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45982


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71832|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #25 from Pavel Feldman <pfeldman at chromium.org>  2010-10-26 13:28:07 PST ---
(From update of attachment 71832)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.