Bug 91861 - FileSystem should provide a way to delete filesystem.
Summary: FileSystem should provide a way to delete filesystem.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on:
Blocks: 91831
  Show dependency treegraph
 
Reported: 2012-07-20 07:40 PDT by Taiju Tsuiki
Modified: 2012-07-25 13:56 PDT (History)
11 users (show)

See Also:


Attachments
Patch (10.99 KB, patch)
2012-07-20 07:47 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2012-07-22 20:37 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2012-07-22 20:40 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (12.48 KB, patch)
2012-07-24 14:37 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (1.02 MB, application/zip)
2012-07-24 15:20 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Taiju Tsuiki 2012-07-20 07:40:54 PDT
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.
Comment 1 Taiju Tsuiki 2012-07-20 07:47:17 PDT
Created attachment 153503 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-20 07:49:06 PDT
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 3 Adam Barth 2012-07-20 09:05:40 PDT
Comment on attachment 153503 [details]
Patch

WebKit API change LGTM
Comment 4 Kinuko Yasuda 2012-07-22 06:33:30 PDT
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()) ?
Comment 5 Taiju Tsuiki 2012-07-22 20:37:55 PDT
Created attachment 153726 [details]
Patch
Comment 6 Taiju Tsuiki 2012-07-22 20:40:34 PDT
Created attachment 153727 [details]
Patch
Comment 7 Taiju Tsuiki 2012-07-22 20:41:34 PDT
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
Comment 8 Kinuko Yasuda 2012-07-24 13:51:01 PDT
Comment on attachment 153727 [details]
Patch

Thanks, FileSystem related change look good to me.
Comment 9 Eric U. 2012-07-24 14:28:40 PDT
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
Comment 10 Taiju Tsuiki 2012-07-24 14:37:28 PDT
Created attachment 154143 [details]
Patch
Comment 11 Taiju Tsuiki 2012-07-24 14:38:50 PDT
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
Comment 12 WebKit Review Bot 2012-07-24 15:20:39 PDT
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
Comment 13 WebKit Review Bot 2012-07-24 15:20:44 PDT
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
Comment 14 Taiju Tsuiki 2012-07-24 15:50:06 PDT
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.
Comment 15 Taiju Tsuiki 2012-07-25 10:05:46 PDT
Adam, could you take a look for it? I believe this patch is ready.
Comment 16 Adam Barth 2012-07-25 10:11:24 PDT
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?
Comment 17 Taiju Tsuiki 2012-07-25 11:06:45 PDT
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.
Comment 18 Adam Barth 2012-07-25 11:25:33 PDT
Comment on attachment 154143 [details]
Patch

Ok.
Comment 19 WebKit Review Bot 2012-07-25 13:56:37 PDT
Comment on attachment 154143 [details]
Patch

Clearing flags on attachment: 154143

Committed r123653: <http://trac.webkit.org/changeset/123653>
Comment 20 WebKit Review Bot 2012-07-25 13:56:43 PDT
All reviewed patches have been landed.  Closing bug.