SyncFS: Reorder initialization sequence of SyncFileSystemService

chrome/browser/sync_file_system/local/sync_file_system_backend.cc

Index: chrome/browser/sync_file_system/local/sync_file_system_backend.cc
diff --git a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
index ac426c9bca6f8f8b0f8c2052ebe7e0fefcc1992c..974f67dad73878953c3512ba1a522f51dce531ce 100644
--- a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
+++ b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
@@ -5,10 +5,15 @@
 #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h"
 
 #include "base/logging.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/sync_file_system/local/local_file_change_tracker.h"
 #include "chrome/browser/sync_file_system/local/local_file_sync_context.h"
 #include "chrome/browser/sync_file_system/local/syncable_file_system_operation.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service_factory.h"
 #include "chrome/browser/sync_file_system/syncable_file_system_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "webkit/browser/fileapi/file_system_context.h"
 #include "webkit/browser/fileapi/file_system_file_stream_reader.h"
 #include "webkit/browser/fileapi/file_system_operation_impl.h"
@@ -16,17 +21,75 @@
 #include "webkit/browser/fileapi/sandbox_quota_observer.h"
 #include "webkit/common/fileapi/file_system_util.h"
 
+using content::BrowserThread;
+
 namespace sync_file_system {
 
-SyncFileSystemBackend::SyncFileSystemBackend()
-    : delegate_(NULL) {
+namespace {
+
+bool CalledOnUIThread() {
+  // Ensure that these methods are called on the UI thread, except for unittests
+  // where a UI thread might not have been created.
+  return BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsMessageLoopValid(BrowserThread::UI);
+}
+
+}  // namespace
+
+SyncFileSystemBackend::ProfileHolder::ProfileHolder(Profile* profile)
+    : profile_(profile) {
+  DCHECK(CalledOnUIThread());
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_PROFILE_DESTROYED,
+                 content::NotificationService::AllSources());

[kinuko, 2013/08/28 11:24:45]

content::Source<Profile>(profile_) ?

[nhiroki, 2013/08/29 04:31:34]

Done.

+}
+
+void SyncFileSystemBackend::ProfileHolder::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(CalledOnUIThread());
+  switch (type) {
+    case chrome::NOTIFICATION_PROFILE_DESTROYED: {

[kinuko, 2013/08/28 11:24:45]

nit: maybe DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type) and no
DCHECK?

[nhiroki, 2013/08/29 04:31:34]

Replaced with DCHECK_EQ(...).

Sorry, I couldn't get what "no DCHECK" means. You mean I can remove line 51...? 

[kinuko, 2013/08/29 08:28:21]

d'oh, I think I wanted to say no switch/case.

+      Profile* profile = content::Source<Profile>(source).ptr();

[kinuko, 2013/08/28 11:24:45]

nit: probably you don't need store it to a temp variable.

[nhiroki, 2013/08/29 04:31:34]

Done.

+      if (profile_ == profile)
+        profile_ = NULL;

[kinuko, 2013/08/28 11:24:45]

I think here this can remove itself from the registrar.

registrar_.RemoveAll();

[nhiroki, 2013/08/29 04:31:34]

Done.

+      break;
+    }
+  }
+}
+
+Profile* SyncFileSystemBackend::ProfileHolder::GetProfile() {
+  DCHECK(CalledOnUIThread());
+  return profile_;
+}
+
+SyncFileSystemBackend::SyncFileSystemBackend(Profile* profile)
+    : context_(NULL),
+      skip_initialize_syncfs_service_for_testing_(false) {
+  DCHECK(CalledOnUIThread());
+  if (profile)
+    profile_holder_.reset(new ProfileHolder(profile));
 }
 
 SyncFileSystemBackend::~SyncFileSystemBackend() {
   if (change_tracker_) {
-    delegate_->file_task_runner()->DeleteSoon(
+    GetDelegate()->file_task_runner()->DeleteSoon(
         FROM_HERE, change_tracker_.release());
   }
+
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {

[kinuko, 2013/08/28 11:24:45]

Also check profile_holder_ is not null?

[nhiroki, 2013/08/29 04:31:34]

Done.

+    BrowserThread::DeleteSoon(
+        BrowserThread::UI, FROM_HERE, profile_holder_.release());
+  }
+}
+
+// static
+SyncFileSystemBackend* SyncFileSystemBackend::CreateForTesting() {
+  DCHECK(CalledOnUIThread());
+  SyncFileSystemBackend* backend = new SyncFileSystemBackend(NULL);
+  backend->skip_initialize_syncfs_service_for_testing_ = true;
+  return backend;
 }
 
 bool SyncFileSystemBackend::CanHandleType(
@@ -37,17 +100,18 @@ bool SyncFileSystemBackend::CanHandleType(
 
 void SyncFileSystemBackend::Initialize(fileapi::FileSystemContext* context) {
   DCHECK(context);
-  DCHECK(!delegate_);
-  delegate_ = context->sandbox_delegate();
+  DCHECK(!context_);
+  context_ = context;
 
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
-  delegate_->AddFileUpdateObserver(
+      delegate->quota_observer(),
+      delegate->file_task_runner());
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncableForInternalSync,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
+      delegate->quota_observer(),
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::OpenFileSystem(
@@ -56,21 +120,27 @@ void SyncFileSystemBackend::OpenFileSystem(
     fileapi::OpenFileSystemMode mode,
     const OpenFileSystemCallback& callback) {
   DCHECK(CanHandleType(type));
-  DCHECK(delegate_);
-  delegate_->OpenFileSystem(origin_url, type, mode, callback,
-                            GetSyncableFileSystemRootURI(origin_url));
+
+  if (skip_initialize_syncfs_service_for_testing_) {
+    GetDelegate()->OpenFileSystem(origin_url, type, mode, callback,
+                                  GetSyncableFileSystemRootURI(origin_url));
+    return;
+  }
+
+  SyncStatusCallback initialize_callback =
+      base::Bind(&SyncFileSystemBackend::DidInitializeSyncFileSystemService,
+                 base::Unretained(this), origin_url, type, mode, callback);

[kinuko, 2013/08/28 11:24:45]

Why we use Unretained here (and else)?

[nhiroki, 2013/08/29 04:31:34]

Hmm... yes, we have to manage lifetime of SyncFileSystemBackend appropriately. I
changed here to increment the ref-count of FileSystemContext owning the backend.

+  InitializeSyncFileSystemService(origin_url, initialize_callback);
 }
 
 fileapi::FileSystemFileUtil* SyncFileSystemBackend::GetFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->sync_file_util();
+  return GetDelegate()->sync_file_util();
 }
 
 fileapi::AsyncFileUtil* SyncFileSystemBackend::GetAsyncFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->file_util();
+  return GetDelegate()->file_util();
 }
 
 fileapi::CopyOrMoveFileValidatorFactory*
@@ -91,9 +161,8 @@ SyncFileSystemBackend::CreateFileSystemOperation(
   DCHECK(context);
   DCHECK(error_code);
 
-  DCHECK(delegate_);
   scoped_ptr<fileapi::FileSystemOperationContext> operation_context =
-      delegate_->CreateFileSystemOperationContext(url, context, error_code);
+      GetDelegate()->CreateFileSystemOperationContext(url, context, error_code);
   if (!operation_context)
     return NULL;
 
@@ -113,8 +182,7 @@ SyncFileSystemBackend::CreateFileStreamReader(
     const base::Time& expected_modification_time,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamReader(
+  return GetDelegate()->CreateFileStreamReader(
       url, offset, expected_modification_time, context);
 }
 
@@ -124,13 +192,12 @@ SyncFileSystemBackend::CreateFileStreamWriter(
     int64 offset,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamWriter(
+  return GetDelegate()->CreateFileStreamWriter(
       url, offset, context, fileapi::kFileSystemTypeSyncableForInternalSync);
 }
 
 fileapi::FileSystemQuotaUtil* SyncFileSystemBackend::GetQuotaUtil() {
-  return delegate_;
+  return GetDelegate();
 }
 
 // static
@@ -148,15 +215,15 @@ void SyncFileSystemBackend::SetLocalFileChangeTracker(
   DCHECK(tracker);
   change_tracker_ = tracker.Pass();
 
-  DCHECK(delegate_);
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
-  delegate_->AddFileChangeObserver(
+      delegate->file_task_runner());
+  delegate->AddFileChangeObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::set_sync_context(
@@ -165,4 +232,63 @@ void SyncFileSystemBackend::set_sync_context(
   sync_context_ = sync_context;
 }
 
+fileapi::SandboxFileSystemBackendDelegate*
+SyncFileSystemBackend::GetDelegate() const {
+  DCHECK(context_);
+  DCHECK(context_->sandbox_delegate());
+  return context_->sandbox_delegate();
+}
+
+void SyncFileSystemBackend::InitializeSyncFileSystemService(
+    const GURL& origin_url,
+    const SyncStatusCallback& callback) {
+  // Repost to switch from IO thread to UI thread.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&SyncFileSystemBackend::InitializeSyncFileSystemService,
+                   base::Unretained(this), origin_url, callback));
+    return;
+  }
+
+  if (!profile_holder_->GetProfile()) {
+    // Profile was destroyed.
+    callback.Run(SYNC_FILE_ERROR_FAILED);
+    return;
+  }
+
+  SyncFileSystemService* service = SyncFileSystemServiceFactory::GetForProfile(
+          profile_holder_->GetProfile());
+  DCHECK(service);
+  service->InitializeForApp(context_, origin_url, callback);
+}
+
+void SyncFileSystemBackend::DidInitializeSyncFileSystemService(
+    const GURL& origin_url,
+    fileapi::FileSystemType type,
+    fileapi::OpenFileSystemMode mode,
+    const OpenFileSystemCallback& callback,
+    SyncStatusCode status) {
+  // Repost to switch from UI thread to IO thread.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SyncFileSystemBackend::DidInitializeSyncFileSystemService,
+                   base::Unretained(this),
+                   origin_url, type, mode, callback, status));
+    return;
+  }
+
+  if (status != sync_file_system::SYNC_STATUS_OK) {
+    callback.Run(GURL(), std::string(),
+                 SyncStatusCodeToPlatformFileError(status));
+    return;
+  }
+
+  GetDelegate()->OpenFileSystem(origin_url, type, mode, callback,
+                                GetSyncableFileSystemRootURI(origin_url));
+}
+
 }  // namespace sync_file_system