Bug 91861 – FileSystem should provide a way to delete filesystem.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Taiju Tsuiki]

Can I add LocalFileSystem::deleteFileSystem like following patch?
I'm implementing FileSystem support of Inspector, and I'd like to allow web developers to delete FileSystem for debugging their applications.

[Taiju Tsuiki]

Created attachment 153503 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 153503 [details]
Patch

WebKit API change LGTM

[Kinuko Yasuda]

Comment on attachment 153503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153503&action=review

> Source/WebCore/ChangeLog:7
> +

We need a comment about how we are testing it. (E.g. "We will add a test when we wire-up the feature to the inspector" etc)

> Source/WebCore/platform/AsyncFileSystem.h:70
> +    static void deleteFileSystem(const String& basePath, const String& storageIdentifier, PassOwnPtr<AsyncFileSystemCallbacks>);

This likely needs FileSystemType parameter?

Also while you're there can you add FIXME comment at openFileSystem for adding type parameter?  (Looks like we should have it there too)

> Source/WebKit/chromium/public/WebFrameClient.h:353
> +    // All on-flight operation and following operation may fail after the

nit: on-flight -> in-flight, operation -> operations ?

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237
> +    if (!context->isDocument()) {

Should we rather ASSERT(context->isDocument()) ?

[Taiju Tsuiki]

Created attachment 153726 [details]
Patch

[Taiju Tsuiki]

Created attachment 153727 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 153503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153503&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> We need a comment about how we are testing it. (E.g. "We will add a test when we wire-up the feature to the inspector" etc)

Done

>> Source/WebCore/platform/AsyncFileSystem.h:70
>> +    static void deleteFileSystem(const String& basePath, const String& storageIdentifier, PassOwnPtr<AsyncFileSystemCallbacks>);
> 
> This likely needs FileSystemType parameter?
> 
> Also while you're there can you add FIXME comment at openFileSystem for adding type parameter?  (Looks like we should have it there too)

Done

>> Source/WebKit/chromium/public/WebFrameClient.h:353
>> +    // All on-flight operation and following operation may fail after the
> 
> nit: on-flight -> in-flight, operation -> operations ?

Done

>> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237
>> +    if (!context->isDocument()) {
> 
> Should we rather ASSERT(context->isDocument()) ?

Done

[Kinuko Yasuda]

Comment on attachment 153727 [details]
Patch

Thanks, FileSystem related change look good to me.

[Eric U.]

Comment on attachment 153727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153727&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:350
> +    // WebFileSystemCallbacks::didSuccess() must be called when the operation

s/didSuccess/didSucceed

[Taiju Tsuiki]

Created attachment 154143 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 153727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153727&action=review

>> Source/WebKit/chromium/public/WebFrameClient.h:350
>> +    // WebFileSystemCallbacks::didSuccess() must be called when the operation
> 
> s/didSuccess/didSucceed

Done

[WebKit Review Bot]

Comment on attachment 154143 [details]
Patch

Attachment 154143 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13330527

New failing tests:
fast/loader/loadInProgress.html
fast/inline/positionedLifetime.html
fast/loader/unload-form-post-about-blank.html

[WebKit Review Bot]

Created attachment 154151 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Taiju Tsuiki]

Comment on attachment 154143 [details]
Patch

Changing commit-queu flag to "?" from "-".
I believe this patch doesn't make any functional change until wired-up.

[Taiju Tsuiki]

Adam, could you take a look for it? I believe this patch is ready.

[Adam Barth]

Comment on attachment 154143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154143&action=review

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237
> +    ASSERT(context->isDocument());

This doesn't exist in workers?

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:240
> +    WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());

What if document->frame is null?

[Taiju Tsuiki]

Comment on attachment 154143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154143&action=review

>> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237
>> +    ASSERT(context->isDocument());
> 
> This doesn't exist in workers?

For expected usage from Inspector, we don't need to call this from worker.

>> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:240
>> +    WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
> 
> What if document->frame is null?

document->frame is always non-null for our usage in Inspector.

[Adam Barth]

Comment on attachment 154143 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 154143 [details]
Patch

Clearing flags on attachment: 154143

Committed r123653: <http://trac.webkit.org/changeset/123653>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.