Add onChanged events to the extension settings API, both from sync and between

chrome/browser/extensions/extension_settings_frontend.cc

Index: chrome/browser/extensions/extension_settings_frontend.cc
diff --git a/chrome/browser/extensions/extension_settings_frontend.cc b/chrome/browser/extensions/extension_settings_frontend.cc
index 4963e927ae92c67c77fb084f76387cce05297626..34af3d3a3ceaa76e966654d710e7e7b156eab6da 100644
--- a/chrome/browser/extensions/extension_settings_frontend.cc
+++ b/chrome/browser/extensions/extension_settings_frontend.cc
@@ -6,20 +6,115 @@
 
 #include "base/bind.h"
 #include "base/file_path.h"
+#include "chrome/browser/extensions/extension_event_names.h"
+#include "chrome/browser/extensions/extension_event_router.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_settings_backend.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/browser/browser_thread.h"
 
-ExtensionSettingsFrontend::ExtensionSettingsFrontend(
-    const FilePath& base_path)
-    : core_(new ExtensionSettingsFrontend::Core()) {
+// Observer which sends events to a target profile iff the profile isn't the
+// originating profile for the event.
+class ExtensionSettingsFrontend::DefaultObserver
+    : public ExtensionSettingsObserver {
+ public:
+  explicit DefaultObserver(Profile* profile) : target_profile_(profile) {}
+  virtual ~DefaultObserver() {}
+
+  virtual void OnSettingsChanged(
+      const Profile* origin_profile,
+      const std::string& extension_id,
+      const ExtensionSettingChanges& changes) OVERRIDE {

[akalin, 2011/10/19 19:09:49]

maybe DCHECK that origin_profile->OriginalProfile() == target_profile_ or vice
versa

[not at google - send to devlin, 2011/10/19 22:59:36]

I don't think this would be right though?  target_profile_ could be either the
original or incognito profile.

[akalin, 2011/10/19 23:17:00]

Right, that's why I put 'vice versa', so something like:

if (origin_profile != target_profile_) {
  DCHECK_NE(origin_profile.IsOffTheRecord(), target_profile.IsOffTheRecord());
  if (target_profile_.IsOffTheRecord()) {
    DCHECK_EQ(target_profile->GetOriginalProfile(), origin_profile);
  } else {
    DCHECK_EQ(origin_profile->GetOriginalProfile(), target_profile);
  }
}

[not at google - send to devlin, 2011/10/19 23:47:34]

Don't know.  Seems to be adding noise to a very simple class -- enough dchecking
is done in the methods which actually create the DefaultObservers.

Incidentally I've cleaned up that logic a bit to be clearer so that my previous
statement holds.

+    if (origin_profile != target_profile_) {
+      target_profile_->GetExtensionEventRouter()->DispatchEventToExtension(
+        extension_id,
+        extension_event_names::kOnSettingsChanged,
+        // This is the list of function arguments to pass to the onChanged
+        // handler of extensions, a single argument with the list of changes.
+        std::string("[") + changes.ToJson() + "]",
+        target_profile_,
+        GURL());
+    }
+  }
+
+ private:
+  Profile* target_profile_;
+};
+
+// Ref-counted container for the ExtensionSettingsBackend object.
+class ExtensionSettingsFrontend::Core
+    : public base::RefCountedThreadSafe<Core> {
+ public:
+  explicit Core(
+      const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >&
+          observers)
+      : observers_(observers), backend_(NULL) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  }
+
+  // Does any FILE thread specific initialization, such as construction of
+  // |backend_|.  Must be called before any call to
+  // RunWithBackendOnFileThread().
+  void InitOnFileThread(const FilePath& base_path) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    DCHECK(!backend_);
+    backend_ = new ExtensionSettingsBackend(base_path, observers_);
+  }
+
+  // Runs |callback| with the extension backend.
+  void RunWithBackendOnFileThread(const BackendCallback& callback) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    DCHECK(backend_);
+    callback.Run(backend_);
+  }
+
+ private:
+  virtual ~Core() {
+    if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
+      delete backend_;
+    } else if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+      BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, backend_);
+    } else {
+      NOTREACHED();
+    }
+  }
+
+  friend class base::RefCountedThreadSafe<Core>;
+
+  // Observers to settings changes (thread safe).
+  scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >
+      observers_;
+
+  // Lives on the FILE thread.
+  ExtensionSettingsBackend* backend_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+ExtensionSettingsFrontend::ExtensionSettingsFrontend(Profile* profile)
+    : profile_(profile),
+      observers_(new ObserverListThreadSafe<ExtensionSettingsObserver>()),
+      core_(new ExtensionSettingsFrontend::Core(observers_.get())) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+

[akalin, 2011/10/19 19:09:49]

possible to DCHECK that profile_ isn't an incognito profile?  That should hold,
right?

[not at google - send to devlin, 2011/10/19 22:59:36]

Done.

+  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,

[akalin, 2011/10/19 19:09:49]

comment here about how even though you're listening for any profile to be
created/destroyed, you're really only interested in the incognito profiles for
profile_

[not at google - send to devlin, 2011/10/19 22:59:36]

Done.

+      NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
+      NotificationService::AllBrowserContextsAndSources());
+  OnProfileCreated(profile);
+
   BrowserThread::PostTask(
       BrowserThread::FILE,
       FROM_HERE,
       base::Bind(
           &ExtensionSettingsFrontend::Core::InitOnFileThread,
           core_.get(),
-          base_path));
+          profile->GetPath().AppendASCII(
+              ExtensionService::kSettingsDirectoryName)));
+}
+
+ExtensionSettingsFrontend::~ExtensionSettingsFrontend() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 void ExtensionSettingsFrontend::RunWithBackend(
@@ -34,34 +129,53 @@ void ExtensionSettingsFrontend::RunWithBackend(
           callback));
 }
 
-ExtensionSettingsFrontend::~ExtensionSettingsFrontend() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+void ExtensionSettingsFrontend::AddObserver(
+    ExtensionSettingsObserver* observer) {
+  observers_->AddObserver(observer);
 }
 
-ExtensionSettingsFrontend::Core::Core() : backend_(NULL) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+void ExtensionSettingsFrontend::RemoveObserver(
+    ExtensionSettingsObserver* observer) {
+  observers_->RemoveObserver(observer);
 }
 
-void ExtensionSettingsFrontend::Core::InitOnFileThread(
-    const FilePath& base_path) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(!backend_);
-  backend_ = new ExtensionSettingsBackend(base_path);
+void ExtensionSettingsFrontend::Observe(
+      int type,
+      const NotificationSource& source,
+      const NotificationDetails& details) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  switch (type) {
+    case chrome::NOTIFICATION_PROFILE_CREATED:
+      OnProfileCreated(Source<Profile>(source).ptr());
+      break;
+    case chrome::NOTIFICATION_PROFILE_DESTROYED:
+      OnProfileDestroyed(Source<Profile>(source).ptr());
+      break;
+    default:
+      NOTREACHED();
+  }
 }
 
-void ExtensionSettingsFrontend::Core::RunWithBackendOnFileThread(
-    const BackendCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(backend_);
-  callback.Run(backend_);
+void ExtensionSettingsFrontend::OnProfileCreated(Profile* new_profile) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (new_profile == profile_ && !profile_observer_.get()) {
+    profile_observer_.reset(new DefaultObserver(profile_));
+    AddObserver(profile_observer_.get());
+  } else if (new_profile == profile_->GetOffTheRecordProfile() &&

[akalin, 2011/10/19 19:09:49]

hmm GetOffTheRecordProfile() may create one if one doesn't exist, but I don't
think we want to do that.  What about "new_profile->GetOriginalProfile() ==
profile_"?

[not at google - send to devlin, 2011/10/19 22:59:36]

Yes you're right.  This would work.

+      !incognito_profile_observer_.get()) {
+    incognito_profile_observer_.reset(new DefaultObserver(new_profile));
+    AddObserver(incognito_profile_observer_.get());
+  }
 }
 
-ExtensionSettingsFrontend::Core::~Core() {
-  if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
-    delete backend_;
-  } else if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, backend_);
-  } else {
-    NOTREACHED();
+void ExtensionSettingsFrontend::OnProfileDestroyed(Profile* old_profile) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (old_profile == profile_ && profile_observer_.get()) {
+    RemoveObserver(profile_observer_.get());
+    profile_observer_.reset();
+  } else if (old_profile == profile_->GetOffTheRecordProfile() &&

[akalin, 2011/10/19 19:09:49]

same here

[not at google - send to devlin, 2011/10/19 22:59:36]

Done.

+      incognito_profile_observer_.get()) {
+    RemoveObserver(incognito_profile_observer_.get());
+    incognito_profile_observer_.reset();
   }
 }