Refactor RulesRegistryWithCache to RulesRegistry

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

Index: chrome/browser/extensions/api/declarative/rules_registry.cc
diff --git a/chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc b/chrome/browser/extensions/api/declarative/rules_registry.cc
similarity index 42%
rename from chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
rename to chrome/browser/extensions/api/declarative/rules_registry.cc
index a452130fb03c26eacd4b2a2c1590c28fdd0c1538..675ba8fd68e1fff8c09f5d5d2a3aa209fbfe09dc 100644
--- a/chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
+++ b/chrome/browser/extensions/api/declarative/rules_registry.cc
@@ -2,7 +2,7 @@
 // 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_with_cache.h"
+#include "chrome/browser/extensions/api/declarative/rules_registry.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ -12,6 +12,7 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/api/declarative/rules_cache_delegate.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -59,13 +60,8 @@ std::vector<linked_ptr<extensions::RulesRegistry::Rule> > RulesFromValue(
   return rules;
 }
 
-// Returns the key to use for storing declarative rules in the state store.
-std::string GetDeclarativeRuleStorageKey(const std::string& event_name,
-                                         bool incognito) {
-  if (incognito)
-    return "declarative_rules.incognito." + event_name;
-  else
-    return "declarative_rules." + event_name;
+std::string ToId(int identifier) {
+  return base::StringPrintf("_%d_", identifier);
 }
 
 }  // namespace
@@ -73,40 +69,47 @@ std::string GetDeclarativeRuleStorageKey(const std::string& event_name,
 
 namespace extensions {
 
-// RulesRegistryWithCache
+// RulesRegistry
 
-RulesRegistryWithCache::RulesRegistryWithCache(
+RulesRegistry::RulesRegistry(
     Profile* profile,
     const std::string& event_name,
     content::BrowserThread::ID owner_thread,
     bool log_storage_init_delay,
-    scoped_ptr<RuleStorageOnUI>* ui_part)
-    : RulesRegistry(owner_thread, event_name),
+    scoped_ptr<RulesCacheDelegate>* ui_part)
+    : owner_thread_(owner_thread),
+      event_name_(event_name),
       weak_ptr_factory_(profile ? this : NULL),
-      storage_on_ui_(
-          (profile ? (new RuleStorageOnUI(profile,
+      cache_delegate_(
+          (profile ? (new RulesCacheDelegate(profile,
                                           event_name,
                                           owner_thread,
                                           weak_ptr_factory_.GetWeakPtr(),
                                           log_storage_init_delay))->GetWeakPtr()
-                   : base::WeakPtr<RuleStorageOnUI>())),
+                   : base::WeakPtr<RulesCacheDelegate>())),
       process_changed_rules_requested_(profile ? NOT_SCHEDULED_FOR_PROCESSING
-                                               : NEVER_PROCESS) {
+                                               : NEVER_PROCESS),
+      last_generated_rule_identifier_id_(0) {
   if (!profile) {
     CHECK(!ui_part);
     return;
   }
 
-  ui_part->reset(storage_on_ui_.get());
+  ui_part->reset(cache_delegate_.get());
 
-  storage_on_ui_->Init();
+  cache_delegate_->Init();
 }
 
-std::string RulesRegistryWithCache::AddRules(
+std::string RulesRegistry::AddRules(
     const std::string& extension_id,
     const std::vector<linked_ptr<Rule> >& rules) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
+  std::string error = CheckAndFillInOptionalRules(extension_id, rules);

[Jeffrey Yasskin, 2013/10/30 22:30:48]

This is probably fine, but note that, now that InitializingRulesRegistry and
RulesRegistryWithCache are the same, on startup we'll call these FillIn methods
redundantly when re-adding the rules from the cache.

[Fady Samuel, 2013/10/31 14:02:24]

OK. This is one time operation on browser startup that hopefully isn't too
expensive.

[vabr (Chromium), 2013/10/31 16:35:05]

Actually, the start-up time is a critical point. We spent some time this Spring
trying to speed that part up. For Declarative Web Request rules we have to deal
with the order of tens of thousands of rules. For that numbers, all the string
processing like in CheckAndFillInOptionalRules could easily sum up to hundreds
of ms. So if there is a way to remove redundant calls (in a follow-up CL), that
would be great.

+  if (!error.empty())
+    return error;
+  FillInOptionalPriorities(rules);
+
   // Verify that all rule IDs are new.
   for (std::vector<linked_ptr<Rule> >::const_iterator i =
       rules.begin(); i != rules.end(); ++i) {
@@ -116,7 +119,7 @@ std::string RulesRegistryWithCache::AddRules(
       return base::StringPrintf(kDuplicateRuleId, rule_id.c_str());
   }
 
-  std::string error = AddRulesImpl(extension_id, rules);
+  error = AddRulesImpl(extension_id, rules);
 
   if (!error.empty())
     return error;
@@ -133,7 +136,7 @@ std::string RulesRegistryWithCache::AddRules(
   return kSuccess;
 }
 
-std::string RulesRegistryWithCache::RemoveRules(
+std::string RulesRegistry::RemoveRules(
     const std::string& extension_id,
     const std::vector<std::string>& rule_identifiers) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
@@ -151,11 +154,11 @@ std::string RulesRegistryWithCache::RemoveRules(
   }
 
   MaybeProcessChangedRules(extension_id);
+  RemoveUsedRuleIdentifiers(extension_id, rule_identifiers);
   return kSuccess;
 }
 
-std::string RulesRegistryWithCache::RemoveAllRules(
-    const std::string& extension_id) {
+std::string RulesRegistry::RemoveAllRules(const std::string& extension_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   std::string error = RemoveAllRulesImpl(extension_id);
@@ -173,10 +176,11 @@ std::string RulesRegistryWithCache::RemoveAllRules(
   }
 
   MaybeProcessChangedRules(extension_id);
+  RemoveAllUsedRuleIdentifiers(extension_id);
   return kSuccess;
 }
 
-std::string RulesRegistryWithCache::GetRules(
+std::string RulesRegistry::GetRules(
     const std::string& extension_id,
     const std::vector<std::string>& rule_identifiers,
     std::vector<linked_ptr<RulesRegistry::Rule> >* out) {
@@ -192,7 +196,7 @@ std::string RulesRegistryWithCache::GetRules(
   return kSuccess;
 }
 
-std::string RulesRegistryWithCache::GetAllRules(
+std::string RulesRegistry::GetAllRules(
     const std::string& extension_id,
     std::vector<linked_ptr<RulesRegistry::Rule> >* out) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
@@ -206,18 +210,31 @@ std::string RulesRegistryWithCache::GetAllRules(
   return kSuccess;
 }
 
-void RulesRegistryWithCache::OnExtensionUnloaded(
-    const std::string& extension_id) {
+void RulesRegistry::OnExtensionUnloaded(const std::string& extension_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
   std::string error = RemoveAllRules(extension_id);
   if (!error.empty())
     LOG(ERROR) << error;
+  used_rule_identifiers_.erase(extension_id);
+}
+
+size_t RulesRegistry::GetNumberOfUsedRuleIdentifiersForTesting() const {
+  size_t entry_count = 0u;
+  for (RuleIdentifiersMap::const_iterator extension =
+           used_rule_identifiers_.begin();
+       extension != used_rule_identifiers_.end();
+       ++extension) {
+    // Each extension is counted as 1 just for being there. Otherwise we miss
+    // keys with empty values.
+    entry_count += 1u + extension->second.size();
+  }
+  return entry_count;
 }
 
-RulesRegistryWithCache::~RulesRegistryWithCache() {
+RulesRegistry::~RulesRegistry() {
 }
 
-void RulesRegistryWithCache::MarkReady(base::Time storage_init_time) {
+void RulesRegistry::MarkReady(base::Time storage_init_time) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   if (!storage_init_time.is_null()) {
@@ -228,7 +245,7 @@ void RulesRegistryWithCache::MarkReady(base::Time storage_init_time) {
   ready_.Signal();
 }
 
-void RulesRegistryWithCache::DeserializeAndAddRules(
+void RulesRegistry::DeserializeAndAddRules(
     const std::string& extension_id,
     scoped_ptr<base::Value> rules) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
@@ -236,8 +253,7 @@ void RulesRegistryWithCache::DeserializeAndAddRules(
   AddRules(extension_id, RulesFromValue(rules.get()));
 }
 
-void RulesRegistryWithCache::ProcessChangedRules(
-    const std::string& extension_id) {
+void RulesRegistry::ProcessChangedRules(const std::string& extension_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   process_changed_rules_requested_ = NOT_SCHEDULED_FOR_PROCESSING;
@@ -248,237 +264,91 @@ void RulesRegistryWithCache::ProcessChangedRules(
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&RuleStorageOnUI::WriteToStorage,
-                 storage_on_ui_,
+      base::Bind(&RulesCacheDelegate::WriteToStorage,
+                 cache_delegate_,
                  extension_id,
                  base::Passed(RulesToValue(new_rules))));
 }
 
-void RulesRegistryWithCache::MaybeProcessChangedRules(
-    const std::string& extension_id) {
+void RulesRegistry::MaybeProcessChangedRules(const std::string& extension_id) {
   if (process_changed_rules_requested_ != NOT_SCHEDULED_FOR_PROCESSING)
     return;
 
   process_changed_rules_requested_ = SCHEDULED_FOR_PROCESSING;
   ready_.Post(FROM_HERE,
-              base::Bind(&RulesRegistryWithCache::ProcessChangedRules,
+              base::Bind(&RulesRegistry::ProcessChangedRules,
                          weak_ptr_factory_.GetWeakPtr(),
                          extension_id));
 }
 
-// RulesRegistryWithCache::RuleStorageOnUI
-
-const char RulesRegistryWithCache::RuleStorageOnUI::kRulesStoredKey[] =
-    "has_declarative_rules";
-
-RulesRegistryWithCache::RuleStorageOnUI::RuleStorageOnUI(
-    Profile* profile,
-    const std::string& event_name,
-    content::BrowserThread::ID rules_registry_thread,
-    base::WeakPtr<RulesRegistryWithCache> registry,
-    bool log_storage_init_delay)
-    : profile_(profile),
-      storage_key_(GetDeclarativeRuleStorageKey(event_name,
-                                                profile->IsOffTheRecord())),
-      rules_stored_key_(GetRulesStoredKey(event_name,
-                                          profile->IsOffTheRecord())),
-      log_storage_init_delay_(log_storage_init_delay),
-      registry_(registry),
-      rules_registry_thread_(rules_registry_thread),
-      notified_registry_(false),
-      weak_ptr_factory_(this) {}
-
-RulesRegistryWithCache::RuleStorageOnUI::~RuleStorageOnUI() {}
-
-// Returns the key to use for storing whether the rules have been stored.
-// static
-std::string RulesRegistryWithCache::RuleStorageOnUI::GetRulesStoredKey(
-    const std::string& event_name,
-    bool incognito) {
-  std::string result(kRulesStoredKey);
-  result += incognito ? ".incognito." : ".";
-  return result + event_name;
+bool RulesRegistry::IsUniqueId(const std::string& extension_id,
+                               const std::string& rule_id) const {
+  RuleIdentifiersMap::const_iterator identifiers =
+      used_rule_identifiers_.find(extension_id);
+  if (identifiers == used_rule_identifiers_.end())
+    return true;
+  return identifiers->second.find(rule_id) == identifiers->second.end();
 }
 
-// This is called from the constructor of RulesRegistryWithCache, so it is
-// important that it both
-// 1. calls no (in particular virtual) methods of the rules registry, and
-// 2. does not create scoped_refptr holding the registry. (A short-lived
-// scoped_refptr might delete the rules registry before it is constructed.)
-void RulesRegistryWithCache::RuleStorageOnUI::Init() {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-
-  ExtensionSystem& system = *ExtensionSystem::Get(profile_);
-  extensions::StateStore* store = system.rules_store();
-  if (store)
-    store->RegisterKey(storage_key_);
-
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_EXTENSION_LOADED,
-                 content::Source<Profile>(profile_->GetOriginalProfile()));
-
-  if (profile_->IsOffTheRecord())
-    log_storage_init_delay_ = false;
-
-  system.ready().Post(
-      FROM_HERE,
-      base::Bind(&RuleStorageOnUI::ReadRulesForInstalledExtensions,
-                 GetWeakPtr()));
-  system.ready().Post(FROM_HERE,
-                      base::Bind(&RuleStorageOnUI::CheckIfReady, GetWeakPtr()));
+std::string RulesRegistry::GenerateUniqueId(const std::string& extension_id) {
+  while (!IsUniqueId(extension_id, ToId(last_generated_rule_identifier_id_)))
+    ++last_generated_rule_identifier_id_;
+  return ToId(last_generated_rule_identifier_id_);
 }
 
-void RulesRegistryWithCache::RuleStorageOnUI::WriteToStorage(
+std::string RulesRegistry::CheckAndFillInOptionalRules(
     const std::string& extension_id,
-    scoped_ptr<base::Value> value) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  if (!profile_)
-    return;
+    const std::vector<linked_ptr<RulesRegistry::Rule> >& rules) {
+  // IDs we have inserted, in case we need to rollback this operation.
+  std::vector<std::string> rollback_log;
 
-  const base::ListValue* rules = NULL;
-  CHECK(value->GetAsList(&rules));
-  bool rules_stored_previously = GetDeclarativeRulesStored(extension_id);
-  bool store_rules = !rules->empty();
-  SetDeclarativeRulesStored(extension_id, store_rules);
-  if (!rules_stored_previously && !store_rules)
-    return;
-
-  StateStore* store = ExtensionSystem::Get(profile_)->rules_store();
-  if (store)
-    store->SetExtensionValue(extension_id, storage_key_, value.Pass());
-}
-
-void RulesRegistryWithCache::RuleStorageOnUI::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  DCHECK(type == chrome::NOTIFICATION_EXTENSION_LOADED);
-
-  const extensions::Extension* extension =
-      content::Details<const extensions::Extension>(details).ptr();
-  // TODO(mpcomplete): This API check should generalize to any use of
-  // declarative rules, not just webRequest.
-  if (extension->HasAPIPermission(APIPermission::kDeclarativeContent) ||
-      extension->HasAPIPermission(APIPermission::kDeclarativeWebRequest)) {
-    ExtensionInfoMap* extension_info_map =
-        ExtensionSystem::Get(profile_)->info_map();
-    if (profile_->IsOffTheRecord() &&
-        !extension_info_map->IsIncognitoEnabled(extension->id())) {
-      // Ignore this extension.
-    } else {
-      ReadFromStorage(extension->id());
+  // First we insert all rules with existing identifier, so that generated
+  // identifiers cannot collide with identifiers passed by the caller.
+  for (std::vector<linked_ptr<RulesRegistry::Rule> >::const_iterator i =
+      rules.begin(); i != rules.end(); ++i) {
+    RulesRegistry::Rule* rule = i->get();
+    if (rule->id.get()) {
+      std::string id = *(rule->id);
+      if (!IsUniqueId(extension_id, id)) {
+        RemoveUsedRuleIdentifiers(extension_id, rollback_log);
+        return "Id " + id + " was used multiple times.";
+      }
+      used_rule_identifiers_[extension_id].insert(id);
     }
   }
-}
-
-void RulesRegistryWithCache::RuleStorageOnUI::CheckIfReady() {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  if (notified_registry_ || !waiting_for_extensions_.empty())
-    return;
-
-  content::BrowserThread::PostTask(
-      rules_registry_thread_,
-      FROM_HERE,
-      base::Bind(
-          &RulesRegistryWithCache::MarkReady, registry_, storage_init_time_));
-  notified_registry_ = true;
-}
-
-void
-RulesRegistryWithCache::RuleStorageOnUI::ReadRulesForInstalledExtensions() {
-  ExtensionSystem& system = *ExtensionSystem::Get(profile_);
-  ExtensionService* extension_service = system.extension_service();
-  DCHECK(extension_service);
-  // In an OTR profile, we start on top of a normal profile already, so the
-  // extension service should be ready.
-  DCHECK(!profile_->IsOffTheRecord() || extension_service->is_ready());
-  if (extension_service->is_ready()) {
-    const ExtensionSet* extensions = extension_service->extensions();
-    for (ExtensionSet::const_iterator i = extensions->begin();
-         i != extensions->end();
-         ++i) {
-      bool needs_apis_storing_rules =
-          (*i)->HasAPIPermission(APIPermission::kDeclarativeContent) ||
-          (*i)->HasAPIPermission(APIPermission::kDeclarativeWebRequest);
-      bool respects_off_the_record =
-          !(profile_->IsOffTheRecord()) ||
-          extension_util::IsIncognitoEnabled((*i)->id(), extension_service);
-      if (needs_apis_storing_rules && respects_off_the_record)
-        ReadFromStorage((*i)->id());
+  // Now we generate IDs in case they were not specificed in the rules. This

[Jeffrey Yasskin, 2013/10/30 22:30:48]

sp: specificed

[Fady Samuel, 2013/10/31 14:02:24]

Done.

+  // cannot fail so we do not need to keep track of a rollback log.
+  for (std::vector<linked_ptr<RulesRegistry::Rule> >::const_iterator i =
+      rules.begin(); i != rules.end(); ++i) {
+    RulesRegistry::Rule* rule = i->get();
+    if (!rule->id.get()) {
+      rule->id.reset(new std::string(GenerateUniqueId(extension_id)));
+      used_rule_identifiers_[extension_id].insert(*(rule->id));
     }
   }
+  return std::string();
 }
 
-void RulesRegistryWithCache::RuleStorageOnUI::ReadFromStorage(
-    const std::string& extension_id) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  if (!profile_)
-    return;
-
-  if (log_storage_init_delay_ && storage_init_time_.is_null())
-    storage_init_time_ = base::Time::Now();
-
-  if (!GetDeclarativeRulesStored(extension_id)) {
-    ExtensionSystem::Get(profile_)->ready().Post(
-        FROM_HERE, base::Bind(&RuleStorageOnUI::CheckIfReady, GetWeakPtr()));
-    return;
+void RulesRegistry::FillInOptionalPriorities(
+    const std::vector<linked_ptr<RulesRegistry::Rule> >& rules) {
+  std::vector<linked_ptr<RulesRegistry::Rule> >::const_iterator i;
+  for (i = rules.begin(); i != rules.end(); ++i) {
+    if (!(*i)->priority.get())
+      (*i)->priority.reset(new int(DEFAULT_PRIORITY));
   }
-
-  extensions::StateStore* store = ExtensionSystem::Get(profile_)->rules_store();
-  if (!store)
-    return;
-  waiting_for_extensions_.insert(extension_id);
-  store->GetExtensionValue(extension_id,
-                           storage_key_,
-                           base::Bind(&RuleStorageOnUI::ReadFromStorageCallback,
-                                      weak_ptr_factory_.GetWeakPtr(),
-                                      extension_id));
 }
 
-void RulesRegistryWithCache::RuleStorageOnUI::ReadFromStorageCallback(
+void RulesRegistry::RemoveUsedRuleIdentifiers(
     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(&RulesRegistryWithCache::DeserializeAndAddRules,
-                 registry_,
-                 extension_id,
-                 base::Passed(&value)));
-
-  waiting_for_extensions_.erase(extension_id);
-
-  if (waiting_for_extensions_.empty())
-    ExtensionSystem::Get(profile_)->ready().Post(
-        FROM_HERE, base::Bind(&RuleStorageOnUI::CheckIfReady, GetWeakPtr()));
+    const std::vector<std::string>& identifiers) {
+  std::vector<std::string>::const_iterator i;
+  for (i = identifiers.begin(); i != identifiers.end(); ++i)
+    used_rule_identifiers_[extension_id].erase(*i);
 }
 
-bool RulesRegistryWithCache::RuleStorageOnUI::GetDeclarativeRulesStored(
-    const std::string& extension_id) const {
-  CHECK(profile_);
-  const ExtensionScopedPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
-
-  bool rules_stored = true;
-  if (extension_prefs->ReadPrefAsBoolean(
-          extension_id, rules_stored_key_, &rules_stored))
-    return rules_stored;
-
-  // Safe default -- if we don't know that the rules are not stored, we force
-  // a read by returning true.
-  return true;
-}
-
-void RulesRegistryWithCache::RuleStorageOnUI::SetDeclarativeRulesStored(
-    const std::string& extension_id,
-    bool rules_stored) {
-  CHECK(profile_);
-  ExtensionScopedPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
-  extension_prefs->UpdateExtensionPref(
-      extension_id,
-      rules_stored_key_,
-      new base::FundamentalValue(rules_stored));
+void RulesRegistry::RemoveAllUsedRuleIdentifiers(
+    const std::string& extension_id) {
+  used_rule_identifiers_.erase(extension_id);
 }
 
 }  // namespace extensions