Lazily build the app list model.

chrome/browser/ui/app_list/app_list_syncable_service.cc

 // Copyright 2013 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/ui/app_list/app_list_syncable_service.h"
 
 #include "base/command_line.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/apps/drive/drive_app_provider.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_prefs.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
 #include "chrome/browser/ui/app_list/extension_app_item.h"
 #include "chrome/browser/ui/app_list/extension_app_model_builder.h"
 #include "chrome/browser/ui/app_list/model_pref_updater.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/generated_resources.h"
-#include "content/public/browser/notification_source.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/uninstall_reason.h"
 #include "extensions/common/constants.h"
 #include "sync/api/sync_change_processor.h"
 #include "sync/api/sync_data.h"
 #include "sync/api/sync_merge_result.h"
 #include "sync/protocol/sync.pb.h"
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
@@ skipping 141 matching lines @@
 }
 
 // AppListSyncableService::ModelObserver
 
 class AppListSyncableService::ModelObserver : public AppListModelObserver {
  public:
   explicit ModelObserver(AppListSyncableService* owner)
       : owner_(owner),
         adding_item_(NULL) {
     DVLOG(2) << owner_ << ": ModelObserver Added";
-    owner_->model()->AddObserver(this);
+    owner_->GetModel()->AddObserver(this);
   }
 
   ~ModelObserver() override {
-    owner_->model()->RemoveObserver(this);
+    owner_->GetModel()->RemoveObserver(this);
     DVLOG(2) << owner_ << ": ModelObserver Removed";
   }
 
  private:
   // AppListModelObserver
   void OnAppListItemAdded(AppListItem* item) override {
     DCHECK(!adding_item_);
     adding_item_ = item;  // Ignore updates while adding an item.
     VLOG(2) << owner_ << " OnAppListItemAdded: " << item->ToDebugString();
     owner_->AddOrUpdateFromSyncItem(item);
@@ skipping 38 matching lines @@
       model_(new AppListModel),
       initial_sync_data_processed_(false),
       first_app_list_sync_(true) {
   if (!extension_system) {
     LOG(ERROR) << "AppListSyncableService created with no ExtensionSystem";
     return;
   }
 
   oem_folder_name_ =
       l10n_util::GetStringUTF8(IDS_APP_LIST_OEM_DEFAULT_FOLDER_NAME);
-
-  // Note: model_observer_ is constructed after the initial sync changes are
-  // received in MergeDataAndStartSyncing(). Changes to the model before that
-  // will be synced after the initial sync occurs.
-  if (extension_system->extension_service() &&

[tapted, 2015/03/10 19:19:54]

How do you know this logic isn't needed any more? (maybe `git annotate` to see
why it exists - I think it was something obscure)

I guess model() would have just returned an empty model in those cases. But
could something call GetModel() too soon and die? Or: build an empty model, but
then never receive any signal that the model is ready to be populated?

[stevenjb, 2015/03/10 19:56:06]

Since AppListSyncableService is a context keyed service, it might be initialized
before or after the extension service, so we needed to initialize the model
either here or once the extension service has been created and we are notified
(although that notification appears deprecated, I'm not familiar with the
situation there).

Nothing should actually access the model before the extension service is ready
however. We could add some DCHECKs to BuildModel() to make that clear.
(Otherwise we will crash in ExtensionAppModelBuilder::BuildModel).

[calamity, 2015/03/12 21:23:22]

DCHECKs added.

-      extension_system->extension_service()->is_ready()) {
-    BuildModel();
-    return;
-  }
-
-  // The extensions for this profile have not yet all been loaded.
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED,
-                 content::Source<Profile>(profile));
 }
 
 AppListSyncableService::~AppListSyncableService() {
   // Remove observers.
   model_observer_.reset();
   model_pref_updater_.reset();
 
   STLDeleteContainerPairSecondPointers(sync_items_.begin(), sync_items_.end());
 }
 
 void AppListSyncableService::BuildModel() {
   // For now, use the AppListControllerDelegate associated with the native
   // desktop. TODO(stevenjb): Remove ExtensionAppModelBuilder controller
   // dependency and move the dependent methods from AppListControllerDelegate
   // to an extension service delegate associated with this class.
   AppListControllerDelegate* controller = NULL;
   AppListService* service =
       AppListService::Get(chrome::HOST_DESKTOP_TYPE_NATIVE);
   if (service)
     controller = service->GetControllerDelegate();
   apps_builder_.reset(new ExtensionAppModelBuilder(controller));
   DCHECK(profile_);
   if (app_list::switches::IsAppListSyncEnabled()) {
     VLOG(1) << this << ": AppListSyncableService: InitializeWithService.";
     SyncStarted();
-    apps_builder_->InitializeWithService(this);
+    apps_builder_->InitializeWithService(this, model_.get());
   } else {
     VLOG(1) << this << ": AppListSyncableService: InitializeWithProfile.";
     apps_builder_->InitializeWithProfile(profile_, model_.get());
   }
 
   model_pref_updater_.reset(
       new ModelPrefUpdater(AppListPrefs::Get(profile_), model_.get()));
 
   if (app_list::switches::IsDriveAppsInAppListEnabled())
     drive_app_provider_.reset(new DriveAppProvider(profile_, this));
 }
 
+size_t AppListSyncableService::GetNumSyncItemsForTest() {
+  // If the model isn't built yet, there will be no sync items.
+  if (!apps_builder_)
+    BuildModel();

[stevenjb, 2015/03/10 19:56:06]

nit: GetModel();

[calamity, 2015/03/12 21:23:22]

Done.

+
+  return sync_items_.size();
+}
+
 void AppListSyncableService::ResetDriveAppProviderForTest() {
   drive_app_provider_.reset();
 }
 
 void AppListSyncableService::Shutdown() {
   // DriveAppProvider touches other KeyedServices in its dtor and needs be
   // released in shutdown stage.
   drive_app_provider_.reset();
 }
 
@@ skipping 17 matching lines @@
   const std::string sync_id = GetDriveAppSyncId(drive_app_id);
   SyncItem* sync_item = FindSyncItem(sync_id);
   if (!sync_item)
     return;
 
   DCHECK_EQ(sync_item->item_type,
             sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP);
   DeleteSyncItem(sync_item);
 }
 
-void AppListSyncableService::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED, type);
-  DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
-  registrar_.RemoveAll();
-  BuildModel();
-}
-
 const AppListSyncableService::SyncItem*
 AppListSyncableService::GetSyncItem(const std::string& id) const {
   SyncItemMap::const_iterator iter = sync_items_.find(id);
   if (iter != sync_items_.end())
     return iter->second;
   return NULL;
 }
 
 void AppListSyncableService::SetOemFolderName(const std::string& name) {
   oem_folder_name_ = name;
   AppListFolderItem* oem_folder = model_->FindFolderItem(kOemFolderId);
   if (oem_folder)
     model_->SetItemName(oem_folder, oem_folder_name_);
 }
 
+AppListModel* AppListSyncableService::GetModel() {
+  if (!apps_builder_)
+    BuildModel();
+
+  return model_.get();
+}
+
 void AppListSyncableService::AddItem(scoped_ptr<AppListItem> app_item) {
   SyncItem* sync_item = FindOrAddSyncItem(app_item.get());
   if (!sync_item)
     return;  // Item is not valid.
 
   std::string folder_id;
   if (app_list::switches::IsFolderUIEnabled()) {
     if (AppIsOem(app_item->id())) {
       folder_id = FindOrCreateOemFolder();
       VLOG_IF(2, !folder_id.empty())
@@ skipping 103 matching lines @@
   bool changed = UpdateSyncItemFromAppItem(app_item, sync_item);
   if (!changed) {
     DVLOG(2) << this << " - Update: SYNC NO CHANGE: " << sync_item->ToString();
     return;
   }
   SendSyncChange(sync_item, SyncChange::ACTION_UPDATE);
 }
 
 void AppListSyncableService::RemoveItem(const std::string& id) {
   RemoveSyncItem(id);
   model_->DeleteItem(id);

[tapted, 2015/03/10 19:19:54]

What about all these calls internally that access |model_|? Can sync start
(flare?) for something else without anything calling GetModel() and populating
the initial set?

[stevenjb, 2015/03/10 19:56:06]

No sync events will get called before MergeDataAndStartSyncing, and I don't
think that MergeDataAndStartSyncing() will get called until SyncStarted() gets
called from BuildModel(). However, I'm not 100% certain of that, so we may want
to check with the sync team and/or call GetModel() at the top of
MergeDataAndStartSyncing(). (Or maybe DCHECK(model_) if we're reasonably sure
MergeDataAndStartSyncing won't get called first).
 

[Nicolas Zea, 2015/03/12 18:09:00]

Sync can start up at any time, whether or not GetModel/SyncStarted is called
here. Keep in mind other datatypes have this same logic and can tell sync to
start. So MergeDataAndStartSyncing could happen at any time.

You're right that no sync events will happen until MergeDataAndStartSyncing is
called though.

[calamity, 2015/03/12 21:23:22]

Hmm. I don't know if I should ensure the model is built in
MergeDataAndStartSyncing. Chances are, it was always built before it in the
past. I don't know if there would be problems if it wasn't built when MDASS is
called. I'm not in Sydney now so it's a bit tricky to check.

[Nicolas Zea, 2015/03/12 21:28:05]

If you're concerned about a race here, Sync has the ability to hold off on
calling MergeDataAndStartSyncing until some condition is met via the datatype
controller logic. See for example the search_engine_data_type_controller.cc.
You'd need to create an app_list_data_type_controller to do this.

[tapted, 2015/03/20 11:47:14]

Perhaps just GetModel() with a comment at the start of
MergeDataAndStartSyncing()?

[calamity, 2015/03/23 05:14:49]

Done.

   PruneEmptySyncFolders();
 }
 
 void AppListSyncableService::UpdateItem(AppListItem* app_item) {
   // Check to see if the item needs to be moved to/from the OEM folder.
   if (!app_list::switches::IsFolderUIEnabled())
     return;
   bool is_oem = AppIsOem(app_item->id());
   if (!is_oem && app_item->folder_id() == kOemFolderId)
     model_->MoveItemToFolder(app_item, "");
@@ skipping 501 matching lines @@
   } else {
     res += " { " + item_name + " }";
     res += " [" + item_ordinal.ToDebugString() + "]";
     if (!parent_id.empty())
       res += " <" + parent_id.substr(0, 8) + ">";
   }
   return res;
 }
 
 }  // namespace app_list