FileAPI: Move FileSystemQuotaUtil related functions into SandboxContext

FileAPI: Move FileSystemQuotaUtil related functions into SandboxContext

This moves FileSystemQuotaUtil related functions from SandboxFileSystemBackend
to SandboxContext so that SyncFileSystemBackend which will be introduced in the
following CL can use the same code.

This does not include any behavioral changes of those functions.

This is a preparation patch to introduce SyncFileSystemBackend
https://codereview.chromium.org/18668003/


BUG=242422
TEST=content_unittests
NOTRY=true
TBR=jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214977

[nhiroki, 2013-07-30 07:06:04]

PTAL, thanks!

[kinuko, 2013-07-30 08:22:20]

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.cc (right):

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.cc:68: }
This method probably shouldn't be here.  For IsAccessValid() the type check must
be done by each Backend, and for other quota util methods I think we can remove
the DCHECKs.

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.h (right):

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:92: // FileSystemQuotaUtil
implementations.
nit: this sounds like it's implementing QuotaUtil methods. 'FileSystemQuotaUtil
helpers' ?

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:133: ObfuscatedFileUtil*
sync_file_util();
nit: do we need both file_util() and sync_file_util() separately?

[nhiroki, 2013-07-30 11:56:44]

Updated. Please take another look.

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.cc (right):

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.cc:68: }
On 2013/07/30 08:22:20, kinuko wrote:
> This method probably shouldn't be here.  For IsAccessValid() the type check
must
> be done by each Backend, and for other quota util methods I think we can
remove
> the DCHECKs.

Done.

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.h (right):

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:92: // FileSystemQuotaUtil
implementations.
On 2013/07/30 08:22:20, kinuko wrote:
> nit: this sounds like it's implementing QuotaUtil methods.
'FileSystemQuotaUtil
> helpers' ?

Done.

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:133: ObfuscatedFileUtil*
sync_file_util();
On 2013/07/30 08:22:20, kinuko wrote:
> nit: do we need both file_util() and sync_file_util() separately?

Well... if we remove...

a) sync_file_util(), we probably would like another helper function like
sync_file_util() in both SandboxContext and SandboxFSBackend to avoid annoying
static_cast<ObfuscatedFileUtil*>().

b) file_util(), the backend has no way to get a pointer of AsyncFileUtil in
SandboxFSBackend::GetAsyncFileUtil(). 

So, I'd prefer to leave both functions. WDYT?

[kinuko, 2013-07-31 06:12:34]

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.h (right):

https://codereview.chromium.org/21116008/diff/16001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:133: ObfuscatedFileUtil*
sync_file_util();
On 2013/07/30 11:56:44, nhiroki wrote:
> On 2013/07/30 08:22:20, kinuko wrote:
> > nit: do we need both file_util() and sync_file_util() separately?
> 
> Well... if we remove...
> 
> a) sync_file_util(), we probably would like another helper function like
> sync_file_util() in both SandboxContext and SandboxFSBackend to avoid annoying
> static_cast<ObfuscatedFileUtil*>().
> 
> b) file_util(), the backend has no way to get a pointer of AsyncFileUtil in
> SandboxFSBackend::GetAsyncFileUtil(). 
> 
> So, I'd prefer to leave both functions. WDYT?

Ok, let's keep them.

https://codereview.chromium.org/21116008/diff/36001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc (right):

https://codereview.chromium.org/21116008/diff/36001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc:97:
context_->set_file_system_options(options);
Is set_file_system_option added only for this?

Looks like we should just reset the context too?
(Before we split context out of backend we were re-creating Backend here, so if
we do the same resetting context here sounds more reasonable to me)

https://codereview.chromium.org/21116008/diff/49001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.h (right):

https://codereview.chromium.org/21116008/diff/49001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:113: FileSystemType type);
These are public methods (and in the current design we'll need to expose them in
content/public layer), having 'Impl' suffix feels a bit irrelevant. Could we
just remove the suffix?

[nhiroki, 2013-07-31 07:23:19]

Thanks! Updated.

https://codereview.chromium.org/21116008/diff/36001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc (right):

https://codereview.chromium.org/21116008/diff/36001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend_unittest.cc:97:
context_->set_file_system_options(options);
On 2013/07/31 06:12:34, kinuko wrote:
> Is set_file_system_option added only for this?

Right.

> Looks like we should just reset the context too?
> (Before we split context out of backend we were re-creating Backend here, so
if
> we do the same resetting context here sounds more reasonable to me)

I see. Done.

https://codereview.chromium.org/21116008/diff/49001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.h (right):

https://codereview.chromium.org/21116008/diff/49001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.h:113: FileSystemType type);
On 2013/07/31 06:12:34, kinuko wrote:
> These are public methods (and in the current design we'll need to expose them
in
> content/public layer), having 'Impl' suffix feels a bit irrelevant. Could we
> just remove the suffix?

Done.

[kinuko, 2013-07-31 07:35:53]

lgtm

https://codereview.chromium.org/21116008/diff/72001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.cc (right):

https://codereview.chromium.org/21116008/diff/72001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.cc:27: const char kDisableUsageTracking[]
= "disable-file-system-usage-tracking";
Wasn't this deleted?

https://codereview.chromium.org/21116008/diff/72001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.cc:171: origin_url, type);
nit: indent

[nhiroki, 2013-07-31 07:52:08]

Thank you for reviewing!

https://codereview.chromium.org/21116008/diff/72001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_context.cc (right):

https://codereview.chromium.org/21116008/diff/72001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.cc:27: const char kDisableUsageTracking[]
= "disable-file-system-usage-tracking";
On 2013/07/31 07:35:53, kinuko wrote:
> Wasn't this deleted?

Oops, good catch! I deleted this in the previous change, but failed to rebase.
Anyway, removed.

https://codereview.chromium.org/21116008/diff/72001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_context.cc:171: origin_url, type);
On 2013/07/31 07:35:53, kinuko wrote:
> nit: indent

Done.

[tzik, 2013-08-01 03:53:59]

lgtm

[nhiroki, 2013-08-01 04:08:24]

TBRing jochen@ for content/content_tests.gypi (adding just one file)

[commit-bot: I haz the power, 2013-08-01 04:29:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/21116008/83001

[commit-bot: I haz the power, 2013-08-01 06:24:18]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-01 06:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/21116008/83001

[commit-bot: I haz the power, 2013-08-01 06:29:54]

Change committed as 214977

[jochen (gone - plz use gerrit), 2013-08-01 06:40:31]

lgtm