[webkit-reviews] review denied: [Bug 87724] Web Inspector: Implement InspectorFileSystemAgent::readDirectory. : [Attachment 144540] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Taiju Tsuiki
<tzik at chromium.org>'s request for review:
Bug 87724: Web Inspector: Implement InspectorFileSystemAgent::readDirectory.
https://bugs.webkit.org/show_bug.cgi?id=87724

Attachment 144540: Patch
https://bugs.webkit.org/attachment.cgi?id=144540&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144540&action=review


> Source/WebCore/ChangeLog:7
> +

Can we have a test for readDirectory command?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:98
> +    class ReadDirectoryEntriesCallback;

Do these classes need to be public?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:112
> +    void gotFileSystem(DOMFileSystem*);

These methods should be private.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:152
> +	       m_readDirectoryTask->reportResult(error->code(), 0);

I don't think didReadDirectory allows entries to be 0 we should probably
declare it optional in the protocol or pass an empty array here. BTW I can't
find didReadDirectory in Inspector.json, is it already there?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:172
> +	   m_readDirectoryTask->gotFileSystem(fileSystem);

Can we have ReadDirectoryTask implement all the callbacks to avoid this
intermediate wrappers that just delegate the calls to the ReadDirectoryTask at
the end? All callbacks seem to have handleEvent with different signatures so it
should be doable and would require less code.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:286
> +    Frame* frame =
m_instrumentingAgents->inspectorPageAgent()->frameForId(frameId);

We use InstrumentingAgents to access enabled agents only in
InspectorInstrumentation.h/cpp If InspectorFileSystemAgent depends on another
agent you should pass a pointer to the agent into InspectorFileSystemAgent
constructor(see for example InspectorDOMAgent which depends on
InspectorPageAgent).

Also you should check if a frame with given id exists and report an error if
not.