Summary: | Web Inspector: Support inspector file system access with isolated file system through InspectorFrontendHost. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vsevolod Vlasov <vsevik> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Vsevolod Vlasov <vsevik> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, apavlov, dglazkov, fishd, jamesr, keishi, kinuko, loislo, pfeldman, pmuellr, tkent+wkapi, vsevik, web-inspector-bugs, webkit-ews, webkit.review.bot, yurys | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Vsevolod Vlasov
2012-12-24 09:36:22 PST
Created attachment 180676 [details]
Patch
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 on attachment 180676 [details] Patch Attachment 180676 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15527053 Comment on attachment 180676 [details] Patch Attachment 180676 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15495528 Comment on attachment 180676 [details] Patch Attachment 180676 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15527055 Comment on attachment 180676 [details] Patch Attachment 180676 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15507496 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? Created attachment 180882 [details]
Patch
Created attachment 180884 [details]
Patch
Created attachment 180918 [details]
Patch
Created attachment 180919 [details]
Patch
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 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 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. (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. Committed r139726: <http://trac.webkit.org/changeset/139726> |