Currently LocalFileSystem's requestFileSystem() will always create root directories for temp and persistent filesystem types. Add ability to configure that. We do not want inspector to be creating directories.
Created attachment 71618 [details] patch
Comment on attachment 71618 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=71618&action=review > WebKit/chromium/src/WebWorkerBase.cpp:240 > +void WebWorkerBase::openFileSystem(WebFileSystem::Type type, long long size, WebFileSystemCallbacks* callbacks, bool create, bool synchronous) Per discussion, I don't think we need to propagate this flag via all the worker plumbing. > WebKit/chromium/src/WebWorkerBase.h:96 > + void openFileSystem(WebFileSystem::Type, long long size, WebFileSystemCallbacks*, bool create, bool synchronous); ditto. > WebKit/chromium/src/LocalFileSystemChromium.cpp:60 > +void LocalFileSystem::requestFileSystem(ScriptExecutionContext* context, AsyncFileSystem::Type type, long long size, PassOwnPtr<AsyncFileSystemCallbacks> callbacks, bool create, bool synchronous) If the inspector version will not need the worker implementation, how about adding a new method like queryFileSystem that always calls openFileSystem with create=false and keeping this requestFileSystem as is. > WebKit/chromium/src/LocalFileSystemChromium.cpp:66 > + webFrame->client()->openFileSystem(webFrame, static_cast<WebFileSystem::Type>(type), size, create, new WebFileSystemCallbacksImpl(callbacks)); We still don't have the implementation in chromium -- let's put a default implementation in WebKit/public/WebFileSystem.h with FIXME comment: virtual void openFileSystem(WebKit::WebFrame* frame, WebKit::WebFileSystem::Type type, long long size, bool create, WebKit::WebFileSystemCallbacks* callbacks) { return openFileSystem(frame, type, size, true, callbacks); } After we put chromium sides implementation we can remove the default implementation. ...and if we don't change the worker side code I think we can drop the change in WebCommonWorkerClient::openFileSystem.
Created attachment 72617 [details] patch Please take a look.
Comment on attachment 72617 [details] patch Looks good to me. View in context: https://bugs.webkit.org/attachment.cgi?id=72617&action=review > WebKit/chromium/src/LocalFileSystemChromium.cpp:63 > + if (context->isDocument()) { Might be better to drop this if and have ASSERT(context->isDocument()) instead? > WebCore/fileapi/LocalFileSystem.h:56 > + void readFileSystem(ScriptExecutionContext*, AsyncFileSystem::Type, long long size, PassOwnPtr<AsyncFileSystemCallbacks>, bool synchronous = false); We won't need synchronous flag for this method. > WebCore/platform/AsyncFileSystem.cpp:56 > +void AsyncFileSystem::openFileSystem(const String& basePath, const String& storageIdentifier, Type type, bool create, PassOwnPtr<AsyncFileSystemCallbacks> callbacks) You'll need to omit unused parameter name (s/bool create/bool/) to avoid compile warnings. > WebCore/platform/AsyncFileSystem.h:70 > + static void openFileSystem(const String& basePath, const String& storageIdentifier, Type, bool, PassOwnPtr<AsyncFileSystemCallbacks>); Please include parameter name (s/bool/bool create/) since otherwise it doesn't give idea what to specify. Also it might be good to have some short description about this new flag.
Created attachment 72624 [details] patch Addressed comments. Thanks.
Comment on attachment 72624 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=72624&action=review > WebKit/chromium/src/LocalFileSystemChromium.cpp:62 > + ASSERT(context->isDocument()); ASSERT(context && context->isDocument())? > WebCore/fileapi/LocalFileSystem.h:56 > + void readFileSystem(ScriptExecutionContext*, AsyncFileSystem::Type, long long size, PassOwnPtr<AsyncFileSystemCallbacks>); readFileSystem may sound like reading some contents of the filesystem... how about queryFileSystem? (readFileSystem sounds ok to me too, just giving an idea.) > WebCore/platform/AsyncFileSystem.h:69 > + // Opens a new file system. |create| specifies whether or not to create the path if it does not already exists. nit: we do not use || in WebKit as far as I know..
(In reply to comment #6) > (From update of attachment 72624 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72624&action=review > > > WebKit/chromium/src/LocalFileSystemChromium.cpp:62 > > + ASSERT(context->isDocument()); > > ASSERT(context && context->isDocument())? i've been told that ASSERT(context->isDocument()) is enough, because if 'context' is NULL, then we'll crash on this line anyway.
Comment on attachment 72624 [details] patch r=me. please address kinuko's comments though before landing this patch, or upload a new one if you want the commit-queue to land it.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 72624 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=72624&action=review > > > > > WebKit/chromium/src/LocalFileSystemChromium.cpp:62 > > > + ASSERT(context->isDocument()); > > > > ASSERT(context && context->isDocument())? > > i've been told that ASSERT(context->isDocument()) is enough, because if 'context' is NULL, then we'll crash on this line anyway. Both will crash but they are not same - crashing by an assertion failure and crashing by null-pointer access could have different meanings/crash reports.
Created attachment 72746 [details] patch
Comment on attachment 72746 [details] patch Clearing flags on attachment: 72746 Committed r71206: <http://trac.webkit.org/changeset/71206>
All reviewed patches have been landed. Closing bug.