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

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Michael Nordman <michaeln at google.com>  2010-10-22 14:45:26 PST ---
(From update of attachment 71519)
View in context: https://bugs.webkit.org/attachment.cgi?id=71519&action=review

I think you should rework the interfaces to reflect the per securityOrigin nature of the FileSystem. Having it work for the main page only to start with is fine, but having more flexible interface in place would be good.

> WebCore/inspector/CodeGeneratorInspector.pm:49
> +    "domainAccessor" => "m_inspectorController->fileSystemAgent()",

How is this "domainAccessor" relevant to the FileSystem? The file system is per security origin.

> WebCore/inspector/InspectorFileSystemAgent.cpp:125
> +void InspectorFileSystemAgent::getFileSystemPathAsync(unsigned int type, const String& domain)

Since file systems are per security origin, should the second param be a const SecurityOrigin& ?

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

Would it make sense for the InspectorFileSystemAgentCallbacks instance to keep track of the SecurityOrigin being queried?

> WebCore/inspector/InspectorFileSystemAgent.cpp:147
> +void InspectorFileSystemAgent::didGetFileSystemPath(const String& root, AsyncFileSystem::Type type)

Should this method also indicate which SecurityOrigin this 'root' and would 'rootPath' be a better name?

> WebCore/inspector/InspectorFileSystemAgent.cpp:154
> +        m_persistentRoot = root;

Since the 'rootPath' is delivered to the frontend below, does this class need to have these two data members? Could the frontend provide the path when calling revealFolderInOS(path)?

> WebCore/inspector/InspectorFileSystemAgent.cpp:158
> +    m_frontend->didGetFileSystemPath(root, static_cast<unsigned int>(type));

Similarly, indicate which SecurityOrigin.

> WebCore/inspector/InspectorFileSystemAgent.cpp:166
> +    m_frontend->didGetFileSystemError(static_cast<unsigned int>(type));

Ditto, which SecurityOrigin.

> WebCore/inspector/Inspector.idl:229
> +        [notify] void didGetFileSystemError(out int type);

To summarize these interface changes...

         [handler=FileSystem] void getFileSystemPathAsync(in unsigned int type, in String securityOrigin);
         [handler=FileSystem] void revealFolderInOS(in String path);
         [notify] void didGetFileSystemPath(out String rootPath, out int type, String securityOrigin);
         [notify] void didGetFileSystemError(out int type, String securityOrigin);

I'm not familiar with these IDL annotations as applied to the inspector. Is 'out' correct on those method arguments?

> WebCore/inspector/front-end/FileSystemView.js:35
> +    InspectorBackend.getFileSystemPathAsync(type, domain);

domain s/b securityOrigin

> WebCore/inspector/front-end/FileSystemView.js:38
> +WebInspector.FileSystemView = function(treeElement, fileSystemDomain)

domain s/b securityOrigin

> WebCore/inspector/front-end/StoragePanel.js:214
> +        var fileSystemTreeElement = new WebInspector.FileSystemSidebarTreeElement(domain);

domain s/b securityOrigin

> WebCore/inspector/front-end/StoragePanel.js:312
> +    showFileSystem: function(treeElement, fileSystemDomain)

domain s/b securityOrigin... or securityOrigin a data member is the treeElement?

> WebCore/inspector/front-end/StoragePanel.js:710
> +    this._fileSystemDomain = fileSystemDomain;

domain s/b securityOrigin... and ah... it is a data member of the treeElement.

> WebCore/inspector/front-end/inspector.js:1356
> +WebInspector._addFileSystemDomain = function(domain)

domain s/b securityOrigin

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