Persist declarative rules to the extension state store.

chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc

Index: chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
diff --git a/chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc b/chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3e61dc3463cf8022d9f5794944ebf3eb8af17bf4
--- /dev/null
+++ b/chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
@@ -0,0 +1,233 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.h"
+
+#include "base/bind.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/extensions/state_store.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/extension.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_source.h"
+
+namespace extensions {
+
+namespace {
+
+scoped_ptr<base::Value> RulesToValue(
+    const std::vector<linked_ptr<RulesRegistry::Rule> >& rules) {
+  scoped_ptr<base::ListValue> list(new base::ListValue());
+  for (size_t i = 0; i < rules.size(); ++i)
+    list->Append(rules[i]->ToValue().release());
+  return scoped_ptr<base::Value>(list.release());

[battre, 2012/06/15 22:56:49]

nit: how about
return list.PassAs<base::Value>();
?

[Matt Perry, 2012/06/18 21:01:01]

Cool, thanks!

+}
+
+std::vector<linked_ptr<RulesRegistry::Rule> > RulesFromValue(
+    base::Value* value) {
+  std::vector<linked_ptr<RulesRegistry::Rule> > rules;
+
+  base::ListValue* list = NULL;
+  if (!value || !value->GetAsList(&list))
+    return rules;
+
+  for (size_t i = 0; i < list->GetSize(); ++i) {
+    base::DictionaryValue* dict = NULL;
+    if (!list->GetDictionary(i, &dict))
+      continue;
+    linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule());
+    if (RulesRegistry::Rule::Populate(*dict, rule.get()))
+      rules.push_back(rule);
+  }
+
+  return rules;
+}
+
+}  // namespace
+
+// This class coordinates information between the UI and RulesRegistry threads.
+// It may outlive the RulesRegistry, which owns the delegate. Methods/variables
+// should be used on the UI thread unless otherwise noted.
+class RulesRegistryStorageDelegate::Inner
+    : public content::NotificationObserver,
+      public base::RefCountedThreadSafe<
+          Inner, content::BrowserThread::DeleteOnUIThread> {
+ public:
+  Inner(Profile* profile,
+        RulesRegistryWithCache* rules_registry,
+        const std::string& storage_key);
+
+ private:
+  friend class RulesRegistryStorageDelegate;
+  friend struct content::BrowserThread::DeleteOnThread<
+      content::BrowserThread::UI>;
+  friend class base::DeleteHelper<Inner>;
+
+  ~Inner();
+
+  // NotificationObserver
+  virtual void Observe(
+      int type,
+      const content::NotificationSource& source,
+      const content::NotificationDetails& details) OVERRIDE;
+
+  // Read/write a list of rules serialized to Values.
+  void ReadFromStorage(const std::string& extension_id,
+                       scoped_ptr<base::Value> value);
+  void WriteToStorage(const std::string& extension_id,
+                      scoped_ptr<base::Value> value);
+
+  // Check if we are done reading all data from storage, and notify the
+  // RulesRegistry if so.

[battre, 2012/06/15 22:56:49]

add comment how this notification happens?

[Matt Perry, 2012/06/18 21:01:01]

I'm not sure I understand what you mean. I added a note that the notification
happens on the RulesRegistry thread.

+  void CheckIfReady();
+
+  // Deserialize the rules from the given Value object and add them to the
+  // RulesRegistry.
+  void ReadFromStorageOnRegistryThread(const std::string& extension_id,
+                                       scoped_ptr<base::Value> value);
+
+  // Notify the RulesRegistry that we are now ready.
+  void NotifyReadyOnRegistryThread();
+
+  content::NotificationRegistrar registrar_;
+  Profile* profile_;
+
+  // The key under which rules are stored.
+  const std::string storage_key_;
+
+  // A set of extension IDs that have rules we are reading from storage.
+  std::set<std::string> waiting_for_extensions_;
+
+  // The thread that our RulesRegistry lives on.
+  content::BrowserThread::ID rules_registry_thread_;
+
+  // The following are only accessible on rules_registry_thread_.
+
+  // The RulesRegistry whose delegate we are.
+  RulesRegistryWithCache* rules_registry_;
+
+  // True when we have finished reading from storage for all extensions that
+  // are loaded on startup.
+  bool ready_;
+};
+
+RulesRegistryStorageDelegate::RulesRegistryStorageDelegate() {
+}
+
+RulesRegistryStorageDelegate::~RulesRegistryStorageDelegate() {
+  // RulesRegistry owns us, which means it has been deleted.
+  inner_->rules_registry_ = NULL;
+}
+
+void RulesRegistryStorageDelegate::Init(Profile* profile,
+                                RulesRegistryWithCache* rules_registry,
+                                const std::string& storage_key) {

[battre, 2012/06/15 22:56:49]

nit: indent

[Matt Perry, 2012/06/18 21:01:01]

Done.

+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  inner_ = new Inner(profile, rules_registry, storage_key);
+}
+
+bool RulesRegistryStorageDelegate::IsReady() {
+  DCHECK(content::BrowserThread::CurrentlyOn(inner_->rules_registry_thread_));
+  return inner_->ready_;

[battre, 2012/06/15 22:56:49]

this contradicts the comment "Methods/variables should be used on the UI thread
unless otherwise noted."

[Matt Perry, 2012/06/18 21:01:01]

Are you referring to ready_? ready_'s definition follows the comment "The
following are only accessible on rules_registry_thread_."

+}
+
+void RulesRegistryStorageDelegate::OnRulesChanged(
+    RulesRegistryWithCache* rules_registry,
+    const std::string& extension_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(inner_->rules_registry_thread_));
+  std::vector<linked_ptr<RulesRegistry::Rule> > new_rules;
+  std::string error = rules_registry->GetAllRules(extension_id, &new_rules);
+  DCHECK_EQ("", error);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&Inner::WriteToStorage, inner_.get(), extension_id,
+                 base::Passed(RulesToValue(new_rules))));
+}
+
+RulesRegistryStorageDelegate::Inner::Inner(Profile* profile,
+                                   RulesRegistryWithCache* rules_registry,
+                                   const std::string& storage_key)

[battre, 2012/06/15 22:56:49]

nit: indent

[Matt Perry, 2012/06/18 21:01:01]

Done.

+    : profile_(profile),
+      storage_key_(storage_key),
+      rules_registry_thread_(rules_registry->GetOwnerThread()),
+      rules_registry_(rules_registry),
+      ready_(false) {
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
+                 content::Source<Profile>(profile));
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSIONS_READY,
+                 content::Source<Profile>(profile));
+}
+
+RulesRegistryStorageDelegate::Inner::~Inner() {}
+
+void RulesRegistryStorageDelegate::Inner::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  if (type == chrome::NOTIFICATION_EXTENSION_LOADED) {
+    const extensions::Extension* extension =
+        content::Details<const extensions::Extension>(details).ptr();
+    extensions::StateStore* store =
+        ExtensionSystem::Get(profile_)->state_store();
+    if (store && extension->HasAPIPermission(
+            ExtensionAPIPermission::kDeclarativeWebRequest)) {

[Matt Perry, 2012/06/15 21:54:41]

Is there a better way to check if an extension is using declarative rules? I
tried checking for the declarative permission, but apparently
"declarativeWebRequest" does not imply "declarative".

[battre, 2012/06/15 22:56:49]

"declarative" (and ExtensionAPIPermission::kDeclarative) is an artifact that I
forgot to delete when I migrated the declarative API definition into the
chrome.Events namespace. We could revive it for the purpose you mention.

[Matt Perry, 2012/06/18 21:01:01]

I'll leave as-is for now but add a TODO.

+      waiting_for_extensions_.insert(extension->id());
+      store->GetExtensionValue(extension->id(), storage_key_,
+          base::Bind(&Inner::ReadFromStorage, this, extension->id()));

[battre, 2012/06/15 22:56:49]

What do you think of moving 177-178 into a function
void ReadFromStorage(const std::string& extension_id)
and renaming 185 to ReadFromStorageCallback(...)?

I found it confusing that ReadFromStorage would consume a
scoped_ptr<base::Value>.

[Matt Perry, 2012/06/18 21:01:01]

Done.

+    }
+  } else if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
+    CheckIfReady();
+  }
+}
+
+void RulesRegistryStorageDelegate::Inner::ReadFromStorage(
+    const std::string& extension_id, scoped_ptr<base::Value> value) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  content::BrowserThread::PostTask(
+      rules_registry_thread_, FROM_HERE,
+      base::Bind(&Inner::ReadFromStorageOnRegistryThread, this,
+                 extension_id, base::Passed(value.Pass())));
+
+  waiting_for_extensions_.erase(extension_id);
+  CheckIfReady();
+}
+
+void RulesRegistryStorageDelegate::Inner::WriteToStorage(
+    const std::string& extension_id, scoped_ptr<base::Value> value) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  StateStore* store = ExtensionSystem::Get(profile_)->state_store();
+  if (store)
+    store->SetExtensionValue(extension_id, storage_key_, value.Pass());
+}
+
+void RulesRegistryStorageDelegate::Inner::CheckIfReady() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (!waiting_for_extensions_.empty())
+    return;
+
+  if (ready_)

[battre, 2012/06/15 22:56:49]

access to ready_ on UI thread

[Matt Perry, 2012/06/18 21:01:01]

Doh! Good catch.

+    return;  // we've already notified our readiness

[battre, 2012/06/15 22:56:49]

This behavior (call callback only once) is not reflected in the method comment.

I think there is a race condition possible if CheckIfReady() is called twice in
short sequence before the NotifyReadyOnRegistryThread task can be executed?

[Matt Perry, 2012/06/18 21:01:01]

Added comments to CheckIfReady and OnReady.
Good catch - moving the ready_ check to the RulesRegistry thread fixes this.

+
+  content::BrowserThread::PostTask(
+      rules_registry_thread_, FROM_HERE,
+      base::Bind(&Inner::NotifyReadyOnRegistryThread, this));
+}
+
+void RulesRegistryStorageDelegate::Inner::ReadFromStorageOnRegistryThread(
+    const std::string& extension_id, scoped_ptr<base::Value> value) {
+  DCHECK(content::BrowserThread::CurrentlyOn(rules_registry_thread_));
+  if (!rules_registry_)
+    return;  // registry went away
+
+  rules_registry_->AddRules(extension_id, RulesFromValue(value.get()));
+}
+
+void RulesRegistryStorageDelegate::Inner::NotifyReadyOnRegistryThread() {
+  DCHECK(content::BrowserThread::CurrentlyOn(rules_registry_thread_));
+  ready_ = true;

[battre, 2012/06/15 22:56:49]

access to ready_ on rules_registry_thread_

[Matt Perry, 2012/06/18 21:01:01]

That is the intended thread.

+  rules_registry_->OnReady();
+}
+
+}  // namespace extensions