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

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik 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 145994: Patch
https://bugs.webkit.org/attachment.cgi?id=145994&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145994&action=review


> Source/WebCore/inspector/Inspector.json:1401
> +		       {

We have one line per property unless it is very complex usually.

> Source/WebCore/inspector/Inspector.json:1438
> +		   "description": "Requests to read the directory content.
Result should return on didReadDirectory event with request ID.",

request id

> Source/WebCore/inspector/Inspector.json:1450
> +		       {

One line per parameter please.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
> +    typedef TypeBuilder::FileSystem::Entry InspectorFileSystemEntry;

This seems confusing, let's use "using" instead, we'll have to write
Array<Entry> then which is clearer.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:126
> +	   m_frontendProvider = 0;

Shouldn't ReadDirectoryTask be destroyed after exiting this method? 
If it should - you don't need to explicitly clear frontendProvider
If it shouldn't - when will it be destroyed?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:266
> +    readDirectoryEntries();

Isn't that an infinite loop? r- for that.
Shouldn't you call reportResult(static_cast<FileError::ErrorCode>(0),
m_entries) instead?
Also I don't think you really need m_entries field, local variable should be
enough.

> LayoutTests/http/tests/inspector/filesystem/read-directory.html:8
> +    function FileSystemDispatcher() {}

I think we should install sniffer on the dispatcher used in the front-end code
instead.
I know it is not landed yet, so you should either land front end part first or
update this test in the front end patch later.

Ideally this should look like:
var fileSystemModel = new WebInspector.FileSystemModel();
InspectorTest.addSniffer(WebInspector.FileSystemDispatcher.prototype,
"didReadDirectory", nextStep, false);

> LayoutTests/http/tests/inspector/filesystem/read-directory.html:27
> +    InspectorTest.createDirectory("/hoge", function() {

We use named functions (either with meaningful names or with names like stepN)
in inspector tests. Please make sure { starting a function always goes on the
separate line.

> LayoutTests/http/tests/inspector/filesystem/read-directory.html:30
> +		   FileSystemAgent.readDirectory(1,
WebInspector.resourceTreeModel.mainFrame.id,
"filesystem:http://127.0.0.1/temporary/hoge");

I think it's better to initiate directory reading from FileSystemModel so that
we test how this machinery works from the front end point of view as a whole.