Bug 45982 – Web Inspector: FileSystem integration

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

[Kavita Kanetkar]

http://dev.w3.org/2009/dap/file-system/file-dir-sys.html needs integration with inspector.

[Kavita Kanetkar]

Created attachment 69873 [details]
patch

These are the WebKit API/WebCore changes for integration.

[Kinuko Yasuda]

Comment on attachment 69873 [details]
patch

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

> WebCore/ChangeLog:23
> +        * inspector/InspectorFileSystemAgent.cpp: Added.

I think for newly added files you can omit the detailed methods list below.

> WebCore/ChangeLog:43
> +        * inspector/InspectorFileSystemAgent.h: Added.

ditto.

> WebCore/ChangeLog:51
> +        * inspector/front-end/FileSystemView.js: Added. Handles display in storage tab for "FILE SYSTEM"

ditto.

> WebCore/platform/AsyncFileSystem.h:122
> +    virtual String root() const { return m_platformRootPath; }

virtual const String& root() const { return m_platformRootPath; }

does this need to be virtual?

> WebCore/inspector/InspectorFileSystemAgent.h:54
> +    void getTempFileSystem();

Please use 'Temporary' instead of 'Temp'.  (Seems like we do not use abbreviate names in WebKit)

> WebCore/inspector/InspectorFileSystemAgent.h:59
> +    void didGetFileSystemError(AsyncFileSystem::Type type);

Please remove parameter names that add no info (type)

> WebCore/inspector/InspectorFileSystemAgent.h:63
> +    void getFileSystemRoot(AsyncFileSystem::Type type);

ditto. (Please remove parameter names that add no info (type))

> WebCore/inspector/InspectorFileSystemAgent.h:68
> +    String m_tempRoot;

s/m_tempRoot/m_temporaryRoot/

> WebCore/inspector/InspectorController.cpp:110
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)

We use ENABLE(INSPECTOR) && ENABLE(FILE_SYSTEM) in InspectorFileSystemAgent.h and OFFLINE_WEB_APPLICATIONS here.  Is it intentional?

> WebCore/inspector/InspectorFileSystemAgent.cpp:44
> +InspectorFileSystemAgentCallbacks(InspectorFileSystemAgent* agent, AsyncFileSystem::Type type)

Indentation? (here and all the methods of InspectorFileSystemAgentCallbacks below.)

> WebCore/inspector/InspectorFileSystemAgent.cpp:58
> +    // Agent will be alive even if InspectorController is destroyed.

...until it gets called back.

> WebCore/inspector/InspectorFileSystemAgent.cpp:99
> +InspectorFileSystemAgent::~InspectorFileSystemAgent() { }

I think we usually write this in three lines like:

InspectorFileSystemAgent::~InspectorFileSystemAgent()
{
}

> WebCore/inspector/InspectorFileSystemAgent.cpp:127
> +{

Can we be sure that m_inspectorController is not null here?

> WebCore/inspector/InspectorFileSystemAgent.cpp:146
> +            if (m_persistentRoot.isEmpty())

In which case m_persistentRoot can be non-empty?
And in either case we want to call didGetPersistentFileSystem with the given 'root' path?
Don't we need to check (or ASSERT?) if root == m_persistentRoot or not?

> WebCore/inspector/InspectorFileSystemAgent.cpp:163
> +            m_frontend->didGetPersistentFileSystem("Error while retrieving root");

How can js check the returned string is not a root path but an error message?

[Kinuko Yasuda]

Mostly only for c++ part, and many of them are just nits (style issues).

(In reply to comment #2)
> (From update of attachment 69873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69873&action=review
> 
> > WebCore/ChangeLog:23
> > +        * inspector/InspectorFileSystemAgent.cpp: Added.
> 
> I think for newly added files you can omit the detailed methods list below.
> 
> > WebCore/ChangeLog:43
> > +        * inspector/InspectorFileSystemAgent.h: Added.
> 
> ditto.
> 
> > WebCore/ChangeLog:51
> > +        * inspector/front-end/FileSystemView.js: Added. Handles display in storage tab for "FILE SYSTEM"
> 
> ditto.
> 
> > WebCore/platform/AsyncFileSystem.h:122
> > +    virtual String root() const { return m_platformRootPath; }
> 
> virtual const String& root() const { return m_platformRootPath; }
> 
> does this need to be virtual?
> 
> > WebCore/inspector/InspectorFileSystemAgent.h:54
> > +    void getTempFileSystem();
> 
> Please use 'Temporary' instead of 'Temp'.  (Seems like we do not use abbreviate names in WebKit)
> 
> > WebCore/inspector/InspectorFileSystemAgent.h:59
> > +    void didGetFileSystemError(AsyncFileSystem::Type type);
> 
> Please remove parameter names that add no info (type)
> 
> > WebCore/inspector/InspectorFileSystemAgent.h:63
> > +    void getFileSystemRoot(AsyncFileSystem::Type type);
> 
> ditto. (Please remove parameter names that add no info (type))
> 
> > WebCore/inspector/InspectorFileSystemAgent.h:68
> > +    String m_tempRoot;
> 
> s/m_tempRoot/m_temporaryRoot/
> 
> > WebCore/inspector/InspectorController.cpp:110
> > +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> 
> We use ENABLE(INSPECTOR) && ENABLE(FILE_SYSTEM) in InspectorFileSystemAgent.h and OFFLINE_WEB_APPLICATIONS here.  Is it intentional?
> 
> > WebCore/inspector/InspectorFileSystemAgent.cpp:44
> > +InspectorFileSystemAgentCallbacks(InspectorFileSystemAgent* agent, AsyncFileSystem::Type type)
> 
> Indentation? (here and all the methods of InspectorFileSystemAgentCallbacks below.)
> 
> > WebCore/inspector/InspectorFileSystemAgent.cpp:58
> > +    // Agent will be alive even if InspectorController is destroyed.
> 
> ...until it gets called back.
> 
> > WebCore/inspector/InspectorFileSystemAgent.cpp:99
> > +InspectorFileSystemAgent::~InspectorFileSystemAgent() { }
> 
> I think we usually write this in three lines like:
> 
> InspectorFileSystemAgent::~InspectorFileSystemAgent()
> {
> }
> 
> > WebCore/inspector/InspectorFileSystemAgent.cpp:127
> > +{
> 
> Can we be sure that m_inspectorController is not null here?
> 
> > WebCore/inspector/InspectorFileSystemAgent.cpp:146
> > +            if (m_persistentRoot.isEmpty())
> 
> In which case m_persistentRoot can be non-empty?
> And in either case we want to call didGetPersistentFileSystem with the given 'root' path?
> Don't we need to check (or ASSERT?) if root == m_persistentRoot or not?
> 
> > WebCore/inspector/InspectorFileSystemAgent.cpp:163
> > +            m_frontend->didGetPersistentFileSystem("Error while retrieving root");
> 
> How can js check the returned string is not a root path but an error message?

[Joseph Pecoraro]

Comment on attachment 69873 [details]
patch

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

> WebCore/inspector/front-end/StoragePanel.js:677
> +    this._fileSystemDomain = fileSystemDomain;
> +    this._subtitle = "";
> +    this._mainTitle = this._fileSystemDomain;
> +    this.refreshTitles();
> +}

Since a round of NITs just went out I thought I would mention this.
"application-cache-sidebar-tree-item" should not apply to this as well.
Please make a new CSS class for this. Also, note that this class is only
used to style "application-cache-sidebar-tree-item .icon", so you might
want to put a FIXME to include a new icon for File System related items.

Could you attach a screenshot of the UI for this?

[Joseph Pecoraro]

Apparently my comment was missing the important line:

> +    WebInspector.SidebarTreeElement.call(this, "application-cache-sidebar-tree-item", fileSystemDomain, "", null, false);

[Pavel Feldman]

Comment on attachment 69873 [details]
patch

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

> WebKit/chromium/src/ChromiumBridge.cpp:345
> +    webKitClient()->fileUtilities()->openInFolder(path);

"revealOSFolder" or "revealFolderInOS"? Otherwise I might think that something is opening in the folder...

> WebKit/chromium/ChangeLog:5
> +        Web Inspector: FileSystem integration

How come Joe did not mention - please add more information here.

>> WebCore/platform/AsyncFileSystem.h:122
>> +    virtual String root() const { return m_platformRootPath; }
> 
> virtual const String& root() const { return m_platformRootPath; }
> 
> does this need to be virtual?

@Kinuko Yasuda: I am usually opposed to returning const references to internals (see abarth's recent message). Is there a big performance win in the case?

> WebCore/inspector/InspectorFileSystemAgent.h:51
> +    void getPersistentFileSystem();

Given that this returns nothing, "get" prefix does no seem right. "request" or "get*Async" would look better.

> WebCore/inspector/InspectorFileSystemAgent.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY

Here and in other new files copyright notices, please use Google's header, not Apple's header with Google as a company name above.

> WebCore/inspector/InspectorFileSystemAgent.cpp:89
> +    InspectorFileSystemAgent* m_agent;

There is a chance inspector is already closed/destroyed by the time you get the callback. You should store RefPtr here. I can see that you make sure that InspectorFileSystemAgent behaves reasonably (does noop) after the call to ::stop which is right.

> WebCore/inspector/InspectorFileSystemAgent.cpp:130
> +        didGetFileSystem(root, type);

It'd be great if we could make these did* methods private. It might be tricky due to the call from the async callback and might require moving its declaration to the private: section in InspectorFileSystem.h, but it would make our life safer.

> WebCore/inspector/InspectorFileSystemAgent.cpp:144
> +    if (m_inspectorController) {

Nit: replace nested conditions with guard expressions to avoid nesting:
if (!m_inspectorController)
    return;

> WebCore/inspector/Inspector.idl:204
> +        [handler=FileSystem] void getPersistentFileSystem();

Should we pass the root type notion to the frontend instead of having 2 methods per each action? getFileSystem(FileSystemType) ? Also, this method is not getting FileSystem, it is getting the RootFolderPath, right? Given that the domain already stands for "FileSystem", we can call it getRootPath(FileSystemType).

> WebCore/inspector/Inspector.idl:205
> +        [handler=FileSystem] void getTempFileSystem();

No abbreviations in WebKit please.

> WebCore/inspector/front-end/FileSystemView.js:36
> +    this.tabbedPane = new WebInspector.TabbedPane(this.element);

Is there a reason this should be public (not have "_" prefix) ? Could you attach screenshot please?

> WebCore/inspector/front-end/FileSystemView.js:38
> +    this.persistentFileSystemElement = document.createElement("div");

ditto

> WebCore/inspector/front-end/FileSystemView.js:40
> +    this.tabbedPane.appendTab("persistent", WebInspector.UIString("Persistent File System"), this.persistentFileSystemElement, this._selectFileSystemTab.bind(this, true));

I don't see modifier localizedStrings.js file in this change set.

> WebCore/inspector/front-end/FileSystemView.js:42
> +    this.tempFileSystemElement = document.createElement("div");

ditto

> WebCore/inspector/front-end/FileSystemView.js:54
> +         WebInspector.FileSystem.openPersistentFileSystemFolder();

Not sure you need so much delegation, inline this.

> WebCore/inspector/front-end/FileSystemView.js:59
> +        WebInspector.FileSystem.openTempFileSystemFolder();

ditto

> WebCore/inspector/front-end/FileSystemView.js:115
> +        fileSystemElement.removeChildren();

Screenshot please!

> WebCore/inspector/front-end/FileSystemView.js:137
> +                this.enableButton.addEventListener("click", this._persistentFileSystemSelected.bind(this), false);

You can even bind this straight to InspectorBackend.openPersistentFileSystemFolder

> WebCore/inspector/front-end/StoragePanel.js:674
> +    this._subtitle = "";

Third constructor parameter already assigned fileSystemDomain to mainTitle. Nuke it.

> WebCore/inspector/front-end/StoragePanel.js:675
> +    this._mainTitle = this._fileSystemDomain;

Second constructor parameter already assigned fileSystemDomain to mainTitle. Nuke it.

> WebCore/inspector/front-end/StoragePanel.js:676
> +    this.refreshTitles();

Titles are already up-to-date.

> WebCore/inspector/front-end/StoragePanel.js:682
> +        WebInspector.panels.storage.showFileSystem(this, this._fileSystemDomain);

Are we subclassing for the sake of "select" only? Rest seems to be generic implementation. Can we get away with listener instead?

> WebCore/inspector/front-end/StoragePanel.js:685
> +    get mainTitle()

This is no different form the default behavior you already inherited. Nuke it.

> WebCore/inspector/front-end/StoragePanel.js:690
> +    set mainTitle(x)

ditto

> WebCore/inspector/front-end/StoragePanel.js:696
> +    get subtitle()

ditto

> WebCore/inspector/front-end/StoragePanel.js:701
> +    set subtitle(x)

ditto

> WebCore/inspector/front-end/DOMAgent.js:486
> +WebInspector.FileSystem = {}

FileSystem is not related to DOMAgent, should not belong here. Move to FileSystemView.js

> WebCore/inspector/front-end/inspector.js:1407
> +WebInspector.didGetPersistentFileSystem = function(root)

Move these to FileSystemView.js please.

> WebCore/inspector/InspectorController.h:201
> +    InspectorFileSystemAgent* fileSystemAgent() { return m_fileSystemAgent.get(); }

Strictly speaking, you don't need getters here - generator makes code that has access to backend's privates.

[Kinuko Yasuda]

(In reply to comment #6)
> (From update of attachment 69873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69873&action=review
> >> WebCore/platform/AsyncFileSystem.h:122
> >> +    virtual String root() const { return m_platformRootPath; }
> > 
> > virtual const String& root() const { return m_platformRootPath; }
> > 
> > does this need to be virtual?
> 
> @Kinuko Yasuda: I am usually opposed to returning const references to internals (see abarth's recent message). Is there a big performance win in the case?

I usually return a const reference if it's reasonable (I'm positive this code is safe) but on the second thought with WebCore's String it won't change anything... if you don't like to do that please never mind.

[Timothy Hatcher]

Please post a screenshot.

[Kavita Kanetkar]

Created attachment 70320 [details]
Screenshot for filesystem devtools

This is currently disabled through preferences for non-chromium.

[Kavita Kanetkar]

Created attachment 70330 [details]
patch2

I lost all my detailed review comments since I didn't hit 'Publish'.
I have made most of the changes and added FIXME for a few where it required more investigation: Such as moving didGetFileSystemPath callback to FileSystemView.js.

Currently like other callbacks, inspector.js gets it which delegates to panels.storage which then delegates to FileSystemView. 

Also direct binding of buttons to InspectorBackend.revealFolderInOS didn't work, so I am keeping the indirection as-is.

Made all other changes in InspectorFileSystemAgent: Consolidated 2 get* and didGet methods to 1 get*Async and didGet* which take in type as a parameter to separate persistent/temporary filesystem.

Few questions: Added FIXME for icon. Currently I am using application-cache icon.  Should I create one by myself and add it here?
Another question was for Pavel in regards to your review comment, " I don't see modifier localizedStrings.js file in this change set"
Where is the place I need to add it?

Thanks for the review!

[Joseph Pecoraro]

> Few questions: Added FIXME for icon. Currently I am using application-cache icon.  Should I create one by myself and add it here?

Typically Timothy Hatcher creates icons, as he is awesome at it. But, if
you have the skills, or if you want to make one that is somewhat subtly
different that should be fine. I think I made the AppCache icon by just
changing the colors from one of the others... I vaguely recall doing that.

> Another question was for Pavel in regards to your review comment, " I don't see modifier localizedStrings.js file in this change set"
> Where is the place I need to add it?

See: "WebCore/English.lproj/localizedStrings.js"
Add the new localized string at the bottom. It should than show up in
the svn diff as a "binary" file change. If you use git, adding --binary to
`git diff` will make it show up.

[Timothy Hatcher]

What does the "Persistent Filesystem" and "Temporary Filesystem" tabs do/show?

[Kavita Kanetkar]

Created attachment 70385 [details]
patch3 - corrected minor indents.

I will add a separate patch for localizedStrings.js since svn isn't recognizing the diff. Will add as a follow-up to this.
Even setting as text mime type didn't give me the diff.

@Timothy The 2 tabs show the root of file system for the FileAPI and a "Reveal folder in OS" button to browse the paths.

[Pavel Feldman]

Few comments on the UI:

- Given the amount of information provided, you don't need tabbed pane. Just do the tree as in the Headers tab of resource view:

  v Persistent File System
     Root: <full path here>
  v Temporary File System
     Root: <full path here>

- Please use padding similar to the Headers tab (otherwise your text is too close to the view frame).

- Also, you might want to convert these file names into the links witch custom handlers that reveal corresponding folder in the OS folder. We might want to live with links only (no buttons) if it proves to be usable.

[Pavel Feldman]

Comment on attachment 70385 [details]
patch3 - corrected minor indents.

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

Looks good. Few things to fix in code and UI before we land it!

> WebCore/inspector/InspectorFileSystemAgent.cpp:129
> +    AsyncFileSystem::Type asyncFileSystemType = static_cast<AsyncFileSystem::Type>(type);

This really should be handled outside, but that is the generator's problem, I understand.

> WebCore/inspector/InspectorFileSystemAgent.cpp:162
> +    m_frontend->didGetFileSystemPath("Error while retrieving root", static_cast<unsigned int>(type));

Nit: This is not exactly a 'path' you are passing in here. You should add a separate 'success' / 'errorMessage' out parameter to report about errors.

> WebCore/inspector/front-end/FileSystemView.js:49
> +    this._tabbedPane.appendTab("persistent", WebInspector.UIString("Persistent File System"), this._persistentFileSystemElement, this._selectFileSystemTab.bind(this, true));

Still no localizedStrings.js file changed.

> WebCore/inspector/front-end/FileSystemView.js:88
> +        WebInspector.FileSystem.getFileSystemPathAsync(this.fileSystemType.PERSISTENT);

Nit: given that these are always called next to each other, should we return array of objects?

[Kavita Kanetkar]

Created attachment 71335 [details]
patch3

Changing UI needs a rewrite to FileSystem.js. Will be a fixme for this patch.
Also, I will need to change type of localizedStrings.js to plain text land it, add strings from FileSystem.js and re-change the type.
I have a separate patch for that. Rest all nits addressed. Again, this is disabled for non-chromium platforms.

Thanks for the review.

[Kavita Kanetkar]

Created attachment 71373 [details]
patch3 + separated error and success cases in js file.

[Pavel Feldman]

Comment on attachment 71373 [details]
patch3 + separated error and success cases in js file.

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

> WebCore/WebCore.gypi:4382
> +            'inspector/front-end/FileSystemView.js',

Please also include this file into vcproj.

> WebCore/inspector/InspectorFileSystemAgent.cpp:36
> +#include "AsyncFileWriter.h"

You could use forward declaration instead.

> WebCore/inspector/InspectorFileSystemAgent.cpp:124
> +void InspectorFileSystemAgent::getFileSystemPathAsync(unsigned int type)

So there is no domain query parameter which seems wrong. There is none for cookies since Joe was collecting cookies for all domain and sorting them on the front-end. You should do it similarly or add a query parameter.

> WebCore/inspector/InspectorFileSystemAgent.cpp:126
> +    if (!m_inspectorController)

There always is inspector controller if we get here.

> WebCore/inspector/InspectorFileSystemAgent.cpp:140
> +        LocalFileSystem::localFileSystem().requestFileSystem(document, asyncFileSystemType, 0, new InspectorFileSystemAgentCallbacks(this, asyncFileSystemType));

You probably need to look up a document with matching domain here.

> WebCore/inspector/front-end/FileSystemView.js:44
> +    this._domain = fileSystemDomain;

You seem to be ignoring domain and as a result, you are going to show main document's filesystem for all domains.

> WebCore/inspector/front-end/StoragePanel.js:661
> +WebInspector.FileSystemSidebarTreeElement = function(fileSystemDomain)

You need to rebase since we now use different base classes for tree elements in Storage panel.

> WebCore/inspector/front-end/inspector.js:1245
> +                this._addFileSystemDomain(parsedURL.host);

Nit: We might navigate off one of the domains (via iframe navigation). As a result, we should clean up corresponding storage entries. Cookies, AppCache and now FileSystem - they all suffer from this problem.

[Kavita Kanetkar]

Created attachment 71519 [details]
patch4

Addressed comments reg domain filtering. This only works for main frame. Put additional check that requesting URLs domain matches
that of the main frame's document's host.

Thanks for the review.

[Michael Nordman]

Comment on attachment 71519 [details]
patch4

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

I think you should rework the interfaces to reflect the per securityOrigin nature of the FileSystem. Having it work for the main page only to start with is fine, but having more flexible interface in place would be good.

> WebCore/inspector/CodeGeneratorInspector.pm:49
> +    "domainAccessor" => "m_inspectorController->fileSystemAgent()",

How is this "domainAccessor" relevant to the FileSystem? The file system is per security origin.

> WebCore/inspector/InspectorFileSystemAgent.cpp:125
> +void InspectorFileSystemAgent::getFileSystemPathAsync(unsigned int type, const String& domain)

Since file systems are per security origin, should the second param be a const SecurityOrigin& ?

> WebCore/inspector/InspectorFileSystemAgent.cpp:143
> +        LocalFileSystem::localFileSystem().requestFileSystem(document, asyncFileSystemType, 0, new InspectorFileSystemAgentCallbacks(this, asyncFileSystemType));

Would it make sense for the InspectorFileSystemAgentCallbacks instance to keep track of the SecurityOrigin being queried?

> WebCore/inspector/InspectorFileSystemAgent.cpp:147
> +void InspectorFileSystemAgent::didGetFileSystemPath(const String& root, AsyncFileSystem::Type type)

Should this method also indicate which SecurityOrigin this 'root' and would 'rootPath' be a better name?

> WebCore/inspector/InspectorFileSystemAgent.cpp:154
> +        m_persistentRoot = root;

Since the 'rootPath' is delivered to the frontend below, does this class need to have these two data members? Could the frontend provide the path when calling revealFolderInOS(path)?

> WebCore/inspector/InspectorFileSystemAgent.cpp:158
> +    m_frontend->didGetFileSystemPath(root, static_cast<unsigned int>(type));

Similarly, indicate which SecurityOrigin.

> WebCore/inspector/InspectorFileSystemAgent.cpp:166
> +    m_frontend->didGetFileSystemError(static_cast<unsigned int>(type));

Ditto, which SecurityOrigin.

> WebCore/inspector/Inspector.idl:229
> +        [notify] void didGetFileSystemError(out int type);

To summarize these interface changes...

         [handler=FileSystem] void getFileSystemPathAsync(in unsigned int type, in String securityOrigin);
         [handler=FileSystem] void revealFolderInOS(in String path);
         [notify] void didGetFileSystemPath(out String rootPath, out int type, String securityOrigin);
         [notify] void didGetFileSystemError(out int type, String securityOrigin);

I'm not familiar with these IDL annotations as applied to the inspector. Is 'out' correct on those method arguments?

> WebCore/inspector/front-end/FileSystemView.js:35
> +    InspectorBackend.getFileSystemPathAsync(type, domain);

domain s/b securityOrigin

> WebCore/inspector/front-end/FileSystemView.js:38
> +WebInspector.FileSystemView = function(treeElement, fileSystemDomain)

domain s/b securityOrigin

> WebCore/inspector/front-end/StoragePanel.js:214
> +        var fileSystemTreeElement = new WebInspector.FileSystemSidebarTreeElement(domain);

domain s/b securityOrigin

> WebCore/inspector/front-end/StoragePanel.js:312
> +    showFileSystem: function(treeElement, fileSystemDomain)

domain s/b securityOrigin... or securityOrigin a data member is the treeElement?

> WebCore/inspector/front-end/StoragePanel.js:710
> +    this._fileSystemDomain = fileSystemDomain;

domain s/b securityOrigin... and ah... it is a data member of the treeElement.

> WebCore/inspector/front-end/inspector.js:1356
> +WebInspector._addFileSystemDomain = function(domain)

domain s/b securityOrigin

[Michael Nordman]

A good way to represent SecurityOrigin in the inspector front/back interfaces could be as a String, see SecurityOrigin::toString() and SecurityOrigin::fromString().

[Pavel Feldman]

Comment on attachment 71519 [details]
patch4

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

>> WebCore/inspector/InspectorFileSystemAgent.cpp:125
>> +void InspectorFileSystemAgent::getFileSystemPathAsync(unsigned int type, const String& domain)
> 
> Since file systems are per security origin, should the second param be a const SecurityOrigin& ?

@Michael: You can't pass SecurityOrigin to/from front-end, so passing domain is our best guess here.

> WebCore/inspector/InspectorFileSystemAgent.cpp:138
> +        // FIXME: Change this to iterate over all frames to match domain when UI supports

I really don't see point in not supporting non-main frame domain since it is fairly trivial. All you need is to traverse the frame tree (using mainFrame()->tree()->traverseNext) and pick appropriate instance.

>> WebCore/inspector/Inspector.idl:229
>> +        [notify] void didGetFileSystemError(out int type);
> 
> To summarize these interface changes...
> 
>          [handler=FileSystem] void getFileSystemPathAsync(in unsigned int type, in String securityOrigin);
>          [handler=FileSystem] void revealFolderInOS(in String path);
>          [notify] void didGetFileSystemPath(out String rootPath, out int type, String securityOrigin);
>          [notify] void didGetFileSystemError(out int type, String securityOrigin);
> 
> I'm not familiar with these IDL annotations as applied to the inspector. Is 'out' correct on those method arguments?

@Michael: The problem with querying by security origin is that you need to push it to the front-end first.

[Michael Nordman]

> > Since file systems are per security origin, should the second param be a const SecurityOrigin& ?
> 
> @Michael: You can't pass SecurityOrigin to/from front-end, so passing domain is our best guess here.

Using the 'hostname' as the security origin is simply incorrect. It just doesn't work.

> @Michael: The problem with querying by security origin is that you need to push it to the front-end first.

Why is that a problem? When the main resource of a frame is loaded, pass along a string representation of the SecurityOrigin. This string value can be used as the label for the 'tree' element shown in the left-hand-side of the inspector.

[Kavita Kanetkar]

Created attachment 71832 [details]
patch5

1) Now passing security origin back and forth between FE and BE through IDL.

2) Agent now traversing frame tree to match first frame for which document's security origin is the one selected from FE.
Returning FS for only that document.

Thanks!

[Pavel Feldman]

Comment on attachment 71832 [details]
patch5

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

Overall looks good. Better guard from dupe entries and localizesStrings modifications and we are good to land. Btw, I'd really like to see some tests!

> WebCore/inspector/InspectorFileSystemAgent.cpp:129
> +    if (!tree)

There always is a tree.

> WebCore/inspector/InspectorFileSystemAgent.cpp:131
> +    while (Frame* frame = tree->traverseNext()) {

We usually We usually use for loop while iterating frame tree.

> WebCore/inspector/front-end/FileSystemView.js:92
> +        WebInspector.FileSystem.getFileSystemPathAsync(this.fileSystemType.PERSISTENT, this._origin);

Given that you make these calls together, it might make sense to merge them.

> WebCore/inspector/front-end/FileSystemView.js:145
> +        var rootPath = WebInspector.UIString("File System root path not available.");

I don't see a localizedStrings file in this patch.

> WebCore/inspector/front-end/FileSystemView.js:147
> +            rootPath = WebInspector.UIString("Error in fetching root path for file system.");

ditto

> WebCore/inspector/front-end/FileSystemView.js:159
> +            this.contentElement = document.createElement("div");

Theses all should be variables, not fields of this. (i.e. var contentElement, var choicesForm, etc.)

> WebCore/inspector/front-end/inspector.css:1914
> +    content: url(Images/applicationCache.png);

Do you have an icon?

> WebCore/inspector/front-end/StoragePanel.js:66
> +        this.fileSystemListTreeElement = new WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("File System"), "file-system-storage-tree-item");

ditto

> WebCore/inspector/front-end/StoragePanel.js:112
> +        this._fileSystemView = null;

I don't think you need to cache the view. (Others here should not do it as well)

> WebCore/inspector/front-end/StoragePanel.js:371
> +        var view = this._fileSystemView;

It is safe to create one each time...

> WebCore/inspector/front-end/inspector.js:1271
> +                this._addFileSystemOrigin(parsedURL.scheme + "://" + parsedURL.host + (parsedURL.port ? (":" + parsedURL.port) : ""));

This is fragile. You should either push security origin strings into front-end explicitly, or at least put a comment here that this string should match SecurityOrigin::toString. You also might want to add a test for this.

> WebCore/inspector/front-end/inspector.js:1390
> +    this.fileSystemOrigins[origin] = true;

I don't see how setting property to true eliminates the duplicate origin.

> WebCore/inspector/front-end/inspector.js:1394
> +    this.panels.storage.addFileSystem(origin);

I'd rather have storage panel handling the dupe origins with no global fileSystemOrigins map.

[Michael Nordman]

Comment on attachment 71832 [details]
patch5

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

> WebCore/inspector/CodeGeneratorInspector.pm:49
> +    "domainAccessor" => "m_inspectorController->fileSystemAgent()",

I'm just curious about what the "domainAccessor" annotation here means?

> WebCore/inspector/InspectorFileSystemAgent.cpp:134
> +            LocalFileSystem::localFileSystem().requestFileSystem(document, asyncFileSystemType, 0, new InspectorFileSystemAgentCallbacks(this, asyncFileSystemType, origin));

We're bailing out of the 'tree' iteration early, does 'tree' need to be reset for the next consumer? How does that work?

Oh... i see FrameTree is actually a node in the tree, so this iteration isn't quite right. You have to access the tree(node) associated with the current 'frame' and traverse to the next thru it. If there's a convention for performing this iteration, its probably a good idea to follow that convention here.

[Kavita Kanetkar]

Created attachment 72143 [details]
patch6

I am using appcache icon. I do have a FIXME to create a new one for file system.
Added binary diff for localizedStrings.js and addressed the comments.

Thanks.

[WebKit Review Bot]

Attachment 72143 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4849070

[Kavita Kanetkar]

Created attachment 72216 [details]
patch

AsyncFileSystemCallbacks.h change landed before this which broke the compile.

[Pavel Feldman]

Comment on attachment 72216 [details]
patch

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

> WebCore/inspector/InspectorFileSystemAgent.cpp:128
> +    for (Frame* frame = mainFrame; frame; frame = frame->tree()->traverseNext()) {

nit: you should pass mainFrame into the traverseNext

> WebCore/inspector/front-end/inspector.js:55
> +    fileSystemOrigins: {},

I really don't like polluting our global namespace with public maps like this. I am currently getting rid of the nastiest one, the WebInspector.resources. It is painful to see the new ones coming. They belong to the corresponding panels. I'd like to see this fixed in the follow up change.

> WebCore/inspector/front-end/inspector.js:1296
> +                // This should match the SecurityOrigin::toString(). FIXME: Add a test for this.

I'd like to see the test in the follow up change please.

[Michael Nordman]

Comment on attachment 72216 [details]
patch

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

> WebCore/inspector/front-end/StoragePanel.js:369
> +    showFileSystem: function(treeElement, fileSystemDomain)

should 'fileSystemDomain' be 'origin' here?

> WebCore/inspector/front-end/StoragePanel.js:371
> +        this._fileSystemView =  new WebInspector.FileSystemView(treeElement, fileSystemDomain);

ditto

> WebCore/inspector/front-end/StoragePanel.js:956
> +WebInspector.FileSystemTreeElement = function(storagePanel, fileSystemDomain)

ditto

> WebCore/inspector/front-end/StoragePanel.js:965
> +        this._storagePanel.showFileSystem(this, this._fileSystemDomain);

ditto

[WebKit Commit Bot]

Comment on attachment 72216 [details]
patch

Rejecting patch 72216 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 72216]" exit_code: 2
Last 500 characters of output:
em.h
patching file WebCore/platform/chromium/ChromiumBridge.h
patching file WebCore/platform/chromium/FileSystemChromium.cpp
patching file WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/chromium/public/WebFileUtilities.h
patching file WebKit/chromium/src/ChromiumBridge.cpp
patching file WebKit/chromium/src/js/DevTools.js

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4764070

[Kavita Kanetkar]

Created attachment 72237 [details]
patch

Another 1 line conflict landed before this.

[Dumitru Daniliuc]

Comment on attachment 72237 [details]
patch

rs=me, based on pfeldman's r+.

[WebKit Commit Bot]

Comment on attachment 72237 [details]
patch

Rejecting patch 72237 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 72237]" exit_code: 2
Last 500 characters of output:
h
patching file WebCore/platform/chromium/ChromiumBridge.h
patching file WebCore/platform/chromium/FileSystemChromium.cpp
patching file WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/chromium/public/WebFileUtilities.h
patching file WebKit/chromium/src/ChromiumBridge.cpp
patching file WebKit/chromium/src/js/DevTools.js

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dumitru Daniliuc', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4840063

[Kavita Kanetkar]

Created attachment 72397 [details]
patch

svn-apply works on local machine with updated webkit. If commit-bot rejects it again, I will need some help in figuring out why.

[WebKit Commit Bot]

Comment on attachment 72397 [details]
patch

Clearing flags on attachment: 72397

Committed r70956: <http://trac.webkit.org/changeset/70956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.