[Webkit-unassigned] [Bug 68203] Web Inspector: Add FileSystem support

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Pavel Feldman <pfeldman at chromium.org>  2011-09-19 08:41:17 PST ---
(From update of attachment 107652)
View in context: https://bugs.webkit.org/attachment.cgi?id=107652&action=review

Overall looks good. I added some high-level comments.

> Source/WebCore/inspector/Inspector.json:832
> +        "domain": "FileSystem",

All methods and parameters should be documented.

> Source/WebCore/inspector/Inspector.json:845
> +                    { "name": "fsName", "type": "string", "description": "" },

No abbreviations in WebKit.

> Source/WebCore/inspector/Inspector.json:853
> +                "name": "addFileSystem",

We prefer fileSystemAdded notation. Also, you should not generate any messages unless FileSystem domain is enabled. You should add "enable" and "disable" commands. You could also include "getFileSystems" command that returns all filesystems for the late attach.

> Source/WebCore/inspector/Inspector.json:861
> +                "name": "onSuccess",

There should be no generic "onSuccess"/"onError" events.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

You should use Google license template.

> Source/WebCore/inspector/InspectorFileSystemAgent.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

ditto

> Source/WebCore/inspector/InspectorFileSystemAgent.h:57
> +    class FileSystemTask;

It looks like it can be defined in the anonymous namespace in the .cpp file.

> Source/WebCore/inspector/InspectorFileSystemAgent.h:58
> +    class ReadDirectoryTask;

ditto

> Source/WebCore/inspector/InspectorInstrumentation.h:37
> +#include "DOMFileSystem.h"

InspectorInstrumentation should not add includes. You should add InspectorFileSystemInstrumentation.h (just like InspectorConsoleInstrumentation.h has)

> Source/WebCore/inspector/front-end/FileSystem.js:43
> +        FileSystemAgent.readDirectory(name, path, requestId);

We are using single callback for both success and error paths, you could just check whether resulting message is an error.

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