Bug 105727 - Web Inspector: Support inspector file system access with isolated file system through InspectorFrontendHost.
Summary: Web Inspector: Support inspector file system access with isolated file system...
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: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-24 09:36 PST by Vsevolod Vlasov
Modified: 2013-01-15 01:07 PST (History)
16 users (show)

See Also:


Attachments
Patch (35.83 KB, patch)
2012-12-24 09:43 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (36.91 KB, patch)
2012-12-28 09:05 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (37.06 KB, patch)
2012-12-28 09:13 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (37.00 KB, patch)
2012-12-28 21:47 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (37.00 KB, patch)
2012-12-28 22:10 PST, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2012-12-24 09:36:22 PST
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.
Comment 1 Vsevolod Vlasov 2012-12-24 09:43:35 PST
Created attachment 180676 [details]
Patch
Comment 2 WebKit Review Bot 2012-12-24 09:48:43 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Build Bot 2012-12-24 09:50:34 PST
Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15527053
Comment 4 EFL EWS Bot 2012-12-24 09:57:15 PST
Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15495528
Comment 5 Early Warning System Bot 2012-12-24 09:57:17 PST
Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15527055
Comment 6 Early Warning System Bot 2012-12-24 09:57:35 PST
Comment on attachment 180676 [details]
Patch

Attachment 180676 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15507496
Comment 7 Pavel Feldman 2012-12-24 10:03:37 PST
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?
Comment 8 Vsevolod Vlasov 2012-12-28 09:05:10 PST
Created attachment 180882 [details]
Patch
Comment 9 Vsevolod Vlasov 2012-12-28 09:13:11 PST
Created attachment 180884 [details]
Patch
Comment 10 Vsevolod Vlasov 2012-12-28 21:47:12 PST
Created attachment 180918 [details]
Patch
Comment 11 Vsevolod Vlasov 2012-12-28 22:10:18 PST
Created attachment 180919 [details]
Patch
Comment 12 Pavel Feldman 2012-12-29 05:05:10 PST
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 :)
Comment 13 Kinuko Yasuda 2013-01-09 06:13:27 PST
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
Comment 14 Kinuko Yasuda 2013-01-09 07:19:02 PST
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.
Comment 15 Vsevolod Vlasov 2013-01-14 05:56:54 PST
(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.
Comment 16 Vsevolod Vlasov 2013-01-15 01:07:35 PST
Committed r139726: <http://trac.webkit.org/changeset/139726>