Bug 105727 – Web Inspector: Support inspector file system access with isolated file system through InspectorFrontendHost.

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

[Vsevolod Vlasov]

Inspector needs file system access to allow developers edit/add sources (even those that are not loaded from server, e.g. deployment descriptors, server scripts).

We plan to use File System API (isolated file system).
FileSystem access request is implemented through InspectorFrontendHost methods and InspectorFrontendAPI callbacks.

InspectorFrontendHost.canAccessFileSystem() - Returns true if file system access is implemented for this embedder.

InspectorFrontendHost.requestFileSystemPermissions() - This method should be called once on inspector start.
InspectorFrontendAPI.fileSystemPermissionsLoaded(fileSystems) - This method is called once isolated file systems are registered, and provides inspector with all necessary information.

void selectFolderAndGrantFileSystemPermission() - Requests embedder to show user select file dialog and register selected dialog as a file system.
InspectorFrontendAPI.fileSystemFolderSelected(errorMessage, fileSystem) - This method is called after user action. 

void revokeFileSystemPermission(in DOMString fileSystemPath) - Requests embedder to revoke registered isolated file system.
InspectorFrontendAPI.fileSystemFolderPermissionsRevoked(fileSystemPath) - revoke callback.

DOMFileSystem isolatedFileSystem(in DOMString fileSystemId, in DOMString registeredName) - Creates javascript DOMFileSystem object.

[Vsevolod Vlasov]

Created attachment 180676 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15527053

[EFL EWS Bot]

Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15495528

[Early Warning System Bot]

Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15527055

[Early Warning System Bot]

Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15507496

[Pavel Feldman]

Comment on attachment 180676 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendClient.h:75
> +    virtual bool canAccessFileSystem() = 0;

supportsFilesystems()

> Source/WebCore/inspector/InspectorFrontendClient.h:76
> +    virtual void requestFileSystemPermissions() = 0;

requestFilesystems()

> Source/WebCore/inspector/InspectorFrontendClient.h:77
> +    virtual void selectFolderAndGrantFileSystemPermission() = 0;

addFilesystem()

> Source/WebCore/inspector/InspectorFrontendClient.h:78
> +    virtual void revokeFileSystemPermission(const String& fileSystemPath) = 0;

removeFilesystem()

> Source/WebCore/inspector/front-end/FileSystemMapping.js:40
> +    fileSystemPathes: function() { },

Paths

> Source/WebCore/inspector/front-end/FileSystemMapping.js:46
> +    fileSystemPathForURI: function(uri) { },

fileSystemForURI

> Source/WebCore/inspector/front-end/FileSystemMapping.js:52
> +    filePathForURI: function(uri) { },

pathForURI

> Source/WebCore/inspector/front-end/FileSystemMapping.js:59
> +    uriForPath: function(fileSystemPath, filePath) { },

Should you introduce a FileDescriptor class?

> Source/WebCore/inspector/front-end/FileSystemMapping.js:65
> +    uriPrefixForPath: function(path) { }

Why do you need this?

[Vsevolod Vlasov]

Created attachment 180882 [details]
Patch

[Vsevolod Vlasov]

Created attachment 180884 [details]
Patch

[Vsevolod Vlasov]

Created attachment 180918 [details]
Patch

[Vsevolod Vlasov]

Created attachment 180919 [details]
Patch

[Pavel Feldman]

Comment on attachment 180919 [details]
Patch

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

> Source/WebCore/inspector/front-end/IsolatedFilesystemModel.js:205
> +    this._isolatedFilesystemModel = isolatedFilesystemModel;

You either need a new model or a new manager :)

[Kinuko Yasuda]

Comment on attachment 180919 [details]
Patch

A few comments for the changes in filesystem Module:

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

> Source/WebCore/Modules/filesystem/DOMFileSystem.cpp:65
> +PassRefPtr<DOMFileSystem> DOMFileSystem::createIsolatedFileSystem(ScriptExecutionContext* context, const String& filesystemId, const String& registeredName)

nit: since the 'registeredName' part is used as (a part of) the root directory path, naming this 'additionalRootPath' or something like that might make it clearer?

> Source/WebCore/Modules/filesystem/DOMFileSystem.cpp:86
> +        rootURL.append(registeredName);

Can we check if the given registeredName doesn't contain any '..'?  (Also if there's any assumptions we should probably check them here too)

> Source/WebKit/chromium/src/InspectorFrontendClientImpl.cpp:177
> +

nit: extra empty line

[Kinuko Yasuda]

Comment on attachment 180919 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendHost.cpp:327
> +    return DOMFileSystem::createIsolatedFileSystem(context, filesystemId, registeredName);

By the way instead of modifying DOMFileSystem.{h,cc} you can also create fileSystemName and rootPath in the embedder side (i.e. chromium) and directly create a DOMFileSystem like this:

DOMFileSystem::create(context, FileSystemTypeIsolated, fileSystemName, rootPath);

while it may look a bit tricky it seems more common pattern when we create custom isolated filesystems.

[Vsevolod Vlasov]

(In reply to comment #14)
> (From update of attachment 180919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180919&action=review
> 
> > Source/WebCore/inspector/InspectorFrontendHost.cpp:327
> > +    return DOMFileSystem::createIsolatedFileSystem(context, filesystemId, registeredName);
> 
> By the way instead of modifying DOMFileSystem.{h,cc} you can also create fileSystemName and rootPath in the embedder side (i.e. chromium) and directly create a DOMFileSystem like this:
> 
> DOMFileSystem::create(context, FileSystemTypeIsolated, fileSystemName, rootPath);
> 
> while it may look a bit tricky it seems more common pattern when we create custom isolated filesystems.

Sure, this is exactly what I need, thank you.

[Vsevolod Vlasov]

Committed r139726: <http://trac.webkit.org/changeset/139726>