[SyncFS] Make ExtensionService usage of SyncWorker asynchronous

chrome/browser/sync_file_system/drive_backend/sync_worker.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync_file_system/drive_backend/sync_worker.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/values.h"
 #include "chrome/browser/drive/drive_api_service.h"
 #include "chrome/browser/drive/drive_notification_manager.h"
 #include "chrome/browser/drive/drive_notification_manager_factory.h"
 #include "chrome/browser/drive/drive_service_interface.h"
 #include "chrome/browser/drive/drive_uploader.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/sync_file_system/drive_backend/callback_helper.h"
 #include "chrome/browser/sync_file_system/drive_backend/conflict_resolver.h"
 #include "chrome/browser/sync_file_system/drive_backend/drive_backend_constants.h"
 #include "chrome/browser/sync_file_system/drive_backend/list_changes_task.h"
 #include "chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer.h"
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.h"
 #include "chrome/browser/sync_file_system/drive_backend/register_app_task.h"
 #include "chrome/browser/sync_file_system/drive_backend/remote_to_local_syncer.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_engine_context.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_task.h"
@@ skipping 21 matching lines @@
 
 namespace {
 
 void EmptyStatusCallback(SyncStatusCode status) {}
 
 }  // namespace
 
 scoped_ptr<SyncWorker> SyncWorker::CreateOnWorker(
     const base::FilePath& base_dir,
     Observer* observer,
-    ExtensionServiceInterface* extension_service,
+    const base::WeakPtr<ExtensionServiceInterface>& extension_service,
     scoped_ptr<SyncEngineContext> sync_engine_context,
     leveldb::Env* env_override) {
   scoped_ptr<SyncWorker> sync_worker(
       new SyncWorker(base_dir,
                      extension_service,
                      sync_engine_context.Pass(),
                      env_override));
   sync_worker->AddObserver(observer);
   sync_worker->Initialize();
 
@@ skipping 274 matching lines @@
 SyncTaskManager* SyncWorker::GetSyncTaskManager() {
   return task_manager_.get();
 }
 
 void SyncWorker::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 SyncWorker::SyncWorker(
     const base::FilePath& base_dir,
-    ExtensionServiceInterface* extension_service,
+    const base::WeakPtr<ExtensionServiceInterface>& extension_service,
     scoped_ptr<SyncEngineContext> sync_engine_context,
     leveldb::Env* env_override)
     : base_dir_(base_dir),
       env_override_(env_override),
       service_state_(REMOTE_SERVICE_TEMPORARY_UNAVAILABLE),
       should_check_conflict_(true),
       should_check_remote_change_(true),
       listing_remote_changes_(false),
       sync_enabled_(false),
       default_conflict_resolution_policy_(
@@ skipping 57 matching lines @@
 
   scoped_ptr<MetadataDatabase> metadata_database =
       initializer->PassMetadataDatabase();
   if (metadata_database)
     context_->SetMetadataDatabase(metadata_database.Pass());
 
   UpdateRegisteredApp();
 }
 
 void SyncWorker::UpdateRegisteredApp() {
-  if (extension_service_)
+  MetadataDatabase* metadata_db = GetMetadataDatabase();
+  DCHECK(metadata_db);
+

[peria, 2014/04/24 02:38:21]

Could you add checking |extension_service_|'s availability?
It may cancel meaningless memory allocations and calling PostTask().

[tzik, 2014/04/24 06:56:55]

To do that, we need to hop to UI thread, which costs higher than saved cost by
the shortcut.

+  scoped_ptr<std::vector<std::string> > app_ids(new std::vector<std::string>);
+  metadata_db->GetRegisteredAppIDs(app_ids.get());
+
+  AppStatusMap* app_status = new AppStatusMap;
+  base::Closure callback =
+      base::Bind(&SyncWorker::DidQueryAppStatus,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(app_status));
+
+  context_->GetUiTaskRunner()->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::QueryAppStatusOnUIThread,
+                 extension_service_,
+                 base::Owned(app_ids.release()),
+                 app_status,
+                 CreateRelayedCallback(callback)));
+

[nhiroki, 2014/04/24 01:39:52]

nit: an extra blank line.

[tzik, 2014/04/24 06:56:55]

Done.

+}
+
+// static
+void SyncWorker::QueryAppStatusOnUIThread(

[nhiroki, 2014/04/24 01:39:52]

(optional) How about moving this into anon namespace?

[tzik, 2014/04/24 06:56:55]

Done.

[nhiroki, 2014/04/24 07:09:07]

Hmm... sorry, I missed QueryAppStatusOnUIThread() depends on "enum AppStatus".
The current non-static version is ok, but the previous one is also ok because it
can make the enum and typedef private.

+    const base::WeakPtr<ExtensionServiceInterface>& extension_service_ptr,
+    const std::vector<std::string>* apps,

[peria, 2014/04/24 02:38:21]

(optional) keep its name |app_ids| for consistency

[tzik, 2014/04/24 06:56:55]

Done.

+    AppStatusMap* status,
+    const base::Closure& callback) {
+  ExtensionServiceInterface* extension_service = extension_service_ptr.get();
+  if (!extension_service)

[nhiroki, 2014/04/24 01:39:52]

In this case you don't need to run a callback, right? How about having a comment
for that?

[tzik, 2014/04/24 06:56:55]

Ah, I completely forgot about calling the callback.
Yes, it's not needed, but let me call it just to keep consistency.

     return;
 
-  MetadataDatabase* metadata_db = GetMetadataDatabase();
-  DCHECK(metadata_db);
-  std::vector<std::string> app_ids;
-  metadata_db->GetRegisteredAppIDs(&app_ids);
-
-  // Update the status of every origin using status from ExtensionService.
-  for (std::vector<std::string>::const_iterator itr = app_ids.begin();
-       itr != app_ids.end(); ++itr) {
+  for (std::vector<std::string>::const_iterator itr = apps->begin();
+       itr != apps->end(); ++itr) {
     const std::string& app_id = *itr;
-        GURL origin =
-            extensions::Extension::GetBaseURLFromExtensionId(app_id);
-
-        // TODO(tzik): Switch |extension_service_| to a wrapper and make this
-        // call async.
-        if (!extension_service_->GetInstalledExtension(app_id)) {
-          // Extension has been uninstalled.
-          // (At this stage we can't know if it was unpacked extension or not,
-          // so just purge the remote folder.)
-          UninstallOrigin(origin,
-                          RemoteFileSyncService::UNINSTALL_AND_PURGE_REMOTE,
-                          base::Bind(&EmptyStatusCallback));
-          continue;
-        }
-        FileTracker tracker;
-        if (!metadata_db->FindAppRootTracker(app_id, &tracker)) {
-          // App will register itself on first run.
-          continue;
-        }
-
-        // TODO(tzik): Switch |extension_service_| to a wrapper and make this
-        // call async.
-        bool is_app_enabled = extension_service_->IsExtensionEnabled(app_id);
-            bool is_app_root_tracker_enabled =
-                tracker.tracker_kind() == TRACKER_KIND_APP_ROOT;
-            if (is_app_enabled && !is_app_root_tracker_enabled)
-              EnableOrigin(origin, base::Bind(&EmptyStatusCallback));
-            else if (!is_app_enabled && is_app_root_tracker_enabled)
-              DisableOrigin(origin, base::Bind(&EmptyStatusCallback));
+    if (!extension_service->GetInstalledExtension(app_id))
+      (*status)[app_id] = APP_STATUS_UNINSTALLED;
+    else if (!extension_service->IsExtensionEnabled(app_id))
+      (*status)[app_id] = APP_STATUS_DISABLED;
+    else
+      (*status)[app_id] = APP_STATUS_ENABLED;
   }

[nhiroki, 2014/04/24 01:39:52]

You need to run |callback| here.

[tzik, 2014/04/24 06:56:55]

Done.

 }
 
+void SyncWorker::DidQueryAppStatus(const AppStatusMap* app_status) {
+  MetadataDatabase* metadata_db = GetMetadataDatabase();
+  DCHECK(metadata_db);
+
+  // Update the status of every origin using status from ExtensionService.
+  for (AppStatusMap::const_iterator itr = app_status->begin();
+       itr != app_status->end(); ++itr) {
+    const std::string& app_id = itr->first;
+    GURL origin = extensions::Extension::GetBaseURLFromExtensionId(app_id);
+
+    if (itr->second == APP_STATUS_UNINSTALLED) {
+      // Extension has been uninstalled.
+      // (At this stage we can't know if it was unpacked extension or not,
+      // so just purge the remote folder.)
+      UninstallOrigin(origin,
+                      RemoteFileSyncService::UNINSTALL_AND_PURGE_REMOTE,
+                      base::Bind(&EmptyStatusCallback));
+      continue;
+    }
+
+    FileTracker tracker;
+    if (!metadata_db->FindAppRootTracker(app_id, &tracker)) {
+      // App will register itself on first run.
+      continue;
+    }
+
+    DCHECK(itr->second == APP_STATUS_ENABLED ||
+           itr->second == APP_STATUS_DISABLED);
+    bool is_app_enabled = itr->second == APP_STATUS_ENABLED;

[peria, 2014/04/24 02:38:21]

nit: a = (b == c);

[tzik, 2014/04/24 06:56:55]

Done.

+    bool is_app_root_tracker_enabled =
+        tracker.tracker_kind() == TRACKER_KIND_APP_ROOT;

[peria, 2014/04/24 02:38:21]

diito

[tzik, 2014/04/24 06:56:55]

Done.

+    if (is_app_enabled && !is_app_root_tracker_enabled)
+      EnableOrigin(origin, base::Bind(&EmptyStatusCallback));
+    else if (!is_app_enabled && is_app_root_tracker_enabled)
+      DisableOrigin(origin, base::Bind(&EmptyStatusCallback));
+  }
+}
+
 void SyncWorker::DidProcessRemoteChange(RemoteToLocalSyncer* syncer,
                                         const SyncFileCallback& callback,
                                         SyncStatusCode status) {
   if (syncer->is_sync_root_deletion()) {
     MetadataDatabase::ClearDatabase(context_->PassMetadataDatabase());
     PostInitializeTask();
     callback.Run(status, syncer->url());
     return;
   }
 
@@ skipping 168 matching lines @@
             "Service state changed: %d->%d: %s",
             old_state, GetCurrentState(), description.c_str());
 
   FOR_EACH_OBSERVER(
       Observer, observers_,
       UpdateServiceState(GetCurrentState(), description));
 }
 
 }  // namespace drive_backend
 }  // namespace sync_file_system