Eliminate NOTIFICATION_HISTORY_LOADED notification

chrome/browser/sync/glue/bookmark_data_type_controller.cc

Index: chrome/browser/sync/glue/bookmark_data_type_controller.cc
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.cc b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
index 9e9da929d0b0829a95c485f6dff5812b09d52234..aec8191fffe71e8a667af61880e9fa54b188aeb9 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.cc
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
@@ -40,24 +40,6 @@ syncer::ModelType BookmarkDataTypeController::type() const {
   return syncer::BOOKMARKS;
 }
 
-void BookmarkDataTypeController::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK_EQ(state_, MODEL_STARTING);
-  DCHECK_EQ(chrome::NOTIFICATION_HISTORY_LOADED, type);
-
-  if (!DependentsLoaded())
-    return;
-
-  bookmark_model_->RemoveObserver(this);
-  installed_bookmark_observer_ = false;
-
-  registrar_.RemoveAll();
-  OnModelLoaded();
-}
-
 BookmarkDataTypeController::~BookmarkDataTypeController() {
   if (installed_bookmark_observer_ && bookmark_model_) {
     DCHECK(profile_);
@@ -70,9 +52,9 @@ bool BookmarkDataTypeController::StartModels() {
   if (!DependentsLoaded()) {
     bookmark_model_->AddObserver(this);
     installed_bookmark_observer_ = true;
-
-    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
-                   content::Source<Profile>(sync_service_->profile()));
+    HistoryService* history = HistoryServiceFactory::GetForProfile(
+        profile_, Profile::EXPLICIT_ACCESS);
+    history->AddHistoryServiceObserver(this);

[sdefresne, 2014/09/23 08:45:09]

You never call "history->RemoveHistoryServiceObserver(this);". I think you
should do it a the location you removed the registrar_.RemoveAll().

[sdefresne, 2014/10/20 13:15:42]

Use ScopedObserver<>.

[nshaik, 2014/10/29 08:43:39]

I used ScopedObserver and I m running into an issue where the HistoryService is
already destroyed and then we are trying to remove the object
from ScopedObserver. For now I will keep this as is if it is not an issue.

[nshaik, 2014/10/29 08:43:39]

Done.

     return false;
   }
   return true;
@@ -80,7 +62,6 @@ bool BookmarkDataTypeController::StartModels() {
 
 // Cleanup for our extra registrar usage.
 void BookmarkDataTypeController::CleanUpState() {
-  registrar_.RemoveAll();
   if (bookmark_model_ && installed_bookmark_observer_) {
     bookmark_model_->RemoveObserver(this);
     installed_bookmark_observer_ = false;
@@ -107,7 +88,6 @@ void BookmarkDataTypeController::BookmarkModelLoaded(BookmarkModel* model,
   if (!DependentsLoaded())
     return;
 
-  registrar_.RemoveAll();
   OnModelLoaded();
 }
 
@@ -131,4 +111,14 @@ bool BookmarkDataTypeController::DependentsLoaded() {
   return true;
 }
 
+void BookmarkDataTypeController::HistoryServiceLoaded(HistoryService* service) {
+  if (!DependentsLoaded())

[sdefresne, 2014/09/23 08:45:09]

You removed the following assertion, maybe you should put it back.
DCHECK_EQ(state_, MODEL_STARTING);

[nshaik, 2014/10/29 08:43:39]

Done.

+    return;
+
+  bookmark_model_->RemoveObserver(this);
+  installed_bookmark_observer_ = false;
+
+  OnModelLoaded();
+}
+
 }  // namespace browser_sync