Bug 72691 - [Inspector][FileSystem] Add FileSystem item to storage tree
Summary: [Inspector][FileSystem] Add FileSystem item to storage tree
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:
: 86818 (view as bug list)
Depends on: 72456 86818
Blocks: 68203 73301
  Show dependency treegraph
 
Reported: 2011-11-17 22:38 PST by Taiju Tsuiki
Modified: 2012-06-08 02:50 PDT (History)
13 users (show)

See Also:


Attachments
Patch (37.11 KB, patch)
2011-11-17 22:47 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (not ready) (49.61 KB, patch)
2011-11-28 09:06 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (52.71 KB, patch)
2011-11-28 21:46 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
storage tree icon (1.20 KB, image/png)
2011-11-30 22:25 PST, Taiju Tsuiki
no flags Details
Patch (62.26 KB, patch)
2012-02-19 20:35 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (27.57 KB, patch)
2012-05-17 21:50 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (29.58 KB, patch)
2012-05-22 20:30 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Screenshot (57.79 KB, image/png)
2012-05-22 20:32 PDT, Taiju Tsuiki
no flags Details
Patch (28.82 KB, patch)
2012-05-23 19:47 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (24.12 KB, patch)
2012-06-06 21:58 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (24.18 KB, patch)
2012-06-07 18:42 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 2011-11-17 22:38:26 PST
This is a splitted patch of https://bugs.webkit.org/show_bug.cgi?id=68203, part2.
- Adding notification to Inspector front-end on FileSystem creation and invalidation,
- Adding FileSystem item to storage tree on Resources panel.
Comment 1 Taiju Tsuiki 2011-11-17 22:47:23 PST
Created attachment 115742 [details]
Patch
Comment 2 Taiju Tsuiki 2011-11-28 09:06:37 PST
Created attachment 116767 [details]
Patch (not ready)
Comment 3 Taiju Tsuiki 2011-11-28 21:46:10 PST
Created attachment 116887 [details]
Patch
Comment 4 Vsevolod Vlasov 2011-11-29 16:20:17 PST
Comment on attachment 116887 [details]
Patch

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

> Source/WebCore/ChangeLog:45
> +        * inspector/front-end/Images/fileSystem.png: Added.

I don't like this icon, I think it's too bright.
I suggest we go the "cookies way" - we can take the icon used for storage/application cache, change its color (something calm, please) and add a small folder icon in the bottom right corner.

> Source/WebCore/fileapi/DOMFileSystem.cpp:57
>  PassRefPtr<DOMFileSystem> DOMFileSystem::create(ScriptExecutionContext* context, const String& name, PassOwnPtr<AsyncFileSystem> asyncFileSystem)

First of all could you please clarify:
As I see if I do something like 

var fs; webkitRequestFileSystem(TEMPORARY, 1024*1024, (function(a){fs = a}).bind(this));
var fs2; webkitRequestFileSystem(TEMPORARY, 1024*1024, (function(a){fs2 = a}).bind(this));

from inspector's console, then two DOMFileSystem objects are created but they are actually referring to the same file system.
Is that always true? Can there be more than one TEMPORARY file system for a given origin? And what about PERSISTENT?

> Source/WebCore/fileapi/DOMFileSystem.cpp:60
> +    fileSystem->m_agentProvider = InspectorInstrumentation::didOpenFileSystem(fileSystem.get());

I have a strong concern about making that call from DOMFileSystem constructor/factory.
Looks like DOMFileSystem is some WebCore handle for the file system stored in the browser.
It is usually created when user does "requestWebkitFileSystem", but not only there, see http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=DOMFileSystem::create%5C(&type=cs
So there is no direct correlation between having file system for the given origin in the storage and creating DOMFileSystem object. (We can have a file system in the storage but never request it yet or we can have several DOMFileSystem handles for the same file system). 
I don't see why do you need to store pointers to all DOMFileSystem objects in inspector when all you really need is name, origin, and type.

As I understand your patch will show file systems in resources panel once web page calls "requestWebkitFileSystem()".
It will not even show anything if inspector is opened after they were requested.

I would expect, that upon opening of resources panel, all file systems available for page frame origins were shown no matter if web page already requested them or not.
And by file system I mean here a simple name-origin-type structure.


Also you should not store inspector related state here. I don't think you need that.
You can do something like
InspectorInstrumentation::didOpenFileSystem(fileSystem.get());
InspectorInstrumentation::didcloseFileSystem(fileSystem.get());
and these calls will go to the same InspectorFileSystemAgent.
You should get rid of all the BackPtr stuff and inspector related logic in DOMFileSystem.cpp

> LayoutTests/ChangeLog:1062
> +2011-11-28  Taiju TSUIKI  <tzik@chromium.org>

Looks like you have a duplicate change log entry.
Comment 5 Taiju Tsuiki 2011-11-30 22:25:08 PST
Comment on attachment 116887 [details]
Patch

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

>> Source/WebCore/ChangeLog:45
>> +        * inspector/front-end/Images/fileSystem.png: Added.
> 
> I don't like this icon, I think it's too bright.
> I suggest we go the "cookies way" - we can take the icon used for storage/application cache, change its color (something calm, please) and add a small folder icon in the bottom right corner.

OK, so how about attached icon?

>> Source/WebCore/fileapi/DOMFileSystem.cpp:57
>>  PassRefPtr<DOMFileSystem> DOMFileSystem::create(ScriptExecutionContext* context, const String& name, PassOwnPtr<AsyncFileSystem> asyncFileSystem)
> 
> First of all could you please clarify:
> As I see if I do something like 
> 
> var fs; webkitRequestFileSystem(TEMPORARY, 1024*1024, (function(a){fs = a}).bind(this));
> var fs2; webkitRequestFileSystem(TEMPORARY, 1024*1024, (function(a){fs2 = a}).bind(this));
> 
> from inspector's console, then two DOMFileSystem objects are created but they are actually referring to the same file system.
> Is that always true? Can there be more than one TEMPORARY file system for a given origin? And what about PERSISTENT?

Yes, both of DOMFileSystem instance always refer same browser-side filesystem for each TEMPORARY and PERSISTENT.

>> Source/WebCore/fileapi/DOMFileSystem.cpp:60
>> +    fileSystem->m_agentProvider = InspectorInstrumentation::didOpenFileSystem(fileSystem.get());
> 
> I have a strong concern about making that call from DOMFileSystem constructor/factory.
> Looks like DOMFileSystem is some WebCore handle for the file system stored in the browser.
> It is usually created when user does "requestWebkitFileSystem", but not only there, see http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=DOMFileSystem::create%5C(&type=cs
> So there is no direct correlation between having file system for the given origin in the storage and creating DOMFileSystem object. (We can have a file system in the storage but never request it yet or we can have several DOMFileSystem handles for the same file system). 
> I don't see why do you need to store pointers to all DOMFileSystem objects in inspector when all you really need is name, origin, and type.
> 
> As I understand your patch will show file systems in resources panel once web page calls "requestWebkitFileSystem()".
> It will not even show anything if inspector is opened after they were requested.
> 
> I would expect, that upon opening of resources panel, all file systems available for page frame origins were shown no matter if web page already requested them or not.
> And by file system I mean here a simple name-origin-type structure.
> 
> 
> Also you should not store inspector related state here. I don't think you need that.
> You can do something like
> InspectorInstrumentation::didOpenFileSystem(fileSystem.get());
> InspectorInstrumentation::didcloseFileSystem(fileSystem.get());
> and these calls will go to the same InspectorFileSystemAgent.
> You should get rid of all the BackPtr stuff and inspector related logic in DOMFileSystem.cpp

For storing DOMFileSystem object vs. origin and type:
Currently, DOMFileSystem is created only in the call chain of webkitRequestFileSystem and webkitResolveLocalFileSystemURL.
Though in planning of further extension of API, we'll probabily make filesystem without requestFileSystem and can't re-make it with origin and time.
If we don't need to support them ever, origin and type (and ScriptExecutionContext) are enough, as you said.

For filesystem request before inspector disabled:
Even when Agent is disabled or Frontend detached, we'll store DOMFileSystem objects in Agent after this patch.
So inspector captures requests even before it opened.

Should we show a filesystem for a page which doesn't touch filesystem at all?
IMO, when we develop a site, it is enough that we can see the filesystem being used.

For InspectorInstrumentation::didCloseFileSystem:
I can't make it. InspectorInstrumentation dispatches the event to InstrumentingAgent using ScriptExecutionContext.
But when DOMFileSystem::OnContextDestroyed, ScriptExecutionContext is dying. So we can't reach InstrumentingAgent through it.
Comment 6 Taiju Tsuiki 2011-11-30 22:25:58 PST
Created attachment 117345 [details]
storage tree icon
Comment 7 Pavel Feldman 2011-12-11 09:40:29 PST
(In reply to comment #6)
> Created an attachment (id=117345) [details]
> storage tree icon

Vsevolod, could you please ping me when all of your comments / concerns are addressed so that I could proceed with the formal review?
Comment 8 Pavel Feldman 2011-12-11 09:41:00 PST
Comment on attachment 116887 [details]
Patch

Clearing r- as per Vsevolod's comments. It sounds like there is work to be done on the change.
Comment 9 Vsevolod Vlasov 2011-12-15 12:43:49 PST
> OK, so how about attached icon?
I think it's much better.

> >> Source/WebCore/fileapi/DOMFileSystem.cpp:60
> >> +    fileSystem->m_agentProvider = InspectorInstrumentation::didOpenFileSystem(fileSystem.get());
> > 
> > I have a strong concern about making that call from DOMFileSystem constructor/factory.
> > Looks like DOMFileSystem is some WebCore handle for the file system stored in the browser.
> > It is usually created when user does "requestWebkitFileSystem", but not only there, see http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=DOMFileSystem::create%5C(&type=cs
> > So there is no direct correlation between having file system for the given origin in the storage and creating DOMFileSystem object. (We can have a file system in the storage but never request it yet or we can have several DOMFileSystem handles for the same file system). 
> > I don't see why do you need to store pointers to all DOMFileSystem objects in inspector when all you really need is name, origin, and type.
> > 
> > As I understand your patch will show file systems in resources panel once web page calls "requestWebkitFileSystem()".
> > It will not even show anything if inspector is opened after they were requested.
> > 
> > I would expect, that upon opening of resources panel, all file systems available for page frame origins were shown no matter if web page already requested them or not.
> > And by file system I mean here a simple name-origin-type structure.
> > 
> > 
> > Also you should not store inspector related state here. I don't think you need that.
> > You can do something like
> > InspectorInstrumentation::didOpenFileSystem(fileSystem.get());
> > InspectorInstrumentation::didcloseFileSystem(fileSystem.get());
> > and these calls will go to the same InspectorFileSystemAgent.
> > You should get rid of all the BackPtr stuff and inspector related logic in DOMFileSystem.cpp
> 
> For storing DOMFileSystem object vs. origin and type:
> Currently, DOMFileSystem is created only in the call chain of webkitRequestFileSystem and webkitResolveLocalFileSystemURL.
> Though in planning of further extension of API, we'll probabily make filesystem without requestFileSystem and can't re-make it with origin and time.
> If we don't need to support them ever, origin and type (and ScriptExecutionContext) are enough, as you said.
I don't get it. I think name is enough to identify file system when you make requests from inspector and file system origin and type are the only information that we are going to show to developer before he opens FileSystemView. How will potential API changes affect that?

> For filesystem request before inspector disabled:
> Even when Agent is disabled or Frontend detached, we'll store DOMFileSystem objects in Agent after this patch.
> So inspector captures requests even before it opened.
I see. This is not correct. Inspector should not capture any information before it is opened. You should do 
  m_instrumentingAgents->setInspectorFileSystemAgent(this);
instead in InspectorFileSystemAgent::enable() which should be called explicitly from the frontend.
 
> Should we show a filesystem for a page which doesn't touch filesystem at all?
> IMO, when we develop a site, it is enough that we can see the filesystem being used.
I don't think so. Imagine we have a site and some pages already use file system, while others don't. When developer opens some new page he might want to see which file systems are already available for him.

> For InspectorInstrumentation::didCloseFileSystem:
> I can't make it. InspectorInstrumentation dispatches the event to InstrumentingAgent using ScriptExecutionContext.
> But when DOMFileSystem::OnContextDestroyed, ScriptExecutionContext is dying. So we can't reach InstrumentingAgent through it.
Looks like file system is only closed when the document is destroyed, so we can remove file system from inspector once the last frame with matching origin is detached / navigated.
Comment 10 Taiju Tsuiki 2012-02-19 20:35:29 PST
Created attachment 127753 [details]
Patch
Comment 11 Vsevolod Vlasov 2012-02-21 06:24:26 PST
Is this patch ready for review?
Comment 12 Taiju Tsuiki 2012-02-21 10:18:47 PST
No, it needs some refinement, including WebCore.xcodeproj update.
Comment 13 Taiju Tsuiki 2012-05-17 21:50:58 PDT
Created attachment 142628 [details]
Patch
Comment 14 Taiju Tsuiki 2012-05-17 21:56:18 PDT
Sorry for very very slow update for it. I'd like to resume to work on it.
I'll upload splitted version of this patch soon later.

Could you help me with reviewing these patches?
Comment 15 Pavel Feldman 2012-05-18 01:28:57 PDT
Making a step back to sync up on the plans. Is there a screenshot or a mock for this feature anywhere? Also, is filesystem implementation mature enough for inspection to be added?
Comment 16 Taiju Tsuiki 2012-05-18 02:35:00 PDT
> screenshot
Yes, a screen shot of previous version is available here:
https://bug-68203-attachments.webkit.org/attachment.cgi?id=116454


> matuarity of filesystem
I think it is enough mature, at least TEMPORARY and PERSISTENT FileSystem, which I put in scope.
Comment 17 Kinuko Yasuda 2012-05-21 21:42:00 PDT
(In reply to comment #16)
> > screenshot
> Yes, a screen shot of previous version is available here:
> https://bug-68203-attachments.webkit.org/attachment.cgi?id=116454

Pavel, how does this screenshot look to you?  This has been a long desired feature and we want to move this forward.

> > matuarity of filesystem
> I think it is enough mature, at least TEMPORARY and PERSISTENT FileSystem, which I put in scope.

Yes, it's been around for nearly 2 years, a lot of applications are using the API and I think we can safely assume it's mature enough now.
Comment 18 Pavel Feldman 2012-05-21 22:22:36 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > > screenshot
> > Yes, a screen shot of previous version is available here:
> > https://bug-68203-attachments.webkit.org/attachment.cgi?id=116454
> 
> Pavel, how does this screenshot look to you?  This has been a long desired feature and we want to move this forward.
> 

Could you remind me if there was reason you don't grow files tree under the filesystem node itself? Do you anticipate deep trees and lack of scrollbar in the sidebar concerns you?

Glad to hear you are willing to move this forward. Having this initial code hanging in the source tree for so long concerned me a bit.

> > > matuarity of filesystem
> > I think it is enough mature, at least TEMPORARY and PERSISTENT FileSystem, which I put in scope.
> 
> Yes, it's been around for nearly 2 years, a lot of applications are using the API and I think we can safely assume it's mature enough now.

Ok, looking at the patch now.
Comment 19 Pavel Feldman 2012-05-21 22:41:41 PDT
Comment on attachment 142628 [details]
Patch

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

> Source/WebCore/inspector/front-end/FileSystemModel.js:42
> +    WebInspector.resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.FrameNavigated, this._frameNavigated, this);

You should traverse existing frames here. I'm interested to learn why you would like to track frames. If you are only interested in origins, you could maintain listeners for FrameAdded, Navigated and Detached and every time event comes, model would traverse all frames recursively and collect remaining origins. That would be 10 lines of code in total and you would be operating only filesystems in the model. r- for while we are discussing it.

> Source/WebCore/inspector/front-end/FileSystemModel.js:107
> +    _isEmpty: function(obj)

This should go to the utilities.js and be declared on Object (i.e. Object.isEmpty(obj)).

> Source/WebCore/inspector/front-end/FileSystemModel.js:212
> + * @param {Page.Frame} frame

This is WebInspector.ResourceTreeFrame according to the call site. Try compiling this code using Source/WebCore/inspector/compile-frontend.sh with the recent closure compiler. Ignore the __proto__ assignment errors that started popping up in the latest compiler version.

> Source/WebCore/inspector/front-end/FileSystemModel.js:222
> +    this._filesystemModel.addFrameForOrigin(this, this._securityOrigin);

Why do you need to keep track of frames at all? Could you track only origins instead?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:70
> +    if (Capabilities.filesystemInspection) {

I'd call it Capabilities.filesystem

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1647
> +        this._filesystemModel = new WebInspector.FileSystemModel();

Since you create filesystem model lazily (which is good), you should traverse all existing frames in the tree in FileSystemModel constructor. Although I'd remove tracking of frames and left origins only.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1672
> +            if(this.children[i].filesystemName == filesystemName)

Space before ( is missing. We also use === for comparison.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2098
> +    var displayName = filesystem.type + " - " + filesystem.origin;

This should be WebInspector.UIString("%s - $s", filesystem.type, filesystem.origin);
You should also be consistent with naming: filesystem vs fileSystem.

> Source/WebKit/chromium/src/js/DevTools.js:47
> +    Capabilities.filesystemInspection = false;

Preferences are about enabling / disabling features when they are not yet ready.
Capabilities are for querying whether backend supports this feature or not. See other capabilities usages.

You start with Preferences when it is not ready at all. When it is already somewhat useful, you migrate to Capabilities and query backend for filesystem existence. You can then add "experiment" (see Settings.js) so that you could enable it at runtime.
Comment 20 Taiju Tsuiki 2012-05-22 20:30:52 PDT
Created attachment 143443 [details]
Patch
Comment 21 Taiju Tsuiki 2012-05-22 20:32:51 PDT
Created attachment 143444 [details]
Screenshot

Screenshot on Preferences.exposeFileSystemInspection = true
Comment 22 Taiju Tsuiki 2012-05-22 20:39:46 PDT
Comment on attachment 142628 [details]
Patch

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

>> Source/WebCore/inspector/front-end/FileSystemModel.js:42
>> +    WebInspector.resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.FrameNavigated, this._frameNavigated, this);
> 
> You should traverse existing frames here. I'm interested to learn why you would like to track frames. If you are only interested in origins, you could maintain listeners for FrameAdded, Navigated and Detached and every time event comes, model would traverse all frames recursively and collect remaining origins. That would be 10 lines of code in total and you would be operating only filesystems in the model. r- for while we are discussing it.

I'm tracking frame to associate requests with frame, and to migrate in-flight requests to another frame when a frame is detached.
Traversing frames seems to work well. Should I rewrite like that?

>> Source/WebCore/inspector/front-end/FileSystemModel.js:107
>> +    _isEmpty: function(obj)
> 
> This should go to the utilities.js and be declared on Object (i.e. Object.isEmpty(obj)).

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:212
>> + * @param {Page.Frame} frame
> 
> This is WebInspector.ResourceTreeFrame according to the call site. Try compiling this code using Source/WebCore/inspector/compile-frontend.sh with the recent closure compiler. Ignore the __proto__ assignment errors that started popping up in the latest compiler version.

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:222
>> +    this._filesystemModel.addFrameForOrigin(this, this._securityOrigin);
> 
> Why do you need to keep track of frames at all? Could you track only origins instead?

Pending

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:70
>> +    if (Capabilities.filesystemInspection) {
> 
> I'd call it Capabilities.filesystem

I moved it to Preferences and exposeFileSystemInspection referring other variable name.
If it was still not suitable, notice me.

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:1647
>> +        this._filesystemModel = new WebInspector.FileSystemModel();
> 
> Since you create filesystem model lazily (which is good), you should traverse all existing frames in the tree in FileSystemModel constructor. Although I'd remove tracking of frames and left origins only.

Pending

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:1672
>> +            if(this.children[i].filesystemName == filesystemName)
> 
> Space before ( is missing. We also use === for comparison.

Done

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:2098
>> +    var displayName = filesystem.type + " - " + filesystem.origin;
> 
> This should be WebInspector.UIString("%s - $s", filesystem.type, filesystem.origin);
> You should also be consistent with naming: filesystem vs fileSystem.

Done
I renamed filesystem to fileSystem.

>> Source/WebKit/chromium/src/js/DevTools.js:47
>> +    Capabilities.filesystemInspection = false;
> 
> Preferences are about enabling / disabling features when they are not yet ready.
> Capabilities are for querying whether backend supports this feature or not. See other capabilities usages.
> 
> You start with Preferences when it is not ready at all. When it is already somewhat useful, you migrate to Capabilities and query backend for filesystem existence. You can then add "experiment" (see Settings.js) so that you could enable it at runtime.

Make sence. I'll move it to Preferences this time.
Comment 23 Pavel Feldman 2012-05-23 01:53:05 PDT
> I'm tracking frame to associate requests with frame, and to migrate in-flight requests to another frame when a frame is detached.

Could you elaborate on this? What requests and why you want to associate them? Also, I thought that we no longer support detached frame use cases.
Comment 24 Pavel Feldman 2012-05-23 02:15:28 PDT
Comment on attachment 143443 [details]
Patch

Clearing r? while pending question in comment.
Comment 25 Taiju Tsuiki 2012-05-23 19:30:17 PDT
(In reply to comment #18)
> Could you remind me if there was reason you don't grow files tree under the filesystem node itself? Do you anticipate deep trees and lack of scrollbar in the sidebar concerns you?

Yes, I'd like to avoid deep tree in sidebar, as your suggestion.

(In reply to comment #23)
> > I'm tracking frame to associate requests with frame, and to migrate in-flight requests to another frame when a frame is detached.
> 
> Could you elaborate on this? What requests and why you want to associate them? Also, I thought that we no longer support detached frame use cases.

I tried to handle that: I'm going to post a request (e.g. readDirectory, readFile, getMetadata) through a frame.
The request is processed in the context of the frame. Howeven, the lifetime of a frame is up to the page,
 so the frame can be removed even if there are pending requests.
We can continue to process the request in another frame, if there is another frame with same origin.
So, I want to repost all pending request that posted through the removed frame.

Anyway, my frame tracking can be omitted if we don't care to remove frame or handle it in other layer.
Comment 26 Taiju Tsuiki 2012-05-23 19:47:06 PDT
Created attachment 143709 [details]
Patch
Comment 27 Taiju Tsuiki 2012-05-23 20:15:56 PDT
Comment on attachment 142628 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/FileSystemModel.js:222
>>> +    this._filesystemModel.addFrameForOrigin(this, this._securityOrigin);
>> 
>> Why do you need to keep track of frames at all? Could you track only origins instead?
> 
> Pending

Done

>>> Source/WebCore/inspector/front-end/ResourcesPanel.js:1647
>>> +        this._filesystemModel = new WebInspector.FileSystemModel();
>> 
>> Since you create filesystem model lazily (which is good), you should traverse all existing frames in the tree in FileSystemModel constructor. Although I'd remove tracking of frames and left origins only.
> 
> Pending

Done
Comment 28 Pavel Feldman 2012-05-24 10:00:03 PDT
(In reply to comment #25)
> (In reply to comment #18)
> > Could you remind me if there was reason you don't grow files tree under the filesystem node itself? Do you anticipate deep trees and lack of scrollbar in the sidebar concerns you?
> 
> Yes, I'd like to avoid deep tree in sidebar, as your suggestion.
> 

Ok!

> (In reply to comment #23)
> > > I'm tracking frame to associate requests with frame, and to migrate in-flight requests to another frame when a frame is detached.
> > 
> > Could you elaborate on this? What requests and why you want to associate them? Also, I thought that we no longer support detached frame use cases.
> 
> I tried to handle that: I'm going to post a request (e.g. readDirectory, readFile, getMetadata) through a frame.

What do you mean through a frame? Don't you do that natively?

> The request is processed in the context of the frame. Howeven, the lifetime of a frame is up to the page,
>  so the frame can be removed even if there are pending requests.
> We can continue to process the request in another frame, if there is another frame with same origin.
> So, I want to repost all pending request that posted through the removed frame.
> 

I think this adds unnecessary complexity to the code. I'd rather start with no frames collection.

> Anyway, my frame tracking can be omitted if we don't care to remove frame or handle it in other layer.
Comment 29 Taiju Tsuiki 2012-05-24 21:49:21 PDT
(In reply to comment #28)
> 
> > (In reply to comment #23)
> > > > I'm tracking frame to associate requests with frame, and to migrate in-flight requests to another frame when a frame is detached.
> > > 
> > > Could you elaborate on this? What requests and why you want to associate them? Also, I thought that we no longer support detached frame use cases.
> > 
> > I tried to handle that: I'm going to post a request (e.g. readDirectory, readFile, getMetadata) through a frame.
> 
> What do you mean through a frame? Don't you do that natively?

I mean "using its Document as ScriptExecutionContext for DOMFileSystem".
OK, I droped it from frontend and will put it in backend if needed in future.

> 
> > The request is processed in the context of the frame. Howeven, the lifetime of a frame is up to the page,
> >  so the frame can be removed even if there are pending requests.
> > We can continue to process the request in another frame, if there is another frame with same origin.
> > So, I want to repost all pending request that posted through the removed frame.
> > 
> 
> I think this adds unnecessary complexity to the code. I'd rather start with no frames collection.

OK, I removed it in #26 patch.
Comment 30 Kinuko Yasuda 2012-06-04 03:51:49 PDT
The patch seems ready for another review.  Pavel, would you be able to take a look at it or CC a person who could review?  Thanks,
Comment 31 Taiju Tsuiki 2012-06-05 22:53:03 PDT
*** Bug 86818 has been marked as a duplicate of this bug. ***
Comment 32 Vsevolod Vlasov 2012-06-06 06:39:13 PDT
Comment on attachment 143709 [details]
Patch

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

Please add new files to WebCore.xcodeproj/project.pbxproj as well.
Could you please upload a patch with a ChangeLog next time?

> Source/WebCore/inspector/front-end/FileSystemModel.js:44
> +    this._pendingReadDirectoryRequests = {};

Unused, please remove.

> Source/WebCore/inspector/front-end/FileSystemModel.js:93
> +        if (frame.securityOrigin == "null")

Please use ===.

> Source/WebCore/inspector/front-end/FileSystemModel.js:98
> +        var new_origin = false;

newOrigin

> Source/WebCore/inspector/front-end/FileSystemModel.js:129
> +        var last_origin = this._frameIdsForOrigin[origin].isEmpty();

lastOrigin

> Source/WebCore/inspector/front-end/FileSystemModel.js:151
> +        if (!this._fileSystemsForOrigin[origin])

How can this._fileSystemsForOrigin[origin] be already defined here?
This origin was either never added or already removed.

> Source/WebCore/inspector/front-end/FileSystemModel.js:156
> +            if (!this._fileSystemsForOrigin[origin][types[i]]) {

Again, how can this._fileSystemsForOrigin[origin][types[i]] be already defined here?

> Source/WebCore/inspector/front-end/FileSystemModel.js:205
> +    readDirectory: function(directory, callback)

Let's move this method and Entry, Directory and File classes to another patch, they are not really related to adding file system item to storage tree.

> Source/WebCore/inspector/front-end/FileSystemModel.js:216
> +WebInspector.FileSystemModel.nextRequestId = 1;

Unused, please remove.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:70
> +    if (Preferences.exposeFileSystemInspection) {

While this feature is not implemented yet it should be hidden behind experimental flag.
See WebInspector.ExperimentsSettings constructor in Settings.js for usage examples.
You should then run chromium with --enable-devtools-experiments flag and enable experiment in settings pane.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1512
> +        if (this.expanded)

This should be removed since TreeOutline will call onexpand after attaching an expanded node.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1550
> +    refreshFileSystem: function()

Looks like this method is not used outside of this class, please make it private.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2004
> +        if (!this._fileSystemView)

if (this._fileSystemView)

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2012
> +            if (!WebInspector.FileSystemView) {

How is that possible? Seems redundant.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:2023
> +        if (this.fileSystemView && this._storagePanel.visibleView == this.fileSystemView)

You should not need this. When file system node is removed another node will be selected and its view shown automatically.
You might need to let fileSystemView clear some of its state here later though.

> Source/WebCore/inspector/front-end/Settings.js:48
> +    exposeFileSystemInspection: true

Preferences.exposeFileSystemInspection should be false by default.
You should also enable it for chromium port in Source/Webkit/chromium/src/js/DevTools.js

> Source/WebCore/inspector/front-end/utilities.js:680
> +(function() {

I don't think you need a function expression here.
Just define a property on Object.prototype (e.g. like keySet is defined on Array.prototype above)

> Source/WebCore/inspector/front-end/utilities.js:681
> +var objectPrototype = /** @type {!Object} */ Object.prototype;

Please inline.

> Source/WebCore/inspector/front-end/utilities.js:693
> +Object.defineProperty(objectPrototype, "anyOf",

I don't think this is used, please remove.
Comment 33 Vsevolod Vlasov 2012-06-06 06:48:05 PDT
One more thing: I see that you unconditionally add two file systems for each security origin. This could clutter FileSystem node. 

Is there a way to detect and skip "null" file systems meaning file systems that were never used for certain security origins?
I am not saying empty because even an attempt to read from filesystem makes it valuable to show it to developer, but what about filesystems that nobody even tried to read?
This is not something that is needed for the first version but we should do it at some point if possible.
Comment 34 Taiju Tsuiki 2012-06-06 21:58:50 PDT
Created attachment 146196 [details]
Patch
Comment 35 Taiju Tsuiki 2012-06-06 22:28:01 PDT
Comment on attachment 143709 [details]
Patch

Thank you for reviewing!
Sorry, I put too much in previous patch.

I packed necessary part to add storage tree item in this patch.
Could you take another look?

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

>> Source/WebCore/inspector/front-end/FileSystemModel.js:44
>> +    this._pendingReadDirectoryRequests = {};
> 
> Unused, please remove.

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:93
>> +        if (frame.securityOrigin == "null")
> 
> Please use ===.

Done, thanks!

>> Source/WebCore/inspector/front-end/FileSystemModel.js:98
>> +        var new_origin = false;
> 
> newOrigin

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:129
>> +        var last_origin = this._frameIdsForOrigin[origin].isEmpty();
> 
> lastOrigin

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:151
>> +        if (!this._fileSystemsForOrigin[origin])
> 
> How can this._fileSystemsForOrigin[origin] be already defined here?
> This origin was either never added or already removed.

It's from old code. Removed.

>> Source/WebCore/inspector/front-end/FileSystemModel.js:156
>> +            if (!this._fileSystemsForOrigin[origin][types[i]]) {
> 
> Again, how can this._fileSystemsForOrigin[origin][types[i]] be already defined here?

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:205
>> +    readDirectory: function(directory, callback)
> 
> Let's move this method and Entry, Directory and File classes to another patch, they are not really related to adding file system item to storage tree.

Done

>> Source/WebCore/inspector/front-end/FileSystemModel.js:216
>> +WebInspector.FileSystemModel.nextRequestId = 1;
> 
> Unused, please remove.

Done

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:70
>> +    if (Preferences.exposeFileSystemInspection) {
> 
> While this feature is not implemented yet it should be hidden behind experimental flag.
> See WebInspector.ExperimentsSettings constructor in Settings.js for usage examples.
> You should then run chromium with --enable-devtools-experiments flag and enable experiment in settings pane.

Done

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:1512
>> +        if (this.expanded)
> 
> This should be removed since TreeOutline will call onexpand after attaching an expanded node.

Done

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:1550
>> +    refreshFileSystem: function()
> 
> Looks like this method is not used outside of this class, please make it private.

Done

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:2004
>> +        if (!this._fileSystemView)
> 
> if (this._fileSystemView)

Done

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:2012
>> +            if (!WebInspector.FileSystemView) {
> 
> How is that possible? Seems redundant.

Removed

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:2023
>> +        if (this.fileSystemView && this._storagePanel.visibleView == this.fileSystemView)
> 
> You should not need this. When file system node is removed another node will be selected and its view shown automatically.
> You might need to let fileSystemView clear some of its state here later though.

OK, I removed this time.

>> Source/WebCore/inspector/front-end/Settings.js:48
>> +    exposeFileSystemInspection: true
> 
> Preferences.exposeFileSystemInspection should be false by default.
> You should also enable it for chromium port in Source/Webkit/chromium/src/js/DevTools.js

Thanks. Done.

>> Source/WebCore/inspector/front-end/utilities.js:680
>> +(function() {
> 
> I don't think you need a function expression here.
> Just define a property on Object.prototype (e.g. like keySet is defined on Array.prototype above)

Done

>> Source/WebCore/inspector/front-end/utilities.js:681
>> +var objectPrototype = /** @type {!Object} */ Object.prototype;
> 
> Please inline.

Closure compiler can't recognize Object.prototype as an Object. Should I add new class for it? or ignore its warning?

>> Source/WebCore/inspector/front-end/utilities.js:693
>> +Object.defineProperty(objectPrototype, "anyOf",
> 
> I don't think this is used, please remove.

Done
Comment 36 Taiju Tsuiki 2012-06-07 02:19:32 PDT
(In reply to comment #33)
> One more thing: I see that you unconditionally add two file systems for each security origin. This could clutter FileSystem node. 
> 
> Is there a way to detect and skip "null" file systems meaning file systems that were never used for certain security origins?
Yes, we can detect it at least in chrome.

> I am not saying empty because even an attempt to read from filesystem makes it valuable to show it to developer, but what about filesystems that nobody even tried to read?
> This is not something that is needed for the first version but we should do it at some point if possible.

I agree. I'm showing too many unused FS on the tree, 2 for tweet button, 2 for like button, 2 for +1 button.
Comment 37 Vsevolod Vlasov 2012-06-07 03:13:34 PDT
WebCore.xcodeproj/project.pbxproj change is still missing.
Comment 38 Vsevolod Vlasov 2012-06-07 03:18:58 PDT
Comment on attachment 146196 [details]
Patch

Please add new files to WebCore.xccodeproj and rename filesystem.png to fileSystem.png. Other than that looks good.
Comment 39 Vsevolod Vlasov 2012-06-07 03:20:19 PDT
(In reply to comment #36)
> (In reply to comment #33)
> > One more thing: I see that you unconditionally add two file systems for each security origin. This could clutter FileSystem node. 
> > 
> > Is there a way to detect and skip "null" file systems meaning file systems that were never used for certain security origins?
> Yes, we can detect it at least in chrome.
Please file a bug for that and add a FIXME with its number to the patch.
Comment 40 Taiju Tsuiki 2012-06-07 18:42:40 PDT
Created attachment 146446 [details]
Patch
Comment 41 Taiju Tsuiki 2012-06-07 18:59:58 PDT
(In reply to comment #37)
> WebCore.xcodeproj/project.pbxproj change is still missing.

WebCore.xcodeproj/project.pbxproj seems not to contain other resources like applicationCache.png.
I think .png is managed automatically.

(In reply to comment #39)
> (In reply to comment #36)
> > (In reply to comment #33)
> > > One more thing: I see that you unconditionally add two file systems for each security origin. This could clutter FileSystem node. 
> > > 
> > > Is there a way to detect and skip "null" file systems meaning file systems that were never used for certain security origins?
> > Yes, we can detect it at least in chrome.
> Please file a bug for that and add a FIXME with its number to the patch.

OK, I filed it as http://webkit.org/b/88602 .
Comment 42 Vsevolod Vlasov 2012-06-08 01:56:28 PDT
(In reply to comment #41)
> (In reply to comment #37)
> > WebCore.xcodeproj/project.pbxproj change is still missing.
> 
> WebCore.xcodeproj/project.pbxproj seems not to contain other resources like applicationCache.png.
> I think .png is managed automatically.
Sorry for that, you don't need to update xcode project file here.
Comment 43 WebKit Review Bot 2012-06-08 02:50:43 PDT
Comment on attachment 146446 [details]
Patch

Clearing flags on attachment: 146446

Committed r119815: <http://trac.webkit.org/changeset/119815>
Comment 44 WebKit Review Bot 2012-06-08 02:50:51 PDT
All reviewed patches have been landed.  Closing bug.