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

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Kinuko Yasuda <kinuko at chromium.org>  2010-10-05 19:47:14 PST ---
(From update of attachment 69873)
View in context: https://bugs.webkit.org/attachment.cgi?id=69873&action=review

> WebCore/ChangeLog:23
> +        * inspector/InspectorFileSystemAgent.cpp: Added.

I think for newly added files you can omit the detailed methods list below.

> WebCore/ChangeLog:43
> +        * inspector/InspectorFileSystemAgent.h: Added.

ditto.

> WebCore/ChangeLog:51
> +        * inspector/front-end/FileSystemView.js: Added. Handles display in storage tab for "FILE SYSTEM"

ditto.

> WebCore/platform/AsyncFileSystem.h:122
> +    virtual String root() const { return m_platformRootPath; }

virtual const String& root() const { return m_platformRootPath; }

does this need to be virtual?

> WebCore/inspector/InspectorFileSystemAgent.h:54
> +    void getTempFileSystem();

Please use 'Temporary' instead of 'Temp'.  (Seems like we do not use abbreviate names in WebKit)

> WebCore/inspector/InspectorFileSystemAgent.h:59
> +    void didGetFileSystemError(AsyncFileSystem::Type type);

Please remove parameter names that add no info (type)

> WebCore/inspector/InspectorFileSystemAgent.h:63
> +    void getFileSystemRoot(AsyncFileSystem::Type type);

ditto. (Please remove parameter names that add no info (type))

> WebCore/inspector/InspectorFileSystemAgent.h:68
> +    String m_tempRoot;

s/m_tempRoot/m_temporaryRoot/

> WebCore/inspector/InspectorController.cpp:110
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)

We use ENABLE(INSPECTOR) && ENABLE(FILE_SYSTEM) in InspectorFileSystemAgent.h and OFFLINE_WEB_APPLICATIONS here.  Is it intentional?

> WebCore/inspector/InspectorFileSystemAgent.cpp:44
> +InspectorFileSystemAgentCallbacks(InspectorFileSystemAgent* agent, AsyncFileSystem::Type type)

Indentation? (here and all the methods of InspectorFileSystemAgentCallbacks below.)

> WebCore/inspector/InspectorFileSystemAgent.cpp:58
> +    // Agent will be alive even if InspectorController is destroyed.

...until it gets called back.

> WebCore/inspector/InspectorFileSystemAgent.cpp:99
> +InspectorFileSystemAgent::~InspectorFileSystemAgent() { }

I think we usually write this in three lines like:

InspectorFileSystemAgent::~InspectorFileSystemAgent()
{
}

> WebCore/inspector/InspectorFileSystemAgent.cpp:127
> +{

Can we be sure that m_inspectorController is not null here?

> WebCore/inspector/InspectorFileSystemAgent.cpp:146
> +            if (m_persistentRoot.isEmpty())

In which case m_persistentRoot can be non-empty?
And in either case we want to call didGetPersistentFileSystem with the given 'root' path?
Don't we need to check (or ASSERT?) if root == m_persistentRoot or not?

> WebCore/inspector/InspectorFileSystemAgent.cpp:163
> +            m_frontend->didGetPersistentFileSystem("Error while retrieving root");

How can js check the returned string is not a root path but an error message?

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