Declarative rules should be removed on uninstalling, not unloading

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

 // 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_RULES_REGISTRY_H__
 #define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_RULES_REGISTRY_H__
 
 #include "chrome/browser/extensions/api/declarative/rules_registry.h"
 
 #include <map>
@@ skipping 102 matching lines @@
   //
   // The returned rules are stored in |out|. Ownership is passed to the caller.
   void GetRules(const std::string& extension_id,
                 const std::vector<std::string>& rule_identifiers,
                 std::vector<linked_ptr<RulesRegistry::Rule> >* out);
 
   // Same as GetRules but returns all rules owned by |extension_id|.
   void GetAllRules(const std::string& extension_id,
                    std::vector<linked_ptr<RulesRegistry::Rule> >* out);
 
-  // Called to notify the RulesRegistry that an extension has been unloaded
-  // and all rules of this extension need to be removed.
+  // Called to notify the RulesRegistry that the extension availability has
+  // changed, so that the registry can update which rules are active.
   void OnExtensionUnloaded(const std::string& extension_id);
+  void OnExtensionUninstalled(const std::string& extension_id);
+  void OnExtensionLoaded(const std::string& extension_id);
 
   // Returns the number of entries in used_rule_identifiers_ for leak detection.
   // Every ExtensionId counts as one entry, even if it contains no rules.
   size_t GetNumberOfUsedRuleIdentifiersForTesting() const;
 
   // Returns the RulesCacheDelegate. This is used for testing.
   RulesCacheDelegate* rules_cache_delegate_for_testing() const {
     return cache_delegate_.get();
   }
 
@@ skipping 70 matching lines @@
   // NOT_SCHEDULED_FOR_PROCESSING.
   void MaybeProcessChangedRules(const std::string& extension_id);
 
   void MarkReady(base::Time storage_init_time);
 
   // Deserialize the rules from the given Value object and add them to the
   // RulesRegistry.
   void DeserializeAndAddRules(const std::string& extension_id,
                               scoped_ptr<base::Value> rules);
 
+  // These functions are responsible for removing rules from RulesRegistry
+  // storage (as opposed to the storage of the inheriting registries, handled by

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

This isn't clear. Maybe "These functions remove rules from RulesRegistry
persistent storage. This contrasts to Remove[All]RulesImpl, which prevents the
rules from affecting Chrome's behavior."

Consider renaming AddRulesImpl to ActivateRules and Remove[All]RulesImpl to
Deactivate[All]Rules. Then CommitRemove[All]Rules could become Delete[All]Rules,
making the distinction somewhat easier to explain.

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

I was also unhappy about the naming.

However, following your other suggestion, I merged the Commit* back to the
non-impl Remove* methods. For RemoveRules there was actually no reason for me to
separate those, and RemoveAllRules can be called during uninstalling including
the impl part, as you observed.

+  // Remove(All)RulesImpl).
+  void CommitRemoveRules(const std::string& extension_id,
+                         const std::vector<std::string>& rule_identifiers);
+  void CommitRemoveAllRules(const std::string& extension_id);
+
   // The profile to which this rules registry belongs.
   Profile* profile_;
 
   // The ID of the thread on which the rules registry lives.
   const content::BrowserThread::ID owner_thread_;
 
   // The name of the event with which rules are registered.
   const std::string event_name_;
 
   // The key that identifies the context in which these rules apply.
@@ skipping 51 matching lines @@
   typedef std::map<ExtensionId, std::set<RuleIdentifier> > RuleIdentifiersMap;
   RuleIdentifiersMap used_rule_identifiers_;
   int last_generated_rule_identifier_id_;
 
   DISALLOW_COPY_AND_ASSIGN(RulesRegistry);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_RULES_REGISTRY_H__