[webkit-reviews] review denied: [Bug 45982] Web Inspector: FileSystem integration : [Attachment 71373] patch3 + separated error and success cases in js file.

[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 71373: patch3 + separated error and success cases in js file.
https://bugs.webkit.org/attachment.cgi?id=71373&action=review

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

> WebCore/WebCore.gypi:4382
> +	       'inspector/front-end/FileSystemView.js',

Please also include this file into vcproj.

> WebCore/inspector/InspectorFileSystemAgent.cpp:36
> +#include "AsyncFileWriter.h"

You could use forward declaration instead.

> WebCore/inspector/InspectorFileSystemAgent.cpp:124
> +void InspectorFileSystemAgent::getFileSystemPathAsync(unsigned int type)

So there is no domain query parameter which seems wrong. There is none for
cookies since Joe was collecting cookies for all domain and sorting them on the
front-end. You should do it similarly or add a query parameter.

> WebCore/inspector/InspectorFileSystemAgent.cpp:126
> +    if (!m_inspectorController)

There always is inspector controller if we get here.

> WebCore/inspector/InspectorFileSystemAgent.cpp:140
> +	   LocalFileSystem::localFileSystem().requestFileSystem(document,
asyncFileSystemType, 0, new InspectorFileSystemAgentCallbacks(this,
asyncFileSystemType));

You probably need to look up a document with matching domain here.

> WebCore/inspector/front-end/FileSystemView.js:44
> +    this._domain = fileSystemDomain;

You seem to be ignoring domain and as a result, you are going to show main
document's filesystem for all domains.

> WebCore/inspector/front-end/StoragePanel.js:661
> +WebInspector.FileSystemSidebarTreeElement = function(fileSystemDomain)

You need to rebase since we now use different base classes for tree elements in
Storage panel.

> WebCore/inspector/front-end/inspector.js:1245
> +		   this._addFileSystemDomain(parsedURL.host);

Nit: We might navigate off one of the domains (via iframe navigation). As a
result, we should clean up corresponding storage entries. Cookies, AppCache and
now FileSystem - they all suffer from this problem.