Refactor notifications of chrome/browser/webdata

chrome/browser/sync/glue/autofill_profile_data_type_controller.cc

Index: chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
diff --git a/chrome/browser/sync/glue/autofill_profile_data_type_controller.cc b/chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
index cb5e8e7dab57da2bad3c3e974788707223c2840e..0e33e43bb8c1ef95955497ff12cc6af8448c0b6f 100644
--- a/chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
+++ b/chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
@@ -12,11 +12,8 @@
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
-#include "chrome/common/chrome_notification_types.h"
 #include "components/autofill/browser/personal_data_manager.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
 #include "sync/api/sync_error.h"
 #include "sync/api/syncable_service.h"
 
@@ -43,29 +40,33 @@ syncer::ModelSafeGroup
   return syncer::GROUP_DB;
 }
 
-void AutofillProfileDataTypeController::Observe(
-    int notification_type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  notification_registrar_.RemoveAll();
+void AutofillProfileDataTypeController::WebDatabaseLoaded() {
   OnModelLoaded();
+  if (web_data_service_)
+    web_data_service_->RemoveObserver(this);

[dhollowa, 2013/03/12 22:22:28]

Curious, why change the order here?  This used to be called prior to
OnModelLoaded().

[kaiwang, 2013/03/13 04:57:20]

Done.

 }
 
 void AutofillProfileDataTypeController::OnPersonalDataChanged() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(state(), MODEL_STARTING);
+
   personal_data_->RemoveObserver(this);
   web_data_service_ = WebDataServiceFactory::GetForProfile(
       profile(), Profile::IMPLICIT_ACCESS);
-  if (web_data_service_.get() && web_data_service_->IsDatabaseLoaded()) {
+
+  if (!web_data_service_)

[dhollowa, 2013/03/12 22:22:28]

This is probably fine, but there's a behavior change here.  Have you verified it
doesn't cause trouble.  That is, if web_data_service_ was NULL before, we'd
still call OnModelLoaded().

[kaiwang, 2013/03/13 04:57:20]

I think it's when web_data_service_ was NULL, we still register observer?

Without WebDataService, no notifications will be generated, and observing is
noop, so I think it's fine here.

+    return;
+
+  if (web_data_service_->IsDatabaseLoaded())
     OnModelLoaded();
-  } else {
-    notification_registrar_.Add(this, chrome::NOTIFICATION_WEB_DATABASE_LOADED,
-                                content::NotificationService::AllSources());
-  }
+  else
+    web_data_service_->AddObserver(this);
 }
 
-AutofillProfileDataTypeController::~AutofillProfileDataTypeController() {}
+AutofillProfileDataTypeController::~AutofillProfileDataTypeController() {
+  if (web_data_service_)
+    web_data_service_->AddObserver(this);

[dhollowa, 2013/03/12 22:22:28]

This can't be right.  RemoveObserver?

[kaiwang, 2013/03/13 04:57:20]

Oops, done

+}

[dhollowa, 2013/03/12 22:22:28]

To make this less error prone, how about using ScopedObserver<S, O>?

[kaiwang, 2013/03/13 04:57:20]

Done.

 
 bool AutofillProfileDataTypeController::PostTaskOnBackendThread(
     const tracked_objects::Location& from_here,
@@ -88,18 +89,24 @@ bool AutofillProfileDataTypeController::StartModels() {
 
   web_data_service_ = WebDataServiceFactory::GetForProfile(
       profile(), Profile::IMPLICIT_ACCESS);
-  if (web_data_service_.get() && web_data_service_->IsDatabaseLoaded())
+
+  if (!web_data_service_)

[dhollowa, 2013/03/12 22:22:28]

Again, a behavior change.  You sure?

+    return false;
+
+  if (web_data_service_->IsDatabaseLoaded())
     return true;
 
-  notification_registrar_.Add(this, chrome::NOTIFICATION_WEB_DATABASE_LOADED,
-                              content::NotificationService::AllSources());
+  web_data_service_->AddObserver(this);
   return false;
 }
 
 void AutofillProfileDataTypeController::StopModels() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(state() == STOPPING || state() == NOT_RUNNING);
-  notification_registrar_.RemoveAll();
+
+  if (web_data_service_)
+    web_data_service_->RemoveObserver(this);
+
   personal_data_->RemoveObserver(this);
 }