SyncFS: Introduce SyncFileSystemBackend

SyncFS: Introduce SyncFileSystemBackend

This change introduces SyncaFileSystemBackend to remove SyncFS dependencies
from FileSystemContext and SandboxFileSystemBackend.


BUG=242422
TEST=content_unittests
R=kinuko@chromium.org, tzik@chromium.org

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

[nhiroki, 2013-07-08 11:06:22]

Hi, can you review this? Thanks!

[kinuko, 2013-07-08 12:31:20]

Thanks for working on this.  While reviewing the patch unfortunately I realized
we need to shave some more yak.
Also I think this patch can be separated into a few more patches. This change is
not very straightforward, we'll need some more pre- and post- changes.

Please see my comments inline..

https://codereview.chromium.org/18668003/diff/39001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18668003/diff/39001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.h:251: const
fileapi::FileSystemOptions& options,
Fyi, we're changing some params of this method in
https://codereview.chromium.org/18667002/

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:263: GURL root_url =
mount_point_provider->GetRootURI(origin_url, type);
Can't each mount point provider create the root_url (and name) and pass it to
the OpenFileSystem's callback on its own rather than create them here?

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_mount_point_provider.h (right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_mount_point_provider.h:59: FileSystemType
type) const = 0;
I don't think this is necessary here. (Please see the comment in
FileSystemContext::OpenFileSystem)

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_mount_point_provider.h:128: FileSystemType
type) const = 0;
Can we move this method out of FileSystemMountPointProvider but into QuotaUtil? 
This one's meaningful only in the sandboxed context (at least now) and I'm
trying to move all those sandbox-fs only methods to QuotaUtil. (So that
non-sandbox FS modules don't need to care about them)

Also can you move this change into a separate CL?

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.cc
(right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.cc:31:
special_storage_policy) {
Hmm... I realized this needs a bit more design discussion.

We cannot create two ObfuscatedFileUtil (sandbox_file_util_) instance for the
same directory, but needs to use the same instance.
So actually naive inheritance doesn't work very well here.

I can't think of a clean design without bigger refactoring, but I think we can
probably start with something like following (not very clean though):

- Move sandbox_file_util_, file_system_usage_cache_ and
  quota_observer_ out of SandboxMountPointProvider into a
  separate class, like 'SandboxContext', and initialize it
  in FileSystemContext().  SandboxMountPointProvider's ctor
  must be given the context and hold it as not-owned member
  variable instead of creating them in its ctors.  Note that
  the context is shared by multiple MountPointProviders/Backends.
  (This change can be done in a separate patch.)

- Change OpenFileSystem's signature to take FileSystemContext.
  Also maybe rename the method to Initialize or InitializeFileSystem.
  (This change can be done in a separate patch.)

- In this class's OpenFileSystem/Initialize implementation,
  set the SandboxContext held by FileSystemContext to its
  member variable (as not-owned pointer), and use the
  SandboxContext for serving the succeeding method calls.
  (not very clean, but would work)

(If you again hit some other problems feel free to talk to me)

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.h
(right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.h:15: :
public fileapi::SandboxMountPointProvider {
Fyi2, we're changing the MountPointProvider naming to FileSystemBackends
everywhere. (Many pending conflicting changes due to Jul 4!)

https://codereview.chromium.org/18344013/

After the rename patch is submitted maybe we can simply name this class
'SyncFileSystemBackends'?

[nhiroki, 2013-07-17 13:54:28]

Thank you for your comments.

Addressed previous comments in separate CLs, but this is still not
straightforward...
Anyway, can you take another look? If I'm on the right way, I'll try to split
this into some CLs.

Thanks!

[kinuko, 2013-07-17 15:01:30]

https://codereview.chromium.org/18668003/diff/73001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_file_sync_service.cc (right):

https://codereview.chromium.org/18668003/diff/73001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_file_sync_service.cc:291: origin,
backend->change_tracker()->num_changes());
I think these changes can be made later in a separate CL.

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend.cc:138: :
file_task_runner_(file_task_runner),
Why does this need to be passed separately in this CL?

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:56: return
sandbox_context_->sync_file_util();
Have you checked when these GetFileUtil() and GetAsyncFileUtil() are called?  I
doubt these methods need to return non-null value before InitializeFileSystem. 
If it's not I think we should go back to the idea to add a separate Initialize()
method, because it basically means doing LazyInitialize in InitializeFileSystem
is not a good idea.

[kinuko, 2013-07-17 15:02:59]

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend.cc:138: :
file_task_runner_(file_task_runner),
On 2013/07/17 15:01:30, kinuko wrote:
> Why does this need to be passed separately in this CL?

I mean, it used to be passed via sandbox_context but in this CL we pass this as
a separate value.  Can you explain why this change is necessary?

[nhiroki, 2013-07-22 04:34:14]

Comment only (sorry, I forgot to publish responses for the previous comments).

https://codereview.chromium.org/18668003/diff/39001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18668003/diff/39001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.h:251: const
fileapi::FileSystemOptions& options,
On 2013/07/08 12:31:20, kinuko wrote:
> Fyi, we're changing some params of this method in
> https://codereview.chromium.org/18667002/

Rebased. Thanks!

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:263: GURL root_url =
mount_point_provider->GetRootURI(origin_url, type);
On 2013/07/08 12:31:20, kinuko wrote:
> Can't each mount point provider create the root_url (and name) and pass it to
> the OpenFileSystem's callback on its own rather than create them here?

Done in http://crrev.com/211318/

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_mount_point_provider.h (right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_mount_point_provider.h:59: FileSystemType
type) const = 0;
On 2013/07/08 12:31:20, kinuko wrote:
> I don't think this is necessary here. (Please see the comment in
> FileSystemContext::OpenFileSystem)

Removed.

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_mount_point_provider.h:128: FileSystemType
type) const = 0;
On 2013/07/08 12:31:20, kinuko wrote:
> Can we move this method out of FileSystemMountPointProvider but into
QuotaUtil? 
> This one's meaningful only in the sandboxed context (at least now) and I'm
> trying to move all those sandbox-fs only methods to QuotaUtil. (So that
> non-sandbox FS modules don't need to care about them)
> 
> Also can you move this change into a separate CL?

Done in http://crrev.com/211332/

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.cc
(right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.cc:31:
special_storage_policy) {
On 2013/07/08 12:31:20, kinuko wrote:
> Hmm... I realized this needs a bit more design discussion.
> 
> We cannot create two ObfuscatedFileUtil (sandbox_file_util_) instance for the
> same directory, but needs to use the same instance.
> So actually naive inheritance doesn't work very well here.
> 
> I can't think of a clean design without bigger refactoring, but I think we can
> probably start with something like following (not very clean though):
> 
> - Move sandbox_file_util_, file_system_usage_cache_ and
>   quota_observer_ out of SandboxMountPointProvider into a
>   separate class, like 'SandboxContext', and initialize it
>   in FileSystemContext().  SandboxMountPointProvider's ctor
>   must be given the context and hold it as not-owned member
>   variable instead of creating them in its ctors.  Note that
>   the context is shared by multiple MountPointProviders/Backends.
>   (This change can be done in a separate patch.)

Done in http://crrev.com/210790/

> - Change OpenFileSystem's signature to take FileSystemContext.
>   Also maybe rename the method to Initialize or InitializeFileSystem.
>   (This change can be done in a separate patch.)

Done in http://crrev.com/211888/

> - In this class's OpenFileSystem/Initialize implementation,
>   set the SandboxContext held by FileSystemContext to its
>   member variable (as not-owned pointer), and use the
>   SandboxContext for serving the succeeding method calls.
>   (not very clean, but would work)

Done in this change.

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.h
(right):

https://codereview.chromium.org/18668003/diff/39001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/syncable_sandbox_mount_point_provider.h:15: :
public fileapi::SandboxMountPointProvider {
On 2013/07/08 12:31:20, kinuko wrote:
> Fyi2, we're changing the MountPointProvider naming to FileSystemBackends
> everywhere. (Many pending conflicting changes due to Jul 4!)
> 
> https://codereview.chromium.org/18344013/
> 
> After the rename patch is submitted maybe we can simply name this class
> 'SyncFileSystemBackends'?

Thanks! Rebased and renamed.

https://codereview.chromium.org/18668003/diff/73001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_file_sync_service.cc (right):

https://codereview.chromium.org/18668003/diff/73001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_file_sync_service.cc:291: origin,
backend->change_tracker()->num_changes());
On 2013/07/17 15:01:30, kinuko wrote:
> I think these changes can be made later in a separate CL.

Hmm... these changes are needed for compiling successfully. You mean we should
just introduce SyncFileSystemBackend and should NOT use it in this CL?

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend.cc:138: :
file_task_runner_(file_task_runner),
On 2013/07/17 15:02:59, kinuko wrote:
> On 2013/07/17 15:01:30, kinuko wrote:
> > Why does this need to be passed separately in this CL?
> 
> I mean, it used to be passed via sandbox_context but in this CL we pass this
as
> a separate value.  Can you explain why this change is necessary?

This is because that SyncFileSystemBackend::set_change_tracker needs the runner
before calling InitializeFileSystem().

Probably it is no longer necessary to pass separately because of calling
LazyInitialize() in GetFileUtil().

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:56: return
sandbox_context_->sync_file_util();
On 2013/07/17 15:01:30, kinuko wrote:
> Have you checked when these GetFileUtil() and GetAsyncFileUtil() are called? 
I
> doubt these methods need to return non-null value before InitializeFileSystem.

> If it's not I think we should go back to the idea to add a separate
Initialize()
> method, because it basically means doing LazyInitialize in
InitializeFileSystem
> is not a good idea.

Yes, I have checked. There is possibility to call those functions before calling
InitializeFileSystem().

(1) GetFileUtil() is called from
LocalFileChangeTracker::CollectLastDirtyChanges() on initialization.
(2) GetAsyncFileUtil() is called when creating LocalFileSystemOperation object
via FileSystemBackend::CreateFileSystemOperation().

[kinuko, 2013-07-22 05:11:30]

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:56: return
sandbox_context_->sync_file_util();
On 2013/07/22 04:34:14, nhiroki wrote:
> On 2013/07/17 15:01:30, kinuko wrote:
> > Have you checked when these GetFileUtil() and GetAsyncFileUtil() are called?

> I
> > doubt these methods need to return non-null value before
InitializeFileSystem.
> 
> > If it's not I think we should go back to the idea to add a separate
> Initialize()
> > method, because it basically means doing LazyInitialize in
> InitializeFileSystem
> > is not a good idea.
> 
> Yes, I have checked. There is possibility to call those functions before
calling
> InitializeFileSystem().
> 
> (1) GetFileUtil() is called from
> LocalFileChangeTracker::CollectLastDirtyChanges() on initialization.
> (2) GetAsyncFileUtil() is called when creating LocalFileSystemOperation object
> via FileSystemBackend::CreateFileSystemOperation().

1) This is happening because we try to initialize LocalFileSyncContext and
LocalFileChangeTracker before we call FileSystemContext::OpenFileSystem.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

We plan to unifying these syncfs initialization which resides in
chrome/browser/syncfs into SyncFileSystemBackend::InitializeFileSystem
eventually so it should go away soon, but until then could we reorder the
initialization (e.g. can we first call OpenFileSystem and then call
InitializeForApp)?

2) How can CreateFileSystemOperation be called without opening a filesystem?  Is
it test only or also called in the production code?  If it's only in testing
could we fix the tests?

If both or either one of the cases look reasonable and inevitable I think we
should revert the previous InitializeFileSystem change but re-add a separate
Initialize method (unfortunately).

[kinuko, 2013-07-22 05:51:19]

(additional nit)

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (left):

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:329: void
FileSystemContext::SetLocalFileChangeTracker(
Can you separate this change when you upload a patch for review?  I think this
change can be made separately, before or after this CL

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:163: void
SyncFileSystemBackend::set_change_tracker(
When a setter is not inline and do more than just assigning a value, I think we
usually use regular method naming style (i.e. SetChangeTracker).

[nhiroki, 2013-07-22 06:33:37]

Partially updated (not ready for review yet)

https://codereview.chromium.org/18668003/diff/73001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_file_sync_service.cc (right):

https://codereview.chromium.org/18668003/diff/73001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_file_sync_service.cc:291: origin,
backend->change_tracker()->num_changes());
On 2013/07/17 15:01:30, kinuko wrote:
> I think these changes can be made later in a separate CL.

Reverted. I'll make a separate CL.

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
File webkit/browser/fileapi/sandbox_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sa...
webkit/browser/fileapi/sandbox_file_system_backend.cc:138: :
file_task_runner_(file_task_runner),
On 2013/07/22 04:34:14, nhiroki wrote:
> On 2013/07/17 15:02:59, kinuko wrote:
> > On 2013/07/17 15:01:30, kinuko wrote:
> > > Why does this need to be passed separately in this CL?
> > 
> > I mean, it used to be passed via sandbox_context but in this CL we pass this
> as
> > a separate value.  Can you explain why this change is necessary?
> 
> This is because that SyncFileSystemBackend::set_change_tracker needs the
runner
> before calling InitializeFileSystem().
> 
> Probably it is no longer necessary to pass separately because of calling
> LazyInitialize() in GetFileUtil().

Pushed file_task_runner into SandboxContext again.

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (left):

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:329: void
FileSystemContext::SetLocalFileChangeTracker(
On 2013/07/22 05:51:19, kinuko wrote:
> Can you separate this change when you upload a patch for review?  I think this
> change can be made separately, before or after this CL

Reverted those setter/getter related changes.

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/86002/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:163: void
SyncFileSystemBackend::set_change_tracker(
On 2013/07/22 05:51:19, kinuko wrote:
> When a setter is not inline and do more than just assigning a value, I think
we
> usually use regular method naming style (i.e. SetChangeTracker).

Removed this setter for now.

[nhiroki, 2013-07-23 05:02:12]

Updated. Can you take another look?

This round added Initialize() function to FileSystemBackend. I checked this
works well on tests, and will check on real app (e.g. syncfs-editor). If looks
good overall to you, I'll make separate patches to add the function and to
revert InitializeFilesystem() related change.

Thanks!

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/73001/webkit/browser/fileapi/sy...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:56: return
sandbox_context_->sync_file_util();
On 2013/07/22 05:11:30, kinuko wrote:
> On 2013/07/22 04:34:14, nhiroki wrote:
> > On 2013/07/17 15:01:30, kinuko wrote:
> > > Have you checked when these GetFileUtil() and GetAsyncFileUtil() are
called?
> 
> > I
> > > doubt these methods need to return non-null value before
> InitializeFileSystem.
> > 
> > > If it's not I think we should go back to the idea to add a separate
> > Initialize()
> > > method, because it basically means doing LazyInitialize in
> > InitializeFileSystem
> > > is not a good idea.
> > 
> > Yes, I have checked. There is possibility to call those functions before
> calling
> > InitializeFileSystem().
> > 
> > (1) GetFileUtil() is called from
> > LocalFileChangeTracker::CollectLastDirtyChanges() on initialization.
> > (2) GetAsyncFileUtil() is called when creating LocalFileSystemOperation
object
> > via FileSystemBackend::CreateFileSystemOperation().
> 
> 1) This is happening because we try to initialize LocalFileSyncContext and
> LocalFileChangeTracker before we call FileSystemContext::OpenFileSystem.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> We plan to unifying these syncfs initialization which resides in
> chrome/browser/syncfs into SyncFileSystemBackend::InitializeFileSystem
> eventually so it should go away soon, but until then could we reorder the
> initialization (e.g. can we first call OpenFileSystem and then call
> InitializeForApp)?
> 
> 2) How can CreateFileSystemOperation be called without opening a filesystem? 
Is
> it test only or also called in the production code?  If it's only in testing
> could we fix the tests?
> 
> If both or either one of the cases look reasonable and inevitable I think we
> should revert the previous InitializeFileSystem change but re-add a separate
> Initialize method (unfortunately).

Both cases looks inevitable, so I introduced the separate Initialize() function
into FileSystemBackend as per offline talk with kinuko-san.

[kinuko, 2013-07-23 06:41:50]

Looking much better/promising, SandboxFSBackend/SyncFSBackend separation would
need some more cleanup, but feel free to address my comments below in a separate
CL.

> Both cases looks inevitable, so I introduced the separate Initialize()
function
> into FileSystemBackend as per offline talk with kinuko-san.

Strictly speaking I think they're not inevitable but require more work and
probably uglier than adding Initialize().

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:139: RegisterBackend(*iter);
I'd have another loop for Initialize after this loop, so that each backend
doesn't need to care about registration order in Initialize().

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:237: }
Can't this be simply written like:

if (backend->GetQuotaUtil())
  return backend->GetQuotaUtil()->GetUpdateObservers(type);
return NULL;

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:251: }
ditto

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/sandbox_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/sandbox_file_system_backend.cc:164: }
It'd be more consistent to move all sandbox_context related initialization into
Initialize()?

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/sandbox_file_system_backend.h (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/sandbox_file_system_backend.h:176: AccessObserverList
access_observers_;
Can we keep them as private members but provide protected accessors?

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/sandbox_file_system_backend.h:181: bool
enable_usage_tracking_;
I wonder if it's cleaner to move this into SandboxContext?

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:62: origin_url,
type, mode, context, callback);
We're implementing SyncFS's own InitializeFileSystem (OpenFileSystem), right?

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:114:
UMA_HISTOGRAM_COUNTS(kSyncableOriginsCountLabel, origins->size());
I think we don't need to add this UMA for this type

[nhiroki, 2013-07-24 05:58:47]

Updated, PTAL.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:139: RegisterBackend(*iter);
On 2013/07/23 06:41:51, kinuko wrote:
> I'd have another loop for Initialize after this loop, so that each backend
> doesn't need to care about registration order in Initialize().

Done in a separate CL.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:237: }
On 2013/07/23 06:41:51, kinuko wrote:
> Can't this be simply written like:
> 
> if (backend->GetQuotaUtil())
>   return backend->GetQuotaUtil()->GetUpdateObservers(type);
> return NULL;

Done.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:251: }
On 2013/07/23 06:41:51, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/sandbox_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/sandbox_file_system_backend.cc:164: }
On 2013/07/23 06:41:51, kinuko wrote:
> It'd be more consistent to move all sandbox_context related initialization
into
> Initialize()?

Done in the separate CL.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/sandbox_file_system_backend.h (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/sandbox_file_system_backend.h:176: AccessObserverList
access_observers_;
On 2013/07/23 06:41:51, kinuko wrote:
> Can we keep them as private members but provide protected accessors?

Done.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/sandbox_file_system_backend.h:181: bool
enable_usage_tracking_;
On 2013/07/23 06:41:51, kinuko wrote:
> I wonder if it's cleaner to move this into SandboxContext?

Done in http://crrev.com/213070/

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:62: origin_url,
type, mode, context, callback);
On 2013/07/23 06:41:51, kinuko wrote:
> We're implementing SyncFS's own InitializeFileSystem (OpenFileSystem), right?

That's right. I mean to implement it in the following CLs.

https://codereview.chromium.org/18668003/diff/111001/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:114:
UMA_HISTOGRAM_COUNTS(kSyncableOriginsCountLabel, origins->size());
On 2013/07/23 06:41:51, kinuko wrote:
> I think we don't need to add this UMA for this type

Done.

[kinuko, 2013-07-24 06:04:06]

Let me send this comment earlier than other comments

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:238: SandboxContext*
sandbox_context() const { return sandbox_context_.get(); }
We're returning a pointer to a member variable, unless we make it return const
pointer please not to make this method const.

(I guess you may need to change Initialize to take non-const FileSystemContext
as well)

[kinuko, 2013-07-24 06:18:30]

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/s...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:44:
sandbox_context()->quota_observer(), NULL));
When do we use the parent class's update_observers?  Why do we need to update
the parent class's observers here?  Can we just call
SandboxFileSystemBackend::Initialize instead?

This SandboxFSBackend - SyncFSBackend's inheritance relationship is not really
great right now, I feel a little more time should be spent to think about what
will make this separation clearer.

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:57:
SandboxFileSystemBackend::OpenFileSystem(origin_url, type, mode, callback);
What's your plan to proceed? I'm slightly not sure why this is separated out.

[nhiroki, 2013-07-24 10:22:58]

Thank you for your comments! PTAL.

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:238: SandboxContext*
sandbox_context() const { return sandbox_context_.get(); }
On 2013/07/24 06:04:06, kinuko wrote:
> We're returning a pointer to a member variable, unless we make it return const
> pointer please not to make this method const.
> 
> (I guess you may need to change Initialize to take non-const FileSystemContext
> as well)

Done.

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/s...
File webkit/browser/fileapi/syncable/sync_file_system_backend.cc (right):

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:44:
sandbox_context()->quota_observer(), NULL));
On 2013/07/24 06:18:30, kinuko wrote:
> When do we use the parent class's update_observers? 

Right, I realized we don't need to treat the those observers in Add*Observer()
and Get*Observers(). Removed them.

> Why do we need to update the parent class's observers here?
> Can we just call SandboxFileSystemBackend::Initialize instead?

As I mentioned on offine chat, this is because I wanted to avoid calling
MaybePrepopulateDatabase() twice from SandboxFSBackend::Initialize() and
SyncFSBackend::Initialize(). Moved the function into SandboxContext so that we
can call SandboxFSB::Initialize() here.

> This SandboxFSBackend - SyncFSBackend's inheritance relationship is not really
> great right now, I feel a little more time should be spent to think about what
> will make this separation clearer.

I think the situation is somewhat getting better by this latest change. WDYT?

https://codereview.chromium.org/18668003/diff/130002/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.cc:57:
SandboxFileSystemBackend::OpenFileSystem(origin_url, type, mode, callback);
On 2013/07/24 06:18:30, kinuko wrote:
> What's your plan to proceed? I'm slightly not sure why this is separated out.

Let me explain... it was my plan that the following CL extracts common routine
from SandboxFS:OpenFileSystem into somewhere and adds start-up routine for the
sync service which could be called if the service hasn't started (i.e. called
via resolveLocalFileSystemURL()).

Anyway, added OpenFileSystemInternal() which does the common routine for both
backends.

[nhiroki, 2013-07-30 03:29:15]

I made sure that we can introduce SyncFileSystemBackend without inheritance or
composition (see PatchSet #11). I plan to proceed as per the following order:

1) Move FileSystemQuotaUtil related functions from SandboxFileSystemBackend to
SandboxContext
2) Move OpenFileSystemInternal to SandboxContext
3) Add SyncFileSystemBackend (this CL)

[nhiroki, 2013-08-01 06:09:23]

Updated! Can you take another look?

On 2013/07/30 03:29:15, nhiroki wrote:
> 1) Move FileSystemQuotaUtil related functions from SandboxFileSystemBackend to
> SandboxContext

Done in https://codereview.chromium.org/21116008/

> 2) Move OpenFileSystemInternal to SandboxContext

Done in https://codereview.chromium.org/21305005/

[kinuko, 2013-08-01 07:20:01]

lgtm

https://codereview.chromium.org/18668003/diff/192001/webkit/browser/fileapi/s...
File webkit/browser/fileapi/syncable/sync_file_system_backend.h (right):

https://codereview.chromium.org/18668003/diff/192001/webkit/browser/fileapi/s...
webkit/browser/fileapi/syncable/sync_file_system_backend.h:52: //
FileSystemQuotaUtil overrides.
It feels if we keep fs_type:observer mapping outside each backend and remove
{Add,Get}Observer methods we can just make SandboxContext implement QuotaUtil
and return the context for GetQuotaUtil().

[nhiroki, 2013-08-01 08:10:26]

Thanks for reviewing!

On 2013/08/01 07:20:01, kinuko wrote:
> lgtm
> 
>
https://codereview.chromium.org/18668003/diff/192001/webkit/browser/fileapi/s...
> File webkit/browser/fileapi/syncable/sync_file_system_backend.h (right):
> 
>
https://codereview.chromium.org/18668003/diff/192001/webkit/browser/fileapi/s...
> webkit/browser/fileapi/syncable/sync_file_system_backend.h:52: //
> FileSystemQuotaUtil overrides.
> It feels if we keep fs_type:observer mapping outside each backend and remove
> {Add,Get}Observer methods we can just make SandboxContext implement QuotaUtil
> and return the context for GetQuotaUtil().

Yes, I was thinking the same thing. I try to do it later.

[tzik, 2013-08-02 02:20:58]

lgtm

[commit-bot: I haz the power, 2013-08-02 02:32:10]

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

[commit-bot: I haz the power, 2013-08-02 02:58:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-02 03:23:50]

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

[nhiroki, 2013-08-02 04:58:56]

Committed patchset #14 manually as r215222 (presubmit successful).