Declarative rules should be removed on uninstalling, not unloading

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.cc b/chrome/browser/extensions/api/declarative/rules_registry.cc
index 369712cef584fbb6b61cd95a07c86a435e3f1df9..7063ea49d25a8722c5226fa310ef3e341258ae14 100644
--- a/chrome/browser/extensions/api/declarative/rules_registry.cc
+++ b/chrome/browser/extensions/api/declarative/rules_registry.cc
@@ -141,37 +141,35 @@ std::string RulesRegistry::AddRules(
   return AddRulesNoFill(extension_id, rules);
 }
 
-std::string RulesRegistry::RemoveRules(
+void RulesRegistry::CommitRemoveRules(
     const std::string& extension_id,
     const std::vector<std::string>& rule_identifiers) {
-  DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
-
-  std::string error = RemoveRulesImpl(extension_id, rule_identifiers);
-
-  if (!error.empty())
-    return error;
-
-  // Commit removal of rules from |rules_| on success.
-  for (std::vector<std::string>::const_iterator i =
-      rule_identifiers.begin(); i != rule_identifiers.end(); ++i) {
+  for (std::vector<std::string>::const_iterator i = rule_identifiers.begin();
+       i != rule_identifiers.end();
+       ++i) {
     RulesDictionaryKey lookup_key(extension_id, *i);
     rules_.erase(lookup_key);
   }
 
   MaybeProcessChangedRules(extension_id);
   RemoveUsedRuleIdentifiers(extension_id, rule_identifiers);
-  return kSuccess;
 }
 
-std::string RulesRegistry::RemoveAllRules(const std::string& extension_id) {
+std::string RulesRegistry::RemoveRules(
+    const std::string& extension_id,
+    const std::vector<std::string>& rule_identifiers) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
-  std::string error = RemoveAllRulesImpl(extension_id);
+  std::string error = RemoveRulesImpl(extension_id, rule_identifiers);
 
   if (!error.empty())
     return error;
 
-  // Commit removal of rules from |rules_| on success.
+  CommitRemoveRules(extension_id, rule_identifiers);
+  return kSuccess;
+}
+
+void RulesRegistry::CommitRemoveAllRules(const std::string& extension_id) {
   for (RulesDictionary::const_iterator i = rules_.begin();
       i != rules_.end();) {
     const RulesDictionaryKey& key = i->first;
@@ -182,6 +180,17 @@ std::string RulesRegistry::RemoveAllRules(const std::string& extension_id) {
 
   MaybeProcessChangedRules(extension_id);
   RemoveAllUsedRuleIdentifiers(extension_id);
+}
+
+std::string RulesRegistry::RemoveAllRules(const std::string& extension_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
+
+  std::string error = RemoveAllRulesImpl(extension_id);
+
+  if (!error.empty())
+    return error;
+
+  CommitRemoveAllRules(extension_id);
   return kSuccess;
 }
 
@@ -213,10 +222,22 @@ void RulesRegistry::GetAllRules(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);
+  std::string error = RemoveAllRulesImpl(extension_id);
+  if (!error.empty())
+    LOG(ERROR) << error;
+}
+
+void RulesRegistry::OnExtensionUninstalled(const std::string& extension_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
+  CommitRemoveAllRules(extension_id);

[Jeffrey Yasskin, 2013/11/20 05:55:28]

Comment that we don't need to call RemoveAllRulesImpl because
OnExtensionUnloaded is guaranteed to be called before OnExtensionUninstalled. Or
just forward to RemoveAllRules() to be clearly safe, since removing an empty set
of rules should be pretty cheap.

[vabr (Chromium), 2013/11/20 17:46:07]

Done -- I went for calling RemoveAllRules. Thanks for this observation!

+}
+
+void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) {
+  std::vector<linked_ptr<Rule> > rules;
+  GetAllRules(extension_id, &rules);
+  std::string error = AddRulesImpl(extension_id, rules);
   if (!error.empty())
     LOG(ERROR) << error;
-  used_rule_identifiers_.erase(extension_id);
 }
 
 size_t RulesRegistry::GetNumberOfUsedRuleIdentifiersForTesting() const {