Wire up the deleteFileSystem operation.

Wire up the deleteFileSystem operation.

BUG=139366
TEST="Will be tested from WebKit inspector test"

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149707

[nhiroki (google), 2012-07-27 00:00:45]

Hi, tzik

Could you review this?

Thanks,
nhiroki

[tzik, 2012-07-27 17:53:47]

Looks good, overall.
I think we need to modify CrosMountPointProvider too.

https://chromiumcodereview.appspot.com/10828043/diff/1/content/browser/fileap...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10828043/diff/1/content/browser/fileap...
content/browser/fileapi/fileapi_message_filter.cc:665: if (result ==
base::PLATFORM_FILE_OK) {
nit: our style allows removing '{' and '}' here.

https://chromiumcodereview.appspot.com/10828043/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10828043/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:3681:
callbacks->didFail(WebKit::WebFileErrorAbort);
I think we can call didSucceed here since we have no data to remove already.

[nhiroki (google), 2012-07-27 18:36:19]

Thank you for your review.

http://codereview.chromium.org/10828043/diff/1/content/browser/fileapi/fileap...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10828043/diff/1/content/browser/fileapi/fileap...
content/browser/fileapi/fileapi_message_filter.cc:665: if (result ==
base::PLATFORM_FILE_OK) {
On 2012/07/27 17:53:47, tzik wrote:
> nit: our style allows removing '{' and '}' here.

Done.

http://codereview.chromium.org/10828043/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10828043/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:3681:
callbacks->didFail(WebKit::WebFileErrorAbort);
On 2012/07/27 17:53:47, tzik wrote:
> I think we can call didSucceed here since we have no data to remove already.

Done.

[tzik, 2012-07-28 00:36:45]

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:146: NOTREACHED();
Could you invoke callback even after we reached NOTREACHED()?
NOTREACHED() will do nothing in Release build.

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.h:42:
DeleteFileSystemCallback;
nit: maybe, we can use "using
fileapi::FileSystemMountPointProvider::DeleteFileSystemCallback" instead.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:113: DeleteFileSystemCallback;
ditto. Use "using"

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.cc:127: NOTREACHED();
ditto. Invoke callback please.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.h:24: DeleteFileSystemCallback;
ditto per "using".

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:484: const
base::Callback<void(PlatformFileError)>& callback) const {
Can we use DeleteFileSystemCallback here?

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:47: DeleteFileSystemCallback;
ditto.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
File webkit/fileapi/test_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
webkit/fileapi/test_mount_point_provider.cc:170: NOTREACHED();
ditto

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
File webkit/fileapi/test_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
webkit/fileapi/test_mount_point_provider.h:32: DeleteFileSystemCallback;
ditto.

[nhiroki (google), 2012-07-28 02:01:55]

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:146: NOTREACHED();
On 2012/07/28 00:36:46, tzik wrote:
> Could you invoke callback even after we reached NOTREACHED()?
> NOTREACHED() will do nothing in Release build.

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.h:42:
DeleteFileSystemCallback;
On 2012/07/28 00:36:46, tzik wrote:
> nit: maybe, we can use "using
> fileapi::FileSystemMountPointProvider::DeleteFileSystemCallback" instead.

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:113: DeleteFileSystemCallback;
I think we cannot use "using" in this case.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.cc:127: NOTREACHED();
On 2012/07/28 00:36:46, tzik wrote:
> ditto. Invoke callback please.

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.h:24: DeleteFileSystemCallback;
On 2012/07/28 00:36:46, tzik wrote:
> ditto per "using".

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:484: const
base::Callback<void(PlatformFileError)>& callback) const {
On 2012/07/28 00:36:46, tzik wrote:
> Can we use DeleteFileSystemCallback here?

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:47: DeleteFileSystemCallback;
On 2012/07/28 00:36:46, tzik wrote:
> ditto.

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
File webkit/fileapi/test_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
webkit/fileapi/test_mount_point_provider.cc:170: NOTREACHED();
On 2012/07/28 00:36:46, tzik wrote:
> ditto

Done.

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
File webkit/fileapi/test_mount_point_provider.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/test_mount_p...
webkit/fileapi/test_mount_point_provider.h:32: DeleteFileSystemCallback;
On 2012/07/28 00:36:46, tzik wrote:
> ditto.

Done.

[tzik, 2012-07-28 04:26:51]

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10828043/diff/6002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:113: DeleteFileSystemCallback;
On 2012/07/28 02:01:56, nhiroki wrote:
> I think we cannot use "using" in this case.

Right, it's my mistake. Sorry.

[nhiroki (google), 2012-07-28 04:42:45]

Sorry I submitted wrong patchset.
Could you take another look?

Thanks.

[tzik, 2012-07-29 13:07:21]

LGTM

Please ask OWNERS to review this CL before you commit it.

[nhiroki (google), 2012-07-30 12:10:14]

+kinuko@, +jamesr@

Hi, kinuko@ and jamesr@

Could you take a look for owner LGTM?

- kinuko@
    content/common/fileapi
    webkit/fileapi
    content/browser/fileapi
    webkit/chromeos
- jamesr@
    content/renderer

Thanks,
nhiroki

[kinuko, 2012-07-30 21:53:38]

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.cc:171: void
FileSystemContext::DeleteFileSystem(
This could be done in a separate CL but I think we could also cleanup similar
deleteFileSystem methods:
- deprecate FSC::DeleteDataForOriginAndTypeOnFileThread in favor of
DeleteFileSystem
- directly call SandboxMPP::DeleteOriginDataOnFileThread when
SandboxMPP::DeleteFileSystem is called

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:315: callback.Run(*error);
Please consider using PostTaskAndReplyWithResult to get rid of this adapter
function.

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:488:
base::Bind(&DeleteFileSystemOnFileThread,
Wonder if we should rather make FSC::DeleteDataForOriginAndTypeOnFileThread 
return PlatformFileError and use PostTaskAndReplyWithResult?

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:491: base::Unretained(context),
Should we rather pass this with make_ref_ptr?

[nhiroki (google), 2012-07-31 15:50:32]

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.cc:171: void
FileSystemContext::DeleteFileSystem(
I see. I will cleanup in another CL.

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:315: callback.Run(*error);
I think we cannot get rid of like this adapter by your suggestion because there
is no function to run the given callback with PlatformFileError object. That is,
we need to create another (more general) adapter like below:

void RunFileSystemCallback(callback, error) {
    callback.Run(error);
}

Or is there already similar helper function?

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:488:
base::Bind(&DeleteFileSystemOnFileThread,
On 2012/07/30 21:53:38, kinuko wrote:
> Wonder if we should rather make FSC::DeleteDataForOriginAndTypeOnFileThread 
> return PlatformFileError and use PostTaskAndReplyWithResult?

Done.

[kinuko, 2012-07-31 16:59:19]

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:315: callback.Run(*error);
On 2012/07/31 15:50:32, nhiroki wrote:
> I think we cannot get rid of like this adapter by your suggestion because
there
> is no function to run the given callback with PlatformFileError object. That
is,
> we need to create another (more general) adapter like below:
> 
> void RunFileSystemCallback(callback, error) {
>     callback.Run(error);
> }
> 
> Or is there already similar helper function?

Can we just pass callback there?

[nhiroki (google), 2012-07-31 17:17:12]

Thanks for your reviews.

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:315: callback.Run(*error);
I'm really sorry I misunderstood.
I modified to pass callback directly. Done.

http://codereview.chromium.org/10828043/diff/14002/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:491: base::Unretained(context),
On 2012/07/30 21:53:38, kinuko wrote:
> Should we rather pass this with make_ref_ptr?

I forgot a reply. Done.

[kinuko, 2012-07-31 20:10:29]

http://codereview.chromium.org/10828043/diff/13038/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10828043/diff/13038/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.cc:89:
FileSystemContext::DeleteDataForOriginAndTypeOnFileThread(
Since you changed the return code here I think you need to change the callsite
FileSystemQuotaClient::DeleteOriginTask
too.

[nhiroki (google), 2012-08-01 00:15:48]

http://codereview.chromium.org/10828043/diff/13038/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10828043/diff/13038/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.cc:89:
FileSystemContext::DeleteDataForOriginAndTypeOnFileThread(
On 2012/07/31 20:10:30, kinuko wrote:
> Since you changed the return code here I think you need to change the callsite
> FileSystemQuotaClient::DeleteOriginTask
> too.

Done.

[kinuko, 2012-08-01 05:52:03]

http://codereview.chromium.org/10828043/diff/17003/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10828043/diff/17003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3681: callbacks->didSucceed();
hmm why is this changed?  (I might be missing something)

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:474:
make_scoped_refptr(context)),
base::Bind(&FileSystemContext::DeleteDataForOriginAndTypeOnFileThread,
                 context, origin_url, type)

does the same?

Or.. looks like we can directly call this->DeleteOriginDataOnFileThread?

base::Bind(&SandboxMountPointProvider::DeleteOriginDataOnFileThread,
           weak_factory_.GetWeakPtr(),
           context, context->quota_manager_proxy(),
           origin_url, type);

[nhiroki (google), 2012-08-01 20:37:45]

http://codereview.chromium.org/10828043/diff/17003/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10828043/diff/17003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3681: callbacks->didSucceed();
Oh... I didn't mean to change. I guess 'git rebase' failed to apply patchset 2.
This is not intended, so I reverted.

Done.

http://codereview.chromium.org/10828043/diff/17003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3699:
callbacks->didFail(WebKit::WebFileErrorAbort);
Ditto. Done.

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:474:
make_scoped_refptr(context)),
The second case looks like difficult because WeakCalls are only supported for
functions with a void return type (see base/bind_internal.h) and
PostTaskAndReplyWithResult needs callback to return value.

I modified it according to the first case.

[kinuko, 2012-08-01 20:48:45]

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:474:
make_scoped_refptr(context)),
On 2012/08/01 20:37:45, nhiroki wrote:
> The second case looks like difficult because WeakCalls are only supported for
> functions with a void return type (see base/bind_internal.h) and
> PostTaskAndReplyWithResult needs callback to return value.
> 
> I modified it according to the first case.

I see, then is it possible to pass an unretained pointer of SMPP and pass
make_scoped_refptr(context) to the Bind?  (since SMPP is owned by context)

[nhiroki (google), 2012-08-01 22:35:43]

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/17003/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:474:
make_scoped_refptr(context)),
All right. I changed it to pass unretained pointer of SMPP and so on.

And I removed const specifier from DeleteFileSystem interface so as to pass
'this' (that is a pointer of SMPP).

Done.

[kinuko, 2012-08-02 00:38:35]

lgtm thanks!

http://codereview.chromium.org/10828043/diff/15005/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/15005/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:464:
base::Bind(&SandboxMountPointProvider::DeleteOriginDataOnFileThread,
nit: could you add a brief comment that it's safe to pass Unretained(this) here
since context owns it?

http://codereview.chromium.org/10828043/diff/15005/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:467:
base::Unretained(context->quota_manager_proxy()),
nit: I guess you don't need this Unretained decoration?

[nhiroki (google), 2012-08-02 17:38:38]

http://codereview.chromium.org/10828043/diff/15005/webkit/fileapi/sandbox_mou...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/10828043/diff/15005/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:464:
base::Bind(&SandboxMountPointProvider::DeleteOriginDataOnFileThread,
On 2012/08/02 00:38:35, kinuko wrote:
> nit: could you add a brief comment that it's safe to pass Unretained(this)
here
> since context owns it?

Done.

http://codereview.chromium.org/10828043/diff/15005/webkit/fileapi/sandbox_mou...
webkit/fileapi/sandbox_mount_point_provider.cc:467:
base::Unretained(context->quota_manager_proxy()),
In this case, we need to decorate it since QuotaManagerProxy is refcounted type.

[nhiroki (google), 2012-08-02 17:51:59]

ping

Hi, jamesr@.
Could you take a look for owner LGTM?

  - content/renderer

Thanks

[jamesr, 2012-08-02 19:57:33]

content/renderer lgtm

[commit-bot: I haz the power, 2012-08-02 19:58:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@google.com/10828043/6032

[nhiroki (google), 2012-08-02 19:59:10]

Thanks for your reviews!

[commit-bot: I haz the power, 2012-08-02 21:43:46]

Change committed as 149707