[Webkit-unassigned] [Bug 72456] [Inspector][FileSystem] Capture DOMFileSystem object

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Kinuko Yasuda <kinuko at chromium.org>  2011-11-17 23:18:25 PST ---
(From update of attachment 115540)
View in context: https://bugs.webkit.org/attachment.cgi?id=115540&action=review

Made a few cosmetic comments.  (Feel free to ignore my comments if any of them conflict with pavel's or inspector people's comment)

> Source/WebCore/ChangeLog:10
> +        No new tests.

Could you add some comments if you're going to add tests later or why this needs no tests?

> Source/WebCore/ChangeLog:27
> +        (WebCore::InspectorFileSystemAgent::create):

nit: in general we don't need to put the functions list for newly added files.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:63
> +    m_enabled = true;

Maybe worth adding some TODO comment or for now you can just get rid of line 61-62?  It's not very clear why we need to early-exit in line 62.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:72
> +{

ASSERT(frontend) ?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:85
> +{

ASSERT(m_instrumentingAgents) ?

> Source/WebCore/inspector/InspectorFileSystemAgent.h:33
> +#if ENABLE(INSPECTOR) && ENABLE(FILE_SYSTEM)

nit: might be good to put one extra line between line 32-33?

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