Bug 48169 – [FileSystem] Support not creating directories when queried by inspector.

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

[Kavita Kanetkar]

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.

[Kavita Kanetkar]

Created attachment 71618 [details]
patch

[Kinuko Yasuda]

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.

[Kavita Kanetkar]

Created attachment 72617 [details]
patch

Please take a look.

[Kinuko Yasuda]

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.

[Kavita Kanetkar]

Created attachment 72624 [details]
patch

Addressed comments. Thanks.

[Kinuko Yasuda]

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..

[Dumitru Daniliuc]

(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.

[Dumitru Daniliuc]

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.

[Kinuko Yasuda]

(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.

[Kavita Kanetkar]

Created attachment 72746 [details]
patch

[WebKit Commit Bot]

Comment on attachment 72746 [details]
patch

Clearing flags on attachment: 72746

Committed r71206: <http://trac.webkit.org/changeset/71206>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.