Refactor notifications of chrome/browser/webdata

Refactor notifications from webdata

Use a simplified, typed system


BUG=181277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191314

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

+joi

Linux bots complain of compile errror.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/api/webdata...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/api/webdata...
chrome/browser/api/webdata/web_data_service_base.h:8: #include
"content/public/browser/notification_source.h"
Is this still needed?

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:46:
web_data_service_->RemoveObserver(this);
Curious, why change the order here?  This used to be called prior to
OnModelLoaded().

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:57: if
(!web_data_service_)
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().

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:68:
web_data_service_->AddObserver(this);
This can't be right.  RemoveObserver?

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:69: }
To make this less error prone, how about using ScopedObserver<S, O>?

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:93: if
(!web_data_service_)
Again, a behavior change.  You sure?

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
chrome/browser/webdata/autocomplete_syncable_service.h:64: virtual void
AutofillEntriesChanged(const AutofillChangeList& changes)
nit: wrap at the arg, not at the OVERRIDE.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_profile_syncable_service.h:60: virtual void
AutofillProfileChanged(const AutofillProfileChange& change)
nit: wrap at arg, not at OVERRIDE.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_web_data_service_impl.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_web_data_service_impl.h:45: //
AutofillWebDataService implementation.
Please confirm with joi@ that this conforms to his plans for componentization of
this stuff.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:325: void
NotifyDatabaseLoadedOnUIThread();
It looks like you made this protected for use by tests.  Please make that test
class a friend (here) instead, and keep this method private.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service_observer.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: No need for "(c)"

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:12: // Called whenever
autofill entries are changed.
nit: capitalize "Autofill" when talking about the feature (noun), lower case
"autofill" when using it as a verb.

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
components/autofill/browser/personal_data_manager.cc:134:
autofill_data->AddObserver(this);
Please use ScopedObserver.

[Jói, 2013-03-12 22:28:18]

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_web_data_service_impl.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_web_data_service_impl.h:45: //
AutofillWebDataService implementation.
On 2013/03/12 22:22:28, dhollowa wrote:
> Please confirm with joi@ that this conforms to his plans for componentization
of
> this stuff.

Yeah, this is OK, although once we get further along the class hierarchy will
collapse a bit and these won't be virtual. Once we finish componentizing, there
will just be one AutofillWebDataService class I think, which will inherit from a
base class for all XyzWebDataService classes, probably named WebDataServiceBase
or just WebDataService.

[Jói, 2013-03-12 23:27:34]

I took a look at the rest of the change, couple of comments below.

Cheers,
Jói

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_data_type_controller.cc:52:
web_data_service_->RemoveObserver(this);
double-checking: this is idempotent, OK to call more than once?

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service_observer.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:10: class
WebDataServiceObserver {
Kai, can you add a TODO here that we'll need to split this into an
Autofill-specific part and the rest, once we split the Autofill parts of
WebDataService off into a separate concrete class?

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

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/api/webdata...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/api/webdata...
chrome/browser/api/webdata/web_data_service_base.h:8: #include
"content/public/browser/notification_source.h"
On 2013/03/12 22:22:28, dhollowa wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_data_type_controller.cc:52:
web_data_service_->RemoveObserver(this);
Yes, ObserverList supports this:
https://cs.corp.google.com/#chrome/src/base/observer_list.h&sq=package:%5Echr...

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:46:
web_data_service_->RemoveObserver(this);
On 2013/03/12 22:22:28, dhollowa wrote:
> Curious, why change the order here?  This used to be called prior to
> OnModelLoaded().

Done.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:57: if
(!web_data_service_)
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.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:68:
web_data_service_->AddObserver(this);
Oops, done

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:69: }
On 2013/03/12 22:22:28, dhollowa wrote:
> To make this less error prone, how about using ScopedObserver<S, O>?

Done.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
chrome/browser/webdata/autocomplete_syncable_service.h:64: virtual void
AutofillEntriesChanged(const AutofillChangeList& changes)
On 2013/03/12 22:22:28, dhollowa wrote:
> nit: wrap at the arg, not at the OVERRIDE.

Done.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_profile_syncable_service.h:60: virtual void
AutofillProfileChanged(const AutofillProfileChange& change)
On 2013/03/12 22:22:28, dhollowa wrote:
> nit: wrap at arg, not at OVERRIDE.

Done.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:325: void
NotifyDatabaseLoadedOnUIThread();
But it's FakeWebDataService in an anonymous namespace..
Since it's a subclass of WebDataService, I think make this protected is more
nature

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service_observer.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Are you sure? Most of the code have this...

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:10: class
WebDataServiceObserver {
On 2013/03/12 23:27:34, Jói wrote:
> Kai, can you add a TODO here that we'll need to split this into an
> Autofill-specific part and the rest, once we split the Autofill parts of
> WebDataService off into a separate concrete class?

Done.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:12: // Called whenever
autofill entries are changed.
On 2013/03/12 22:22:28, dhollowa wrote:
> nit: capitalize "Autofill" when talking about the feature (noun), lower case
> "autofill" when using it as a verb.

Done.

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
components/autofill/browser/personal_data_manager.cc:134:
autofill_data->AddObserver(this);
Can not do it here.. AutofillWebDataService is a wrapper on WebDataService, so I
can not get the true Source here

[dhollowa, 2013-03-13 16:15:28]

ProfileSyncServiceAutofillTest.HasProfileEmptySync is failing.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:325: void
NotifyDatabaseLoadedOnUIThread();
On 2013/03/13 04:57:20, kaiwang wrote:
> But it's FakeWebDataService in an anonymous namespace..
> Since it's a subclass of WebDataService, I think make this protected is more
> nature

Ok.

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service_observer.h (right):

https://codereview.chromium.org/12476031/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service_observer.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/03/13 04:57:20, kaiwang wrote:
> Are you sure? Most of the code have this...

http://crbug.com/140977

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
components/autofill/browser/personal_data_manager.cc:134:
autofill_data->AddObserver(this);
On 2013/03/13 04:57:20, kaiwang wrote:
> Can not do it here.. AutofillWebDataService is a wrapper on WebDataService, so
I
> can not get the true Source here

This should work:
  scoped_observer_.Add(autofill_data.get());

[dhollowa, 2013-03-13 16:17:09]

https://codereview.chromium.org/12476031/diff/42001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/12476031/diff/42001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:922:
TEST_F(ProfileSyncServiceAutofillTest, HasProfileEmptySync) {
This is failing.

[kaiwang, 2013-03-13 17:27:40]

A few testing is failing, since the PersonalDataManager now depends on
ProfileKeyedService. Looking into them...

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
components/autofill/browser/personal_data_manager.cc:134:
autofill_data->AddObserver(this);
AutofillWebDataService is a thin wrapper. Every time a new instance will be
generated and the true notification source is underneath :(
There's a current refactoring on this. ScopedObserver should be able to work
later

[kaiwang, 2013-03-13 22:17:18]

Please take another look, thanks!

[dhollowa, 2013-03-14 00:35:17]

LGTM.  I focused on webdata and autofill.  Please seek review by sync owner.

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/12476031/diff/7001/components/autofill/browse...
components/autofill/browser/personal_data_manager.cc:134:
autofill_data->AddObserver(this);
On 2013/03/13 17:27:40, kaiwang wrote:
> AutofillWebDataService is a thin wrapper. Every time a new instance will be
> generated and the true notification source is underneath :(
> There's a current refactoring on this. ScopedObserver should be able to work
> later

Got it.  Ok.

[kaiwang, 2013-03-14 01:02:38]

OWNERS, please review

akalin: c/b/browser/sync
joi: c/b/browser/api
jam: chrome/ chrome/common and chrome/test

Thanks a lot!

[Jói, 2013-03-14 01:11:25]

LGTM for chrome/browser/api.

[jam, 2013-03-14 16:09:44]

On 2013/03/14 01:02:38, kaiwang wrote:
> OWNERS, please review
> 
> akalin: c/b/browser/sync
> joi: c/b/browser/api
> jam: chrome/ chrome/common and chrome/test
lgtm (for chrome/ I assume you meant the gypi files only, that's all I looked
at)
> 
> Thanks a lot!

[Nicolas Zea, 2013-03-14 22:03:30]

sync/ LGTM with one nit

https://codereview.chromium.org/12476031/diff/79001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

https://codereview.chromium.org/12476031/diff/79001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller.cc:45: if
(web_data_service_)
move above the OnModelLoaded() call

[kaiwang, 2013-03-14 22:11:40]

https://codereview.chromium.org/12476031/diff/79001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

https://codereview.chromium.org/12476031/diff/79001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller.cc:45: if
(web_data_service_)
On 2013/03/14 22:03:30, Nicolas Zea wrote:
> move above the OnModelLoaded() call

Done.

[commit-bot: I haz the power, 2013-03-15 21:33:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12476031/100002

[commit-bot: I haz the power, 2013-03-16 04:40:55]

Retried try job too often on win7_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[dhollowa, 2013-03-27 20:55:08]

SLGTM, but please review DEPS warnings at: 

  https://chromium-status.appspot.com/cq/kaiwang%40chromium.org/12476031/100002

Problem?

[commit-bot: I haz the power, 2013-03-27 21:47:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12476031/145001

[commit-bot: I haz the power, 2013-03-27 21:58:10]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-27 23:21:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12476031/145001

[commit-bot: I haz the power, 2013-03-27 23:50:30]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-28 21:40:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12476031/171001

[commit-bot: I haz the power, 2013-03-28 23:45:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12476031/199001

[commit-bot: I haz the power, 2013-03-28 23:45:32]

Failed to apply patch for chrome/browser/webdata/web_data_service_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/webdata/web_data_service_unittest.cc
  Hunk #1 FAILED at 21.
  Hunk #2 succeeded at 45 (offset 1 line).
  Hunk #3 succeeded at 77 (offset 1 line).
  Hunk #4 succeeded at 128 (offset 1 line).
  Hunk #5 succeeded at 167 (offset 1 line).
  Hunk #6 succeeded at 206 (offset 1 line).
  Hunk #7 succeeded at 234 (offset 1 line).
  Hunk #8 succeeded at 248 (offset 1 line).
  Hunk #9 succeeded at 261 (offset 1 line).
  Hunk #10 succeeded at 278 (offset 1 line).
  Hunk #11 succeeded at 293 (offset 1 line).
  Hunk #12 succeeded at 313 (offset 1 line).
  Hunk #13 succeeded at 330 (offset 1 line).
  Hunk #14 succeeded at 351 (offset 1 line).
  Hunk #15 succeeded at 374 (offset 1 line).
  Hunk #16 succeeded at 476 (offset 1 line).
  Hunk #17 succeeded at 509 (offset 1 line).
  1 out of 17 hunks FAILED -- saving rejects to file
chrome/browser/webdata/web_data_service_unittest.cc.rej

Patch:       chrome/browser/webdata/web_data_service_unittest.cc
Index: chrome/browser/webdata/web_data_service_unittest.cc
diff --git a/chrome/browser/webdata/web_data_service_unittest.cc
b/chrome/browser/webdata/web_data_service_unittest.cc
index
21204e3ea2a49b909d01986e96479630b3e19970..941ad423744b3ac3b0895044fa60a5bd49e2da9b
100644
--- a/chrome/browser/webdata/web_data_service_unittest.cc
+++ b/chrome/browser/webdata/web_data_service_unittest.cc
@@ -21,15 +21,12 @@
 #include "chrome/browser/webdata/autofill_entry.h"
 #include "chrome/browser/webdata/autofill_table.h"
 #include "chrome/browser/webdata/autofill_web_data_service.h"
+#include "chrome/browser/webdata/autofill_web_data_service_observer.h"
 #include "chrome/browser/webdata/web_data_service_test_util.h"
 #include "chrome/browser/webdata/web_database_service.h"
-#include "chrome/common/chrome_notification_types.h"
-#include "chrome/test/base/thread_observer_helper.h"
 #include "components/autofill/browser/autofill_profile.h"
 #include "components/autofill/browser/credit_card.h"
 #include "components/autofill/common/form_field_data.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -44,26 +41,19 @@ using testing::ElementsAreArray;
 using testing::Pointee;
 using testing::Property;
 
-typedef std::vector<AutofillChange> AutofillChangeList;
-
 static const int kWebDataServiceTimeoutSeconds = 8;
 
 ACTION_P(SignalEvent, event) {
   event->Signal();
 }
 
-class AutofillDBThreadObserverHelper : public DBThreadObserverHelper {
- protected:
-  virtual ~AutofillDBThreadObserverHelper() {}
-
-  virtual void RegisterObservers() OVERRIDE {
-    registrar_.Add(&observer_,
-                   chrome::NOTIFICATION_AUTOFILL_ENTRIES_CHANGED,
-                   content::NotificationService::AllSources());
-    registrar_.Add(&observer_,
-                   chrome::NOTIFICATION_AUTOFILL_PROFILE_CHANGED,
-                   content::NotificationService::AllSources());
-  }
+class MockAutofillWebDataServiceObserver
+    : public AutofillWebDataServiceObserverOnDBThread {
+ public:
+  MOCK_METHOD1(AutofillEntriesChanged,
+               void(const AutofillChangeList& changes));
+  MOCK_METHOD1(AutofillProfileChanged,
+               void(const AutofillProfileChange& change));
 };
 
 class WebDataServiceTest : public testing::Test {
@@ -83,8 +73,8 @@ class WebDataServiceTest : public testing::Test {
     wdbs_->AddTable(scoped_ptr<WebDatabaseTable>(new AutofillTable()));
     wdbs_->LoadDatabase(WebDatabaseService::InitCallback());
 
-    wds_ = new AutofillWebDataService(wdbs_,
-        WebDataServiceBase::ProfileErrorCallback());
+    wds_ = new AutofillWebDataService(
+        wdbs_, WebDataServiceBase::ProfileErrorCallback());
     wds_->Init();
   }
 
@@ -134,13 +124,27 @@ class WebDataServiceAutofillTest : public
WebDataServiceTest {
     name2_ = ASCIIToUTF16("name2");
     value1_ = ASCIIToUTF16("value1");
     value2_ = ASCIIToUTF16("value2");
-    observer_helper_ = new AutofillDBThreadObserverHelper();
-    observer_helper_->Init();
+
+    void (AutofillWebDataService::*add_observer_func)(
+        AutofillWebDataServiceObserverOnDBThread*) =
+        &AutofillWebDataService::AddObserver;
+    BrowserThread::PostTask(
+        BrowserThread::DB,
+        FROM_HERE,
+        base::Bind(add_observer_func, wds_, &observer_));
+    WaitForDatabaseThread();
   }
 
   virtual void TearDown() {
-    // Release this first so it can get destructed on the db thread.
-    observer_helper_ = NULL;
+    void (AutofillWebDataService::*remove_observer_func)(
+        AutofillWebDataServiceObserverOnDBThread*) =
+        &AutofillWebDataService::RemoveObserver;
+    BrowserThread::PostTask(
+        BrowserThread::DB,
+        FROM_HERE,
+        base::Bind(remove_observer_func, wds_, &observer_));
+    WaitForDatabaseThread();
+
     WebDataServiceTest::TearDown();
   }
 
@@ -159,7 +163,7 @@ class WebDataServiceAutofillTest : public WebDataServiceTest
{
   string16 value2_;
   int unique_id1_, unique_id2_;
   const TimeDelta test_timeout_;
-  scoped_refptr<AutofillDBThreadObserverHelper> observer_helper_;
+  testing::NiceMock<MockAutofillWebDataServiceObserver> observer_;
   WaitableEvent done_event_;
 };
 
@@ -198,13 +202,9 @@ TEST_F(WebDataServiceAutofillTest, FormFillAdd) {
 
   // This will verify that the correct notification is triggered,
   // passing the correct list of autofill keys in the details.
-  EXPECT_CALL(
-      *observer_helper_->observer(),
-      Observe(int(chrome::NOTIFICATION_AUTOFILL_ENTRIES_CHANGED),
-              content::Source<AutofillWebDataService>(wds_.get()),
-              Property(&content::Details<const AutofillChangeList>::ptr,
-                       Pointee(ElementsAreArray(expected_changes))))).
-      WillOnce(SignalEvent(&done_event_));
+  EXPECT_CALL(observer_,
+              AutofillEntriesChanged(ElementsAreArray(expected_changes)))
+      .WillOnce(SignalEvent(&done_event_));
 
   std::vector<FormFieldData> form_fields;
   AppendFormField(name1_, value1_, &form_fields);
@@ -230,8 +230,8 @@ TEST_F(WebDataServiceAutofillTest, FormFillAdd) {
 
 TEST_F(WebDataServiceAutofillTest, FormFillRemoveOne) {
   // First add some values to autofill.
-  EXPECT_CALL(*observer_helper_->observer(), Observe(_, _, _)).
-      WillOnce(SignalEvent(&done_event_));
+  EXPECT_CALL(observer_, AutofillEntriesChanged(_))
+      .WillOnce(SignalEvent(&done_event_));
   std::vector<FormFieldData> form_fields;
   AppendFormField(name1_, value1_, &form_fields);
   wds_->AddFormFields(form_fields);
@@ -244,13 +244,9 @@ TEST_F(WebDataServiceAutofillTest, FormFillRemoveOne) {
   const AutofillChange expected_changes[] = {
     AutofillChange(AutofillChange::REMOVE, AutofillKey(name1_, value1_))
   };
-  EXPECT_CALL(
-      *observer_helper_->observer(),
-      Observe(int(chrome::NOTIFICATION_AUTOFILL_ENTRIES_CHANGED),
-              content::Source<AutofillWebDataService>(wds_.get()),
-              Property(&content::Details<const AutofillChangeList>::ptr,
-                       Pointee(ElementsAreArray(expected_changes))))).
-      WillOnce(SignalEvent(&done_event_));
+  EXPECT_CALL(observer_,
+              AutofillEntriesChanged(ElementsAreArray(expected_changes)))
+      .WillOnce(SignalEvent(&done_event_));
   wds_->RemoveFormValueForElementName(name1_, value1_);
 
   // The event will be signaled when the mock observer is notified.
@@ -261,8 +257,9 @@ TEST_F(WebDataServiceAutofillTest, FormFillRemoveMany) {
   TimeDelta one_day(TimeDelta::FromDays(1));
   Time t = Time::Now();
 
-  EXPECT_CALL(*observer_helper_->observer(), Observe(_, _, _)).
-      WillOnce(SignalEvent(&done_event_));
+  EXPECT_CALL(observer_, AutofillEntriesChanged(_))
+      .WillOnce(SignalEvent(&done_event_));
+
   std::vector<FormFieldData> form_fields;
   AppendFormField(name1_, value1_, &form_fields);
   AppendFormField(name2_, value2_, &form_fields);
@@ -277,13 +274,9 @@ TEST_F(WebDataServiceAutofillTest, FormFillRemoveMany) {
     AutofillChange(AutofillChange::REMOVE, AutofillKey(name1_, value1_)),
     AutofillChange(AutofillChange::REMOVE, AutofillKey(name2_, value2_))
   };
-  EXPECT_CALL(
-      *observer_helper_->observer(),
-      Observe(int(chrome::NOTIFICATION_AUTOFILL_ENTRIES_CHANGED),
-              content::Source<AutofillWebDataService>(wds_.get()),
-              Property(&content::Details<const AutofillChangeList>::ptr,
-                       Pointee(ElementsAreArray(expected_changes))))).
-      WillOnce(SignalEvent(&done_event_));
+  EXPECT_CALL(observer_,
+              AutofillEntriesChanged(ElementsAreArray(expected_changes)))
+      .WillOnce(SignalEvent(&done_event_));
   wds_->RemoveFormElementsAddedBetween(t, t + one_day);
 
   // The event will be signaled when the mock observer is notified.
@@ -296,13 +289,8 @@ TEST_F(WebDataServiceAutofillTest, ProfileAdd) {
   // Check that GUID-based notification was sent.
   const AutofillProfileChange expected_change(
       AutofillProfileChange::ADD, profile.guid(), &profile);
-  EXPECT_CALL(
-      *observer_helper_->observer(),
-      Observe(int(chrome::NOTIFICATION_AUTOFILL_PROFILE_CHANGED),
-              content::Source<AutofillWebDataService>(wds_.get()),
-              Property(&content::Details<const AutofillProfileChange>::ptr,
-                       Pointee(expected_change)))).
-      WillOnce(SignalEvent(&done_event_));
+  EXPECT_CALL(observer_, AutofillProfileChanged(expected_change))
+      .WillOnce(SignalEvent(&done_event_));
 
   wds_->AddAutofillProfile(profile);
   done_event_.TimedWait(test_timeout_);
@@ -321,9 +309,8 @@ TEST_F(WebDataServiceAutofillTest, ProfileRemove) {
   AutofillProfile profile;
 
   // …
(message too large)

[commit-bot: I haz the power, 2013-03-29 00:51:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12476031/203001

[commit-bot: I haz the power, 2013-03-29 06:08:09]

Change committed as 191314