Bug 88602 - Web Inspector: FileSystem tree should hide uninitialized FileSystem
Summary: Web Inspector: FileSystem tree should hide uninitialized FileSystem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on: 89191
Blocks: 68203 73301 89420
  Show dependency treegraph
 
Reported: 2012-06-07 18:39 PDT by Taiju Tsuiki
Modified: 2012-06-19 03:38 PDT (History)
11 users (show)

See Also:


Attachments
Patch (18.36 KB, patch)
2012-06-12 01:29 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (18.31 KB, patch)
2012-06-12 01:31 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (18.31 KB, patch)
2012-06-12 01:40 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (18.09 KB, patch)
2012-06-12 01:44 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (19.65 KB, patch)
2012-06-12 22:48 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (25.70 KB, patch)
2012-06-14 00:23 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (20.13 KB, patch)
2012-06-15 03:26 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (20.52 KB, patch)
2012-06-19 01:50 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Taiju Tsuiki 2012-06-07 18:39:08 PDT
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.
Comment 1 Taiju Tsuiki 2012-06-12 01:29:04 PDT
Created attachment 147034 [details]
Patch
Comment 2 Taiju Tsuiki 2012-06-12 01:31:44 PDT
Created attachment 147036 [details]
Patch
Comment 3 Taiju Tsuiki 2012-06-12 01:40:17 PDT
Created attachment 147037 [details]
Patch
Comment 4 Taiju Tsuiki 2012-06-12 01:44:31 PDT
Created attachment 147038 [details]
Patch
Comment 5 Taiju Tsuiki 2012-06-12 01:46:18 PDT
Comment on attachment 147038 [details]
Patch

Sorry for upload spamming. I had updoaded wrong one before.
Comment 6 Taiju Tsuiki 2012-06-12 22:48:19 PDT
Created attachment 147237 [details]
Patch
Comment 7 Vsevolod Vlasov 2012-06-13 05:48:51 PDT
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?
Comment 8 Taiju Tsuiki 2012-06-14 00:23:40 PDT
Created attachment 147504 [details]
Patch
Comment 9 Taiju Tsuiki 2012-06-14 00:31:33 PDT
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 :)
Comment 10 Vsevolod Vlasov 2012-06-14 06:51:01 PDT
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?
Comment 11 Taiju Tsuiki 2012-06-15 02:09:38 PDT
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.
Comment 12 Taiju Tsuiki 2012-06-15 03:26:07 PDT
Created attachment 147782 [details]
Patch
Comment 13 Taiju Tsuiki 2012-06-19 01:50:38 PDT
Created attachment 148288 [details]
Patch
Comment 14 Taiju Tsuiki 2012-06-19 01:51:39 PDT
Comment on attachment 148288 [details]
Patch

Forgot to add test expectation.
Comment 15 WebKit Review Bot 2012-06-19 03:38:49 PDT
Comment on attachment 148288 [details]
Patch

Clearing flags on attachment: 148288

Committed r120702: <http://trac.webkit.org/changeset/120702>
Comment 16 WebKit Review Bot 2012-06-19 03:38:55 PDT
All reviewed patches have been landed.  Closing bug.