SuggestionsService: implement SyncServiceObserver

chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc

Index: chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
diff --git a/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc b/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
index 29969c11851c05665a3ef85872b51018c018a69b..75101a48bf60a33f660ecbde330a0dee7efca3d7 100644
--- a/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
+++ b/chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
@@ -50,6 +50,7 @@ class FakeProfileSyncService : public ProfileSyncService {
  public:
   explicit FakeProfileSyncService(Profile* profile)
       : ProfileSyncService(CreateProfileSyncServiceParamsForTest(profile)),
+        test_started_(false),
         sync_initialized_(true),
         initialized_state_violation_(false) {}
 
@@ -61,6 +62,10 @@ class FakeProfileSyncService : public ProfileSyncService {
         new FakeProfileSyncService(static_cast<Profile*>(context)));
   }
 
+  void set_test_started(bool test_started) {
+    test_started_ = test_started;
+  }
+
   void set_sync_initialized(bool sync_initialized) {
     sync_initialized_ = sync_initialized;
   }
@@ -71,6 +76,10 @@ class FakeProfileSyncService : public ProfileSyncService {
   bool IsSyncActive() const override { return sync_initialized_; }
 
   void AddObserver(sync_driver::SyncServiceObserver* observer) override {
+    // Ignore other SyncServiceObsevers which we don't care about.
+    if (!test_started_)
+      return;

[Marc Treib, 2016/03/08 15:37:59]

Before, this test was assuming that nobody else would add a SyncServiceObserver.
This CL introduces one that is created on browser startup, so we'd get here
twice, |initialized_state_violation_| would get set to true, and the test would
fail.

[Devlin, 2016/03/08 17:14:59]

This seems...wrong.  What exactly is this trying to test?  What happens when
there are more observers or clients that call some of these functions?  What if
we end up adding our observer earlier?  This just seems like the wrong pattern
to be testing.

[Marc Treib, 2016/03/08 17:33:22]

I'm reverse engineering, so take everything I say with a big grain of salt :)

There's two tests below for which this is relevant. One sets sync_initialized to
false, the other doesn't (note how it's initially set to true above). So this is
testing that AddObserver only gets called if IsSyncActive (above) says false.
Which, incidentally, is exactly what happens in
PreferencesPrivateGetSyncCategoriesWithoutPassphraseFunction: It registers
itself as a SyncServiceObserver iff IsSyncActive() is false.

I agree that this test setup is less than optimal, as evidenced by the fact that
it was broken by a totally unrelated change. Still, the new code is IMO slightly
less wrong than the old code.

[Devlin, 2016/03/08 20:06:23]

I still think that it's not testing anything useful, and, judging by the fact
that it's making you update the test and me wonder about it, it makes me think
we should just delete it.  I guess we can hold off on that for now and just go
with this, even though I think it's silly.  (You're right that it's no worse
than what was there.)

Relatedly, do we even need this API?  Maybe we can delete it all! :)

+
     if (sync_initialized_)
       initialized_state_violation_ = true;
     // Set sync initialized state to true so the function will run after
@@ -99,6 +108,7 @@ class FakeProfileSyncService : public ProfileSyncService {
   }
 
  private:
+  bool test_started_;
   bool sync_initialized_;
   // Set to true if a function is called when sync_initialized is in an
   // unexpected state.
@@ -141,6 +151,8 @@ class PreferencesPrivateApiTest : public ExtensionApiTest {
         profile, &FakeProfileSyncService::BuildFakeProfileSyncService));
 
     browser_ = new Browser(Browser::CreateParams(profile));
+
+    service_->set_test_started(true);
   }
 
   // Calls GetSyncCategoriesWithoutPassphraseFunction and verifies that the