Bug 72691 – [Inspector][FileSystem] Add FileSystem item to storage tree

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

[Taiju Tsuiki]

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.

[Taiju Tsuiki]

Created attachment 115742 [details]
Patch

[Taiju Tsuiki]

Created attachment 116767 [details]
Patch (not ready)

[Taiju Tsuiki]

Created attachment 116887 [details]
Patch

[Vsevolod Vlasov]

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
> [phone]  Taiju TSUIKI  <[e-mail]>

Looks like you have a duplicate change log entry.

[Taiju Tsuiki]

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.

[Taiju Tsuiki]

Created attachment 117345 [details]
storage tree icon

[Pavel Feldman]

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

[Pavel Feldman]

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.

[Vsevolod Vlasov]

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

[Taiju Tsuiki]

Created attachment 127753 [details]
Patch

[Vsevolod Vlasov]

Is this patch ready for review?

[Taiju Tsuiki]

No, it needs some refinement, including WebCore.xcodeproj update.

[Taiju Tsuiki]

Created attachment 142628 [details]
Patch

[Taiju Tsuiki]

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?

[Pavel Feldman]

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?

[Taiju Tsuiki]

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

[Kinuko Yasuda]

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

[Pavel Feldman]

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

[Pavel Feldman]

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.

[Taiju Tsuiki]

Created attachment 143443 [details]
Patch

[Taiju Tsuiki]

Created attachment 143444 [details]
Screenshot

Screenshot on Preferences.exposeFileSystemInspection = true

[Taiju Tsuiki]

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.

[Pavel Feldman]

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

[Pavel Feldman]

Comment on attachment 143443 [details]
Patch

Clearing r? while pending question in comment.

[Taiju Tsuiki]

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

[Taiju Tsuiki]

Created attachment 143709 [details]
Patch

[Taiju Tsuiki]

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

[Pavel Feldman]

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

[Taiju Tsuiki]

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

[Kinuko Yasuda]

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,

[Taiju Tsuiki]

*** Bug 86818 has been marked as a duplicate of this bug. ***

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

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.

[Taiju Tsuiki]

Created attachment 146196 [details]
Patch

[Taiju Tsuiki]

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

[Taiju Tsuiki]

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

[Vsevolod Vlasov]

WebCore.xcodeproj/project.pbxproj change is still missing.

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

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

[Taiju Tsuiki]

Created attachment 146446 [details]
Patch

[Taiju Tsuiki]

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

[Vsevolod Vlasov]

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

[WebKit Review Bot]

Comment on attachment 146446 [details]
Patch

Clearing flags on attachment: 146446

Committed r119815: <http://trac.webkit.org/changeset/119815>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.