Summary: | [FileSystem] Support not creating directories when queried by inspector. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kavita Kanetkar <kkanetkar> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, dumi, kinuko | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Kavita Kanetkar
2010-10-22 18:22:37 PDT
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. |