Bug 88602 – Web Inspector: FileSystem tree should hide uninitialized FileSystem

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Taiju Tsuiki]

FileSystem tree on resources panel sometimes shows too many items.
Most of them are unused/uninitialized FileSystem, so we should/could hide it from the tree.

[Taiju Tsuiki]

Created attachment 147034 [details]
Patch

[Taiju Tsuiki]

Created attachment 147036 [details]
Patch

[Taiju Tsuiki]

Created attachment 147037 [details]
Patch

[Taiju Tsuiki]

Created attachment 147038 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 147038 [details]
Patch

Sorry for upload spamming. I had updoaded wrong one before.

[Taiju Tsuiki]

Created attachment 147237 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 147237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147237&action=review

> Source/WebCore/inspector/Inspector.json:1408
> +                "id": "Entry",

Could you please make sure every type, property, command and event in FileSystem domain has a description, and that description is the last property for each of them?
Could you please make corresponding drive-by changes to entities added in previous patch as well.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:2
> + * Copyright (C) 2011, 2012 Google Inc. All rights reserved.

I believe part of this patch (e.g. this line) was already landed, could you please rebaseline?

[Taiju Tsuiki]

Created attachment 147504 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 147237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147237&action=review

>> Source/WebCore/inspector/Inspector.json:1408
>> +                "id": "Entry",
> 
> Could you please make sure every type, property, command and event in FileSystem domain has a description, and that description is the last property for each of them?
> Could you please make corresponding drive-by changes to entities added in previous patch as well.

OK, I added some description to them.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:2
>> + * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
> 
> I believe part of this patch (e.g. this line) was already landed, could you please rebaseline?

I rebased it on ToT. Yes, this patch had some common part with a parallel patch, which you reviewed yesterday :)

[Vsevolod Vlasov]

Comment on attachment 147504 [details]
Patch

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?

[Taiju Tsuiki]

Comment on attachment 147504 [details]
Patch

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.

OK, I'll do it. But, it seems worth separating into another patch.
Let me put in http://webkit.org/b/89191

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

Will be done in next patch.

>> 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.

ditto. I'll put it in http://webkit.org/b/89191

>> 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?

Sorry, I'll drop it in next version.

[Taiju Tsuiki]

Created attachment 147782 [details]
Patch

[Taiju Tsuiki]

Created attachment 148288 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 148288 [details]
Patch

Forgot to add test expectation.

[WebKit Review Bot]

Comment on attachment 148288 [details]
Patch

Clearing flags on attachment: 148288

Committed r120702: <http://trac.webkit.org/changeset/120702>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.