[webkit-reviews] review denied: [Bug 88602] Web Inspector: FileSystem tree should hide uninitialized FileSystem : [Attachment 147504] 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 88602: Web Inspector: FileSystem tree should hide uninitialized FileSystem
https://bugs.webkit.org/show_bug.cgi?id=88602

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

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


> Source/WebCore/inspector/front-end/FileSystemModel.js:34
> + * @implements {FileSystemAgent.Dispatcher}

Could you please implement FileSystemDispatcher as a separate class that
delegates all calls to FileSystemModel's private methods?
Otherwise FileSystemModel would have public didReadDirectory() method that
should not be called by front-end which is a bit confusing.

> Source/WebCore/inspector/front-end/FileSystemModel.js:209
> +    getFileSystem: function(origin, type, callback)

Should be private.

> Source/WebCore/inspector/front-end/FileSystemModel.js:211
> +	   var requestId = WebInspector.FileSystemModel.nextRequestId++;

I suggest extracting request managing from FileSystemModel to a separate class
(Like it is done for IndexedDBModel).
Otherwise FileSystemModel becomes is flooded with methods with almost equal
names and different semantics.

> Source/WebCore/inspector/front-end/FileSystemModel.js:225
> +    gotFileSystemRoot: function(requestId, errorCode, backendEntry)

Should be private.

> LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:32
> +    InspectorTest.clearFileSystem = function(callback)

This is the same as the patch uploaded in
https://bugs.webkit.org/show_bug.cgi?id=89066.
Could you please separate one from another?