Remove WebViewKey in rules registry.

extensions/browser/api/declarative/rules_registry_service.cc

 // 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 "extensions/browser/api/declarative/rules_registry_service.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "extensions/browser/api/declarative/rules_cache_delegate.h"
 #include "extensions/browser/api/declarative_content/content_rules_registry.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_constants.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h"
 #include "extensions/browser/api/extensions_api_client.h"
 #include "extensions/browser/api/web_request/web_request_api.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 
 namespace extensions {
 
 namespace {
 
 // Registers |web_request_rules_registry| on the IO thread.
 void RegisterToExtensionWebRequestEventRouterOnIO(
     content::BrowserContext* browser_context,
-    const RulesRegistryService::WebViewKey& webview_key,
+    int rules_registry_id,
     scoped_refptr<WebRequestRulesRegistry> web_request_rules_registry) {
   ExtensionWebRequestEventRouter::GetInstance()->RegisterRulesRegistry(
-      browser_context, webview_key, web_request_rules_registry);
-}
-
-bool IsWebView(const RulesRegistryService::WebViewKey& webview_key) {
-  return webview_key.embedder_process_id && webview_key.webview_instance_id;
+      browser_context, rules_registry_id, web_request_rules_registry);
 }
 
 }  // namespace
 
+const int RulesRegistryService::kDefaultRulesRegistryID = 0;
+
 RulesRegistryService::RulesRegistryService(content::BrowserContext* context)
-    : content_rules_registry_(NULL),
+    : current_rules_registry_id_(0),

[Fady Samuel, 2014/11/28 11:41:31]

Use kDefaultRulesRegistryID

[Xi Han, 2014/11/28 16:12:56]

Done.

+      content_rules_registry_(NULL),
       extension_registry_observer_(this),
       browser_context_(context) {
   if (browser_context_) {
     extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
-    registrar_.Add(
-        this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
-        content::NotificationService::AllBrowserContextsAndSources());
-    EnsureDefaultRulesRegistriesRegistered(WebViewKey(0, 0));
+    EnsureDefaultRulesRegistriesRegistered(kDefaultRulesRegistryID, false);
   }
 }
 
 RulesRegistryService::~RulesRegistryService() {}
 
+int RulesRegistryService::GetNextRulesRegistryID() {
+  return ++current_rules_registry_id_;
+}
+
 void RulesRegistryService::EnsureDefaultRulesRegistriesRegistered(
-    const WebViewKey& webview_key) {
+    int rules_registry_id,
+    bool cache_rules) {

[Fady Samuel, 2014/11/28 11:34:24]

You know, now that I think about it, we can probably get rid of cache_rules too.
Then simply add a comment about how:

// Only cache rules for regular pages.

[Xi Han, 2014/11/28 16:12:56]

This is a great idea, thanks.

   if (!browser_context_)
     return;
-
   RulesRegistryKey key(declarative_webrequest_constants::kOnRequest,
-                       webview_key);
+                       rules_registry_id);
   // If we can find the key in the |rule_registries_| then we have already
   // installed the default registries.
   if (ContainsKey(rule_registries_, key))
     return;
 
 
   RulesCacheDelegate* web_request_cache_delegate = NULL;
-  if (!IsWebView(webview_key)) {
+  if (!cache_rules) {

[Fady Samuel, 2014/11/28 11:34:24]

The logic here is reversed. if (cache_rules) then do whatever.

I would suggest:

if (rules_registry_id == kDefaultRulesRegistryID) {
  ....
  // create a RulesCacheDelegate here.

}

[Xi Han, 2014/11/28 16:12:56]

Thanks a lot for catching up this reversed logic. Updated.

     web_request_cache_delegate =
         new RulesCacheDelegate(true /*log_storage_init_delay*/);
     cache_delegates_.push_back(web_request_cache_delegate);
   }
   scoped_refptr<WebRequestRulesRegistry> web_request_rules_registry(
-      new WebRequestRulesRegistry(browser_context_,
-                                  web_request_cache_delegate,
-                                  webview_key));
+      new WebRequestRulesRegistry(browser_context_, web_request_cache_delegate,
+                                  rules_registry_id));
 
   RegisterRulesRegistry(web_request_rules_registry);
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&RegisterToExtensionWebRequestEventRouterOnIO,
-          browser_context_, webview_key, web_request_rules_registry));
+                 browser_context_, rules_registry_id,
+                 web_request_rules_registry));
 
   // Only create a ContentRulesRegistry for regular pages and not webviews.

[Fady Samuel, 2014/11/28 11:34:24]

Get rid of the reference to <webview>.

[Xi Han, 2014/11/28 16:12:56]

Done.

-  if (!IsWebView(webview_key)) {
+  if (!cache_rules) {

[Fady Samuel, 2014/11/28 11:34:24]

This is inverted. As above, simply do the following:

if (rules_registry_id == kDefaultRulesRegistryID)
...

[Xi Han, 2014/11/28 16:12:56]

Done.

     RulesCacheDelegate* content_rules_cache_delegate =
         new RulesCacheDelegate(false /*log_storage_init_delay*/);
     cache_delegates_.push_back(content_rules_cache_delegate);
     scoped_refptr<ContentRulesRegistry> content_rules_registry =
         ExtensionsAPIClient::Get()->CreateContentRulesRegistry(
             browser_context_, content_rules_cache_delegate);
     if (content_rules_registry.get() != nullptr) {
       RegisterRulesRegistry(content_rules_registry);
       content_rules_registry_ = content_rules_registry.get();
     }
   }
 }
 
 void RulesRegistryService::Shutdown() {
   // Release the references to all registries. This would happen soon during
   // destruction of |*this|, but we need the ExtensionWebRequestEventRouter to
   // be the last to reference the WebRequestRulesRegistry objects, so that
   // the posted task below causes their destruction on the IO thread, not on UI
   // where the destruction of |*this| takes place.
   // TODO(vabr): Remove once http://crbug.com/218451#c6 gets addressed.
   rule_registries_.clear();
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&RegisterToExtensionWebRequestEventRouterOnIO,
-          browser_context_, WebViewKey(0, 0),
-          scoped_refptr<WebRequestRulesRegistry>(NULL)));
+                 browser_context_,
+                 RulesRegistryService::kDefaultRulesRegistryID,
+                 scoped_refptr<WebRequestRulesRegistry>(NULL)));
 }
 
 static base::LazyInstance<BrowserContextKeyedAPIFactory<RulesRegistryService> >
     g_factory = LAZY_INSTANCE_INITIALIZER;
 
 // static
 BrowserContextKeyedAPIFactory<RulesRegistryService>*
 RulesRegistryService::GetFactoryInstance() {
   return g_factory.Pointer();
 }
 
 // static
 RulesRegistryService* RulesRegistryService::Get(
     content::BrowserContext* context) {
   return BrowserContextKeyedAPIFactory<RulesRegistryService>::Get(context);
 }
 
 void RulesRegistryService::RegisterRulesRegistry(
     scoped_refptr<RulesRegistry> rule_registry) {
   const std::string event_name(rule_registry->event_name());
-  RulesRegistryKey key(event_name, rule_registry->webview_key());
+  RulesRegistryKey key(event_name, rule_registry->id());
   DCHECK(rule_registries_.find(key) == rule_registries_.end());
   rule_registries_[key] = rule_registry;
 }
 
 scoped_refptr<RulesRegistry> RulesRegistryService::GetRulesRegistry(
-    const WebViewKey& webview_key,
+    int rules_registry_id,
+    bool cache_rules,

[Fady Samuel, 2014/11/28 11:34:24]

We can probably get rid of this.

[Xi Han, 2014/11/28 16:12:56]

Done.

     const std::string& event_name) {
-  EnsureDefaultRulesRegistriesRegistered(webview_key);
+  EnsureDefaultRulesRegistriesRegistered(rules_registry_id, cache_rules);
 
-  RulesRegistryKey key(event_name, webview_key);
+  RulesRegistryKey key(event_name, rules_registry_id);
   RulesRegistryMap::const_iterator i = rule_registries_.find(key);
   if (i == rule_registries_.end())
     return scoped_refptr<RulesRegistry>();
   return i->second;
 }
 
-void RulesRegistryService::RemoveWebViewRulesRegistries(int process_id) {
-  DCHECK_NE(0, process_id);
-
-  std::set<RulesRegistryKey> registries_to_delete;
+void RulesRegistryService::RemoveRulesRegistryID(int rules_registry_id) {
   for (RulesRegistryMap::iterator it = rule_registries_.begin();
        it != rule_registries_.end(); ++it) {
-    const RulesRegistryKey& key = it->first;
-    const WebViewKey& webview_key = key.webview_key;
-    int embedder_process_id = webview_key.embedder_process_id;
-    // |process_id| will always be non-zero.
-    // |embedder_process_id| will only be non-zero if the key corresponds to a
-    // webview registry.
-    // Thus, |embedder_process_id| == |process_id| ==> the process ID is a
-    // webview embedder.
-    if (embedder_process_id != process_id)
+    if (it->first.rules_registry_id != rules_registry_id)
       continue;
-
-    // Modifying the container while iterating is bad so we'll save the keys we
-    // wish to delete in another container, and delete them in another loop.
-    registries_to_delete.insert(key);
-  }
-  for (std::set<RulesRegistryKey>::iterator it = registries_to_delete.begin();
-       it != registries_to_delete.end(); ++it) {
-    rule_registries_.erase(*it);
+    rule_registries_.erase(it);

[Fady Samuel, 2014/11/28 11:41:31]

Modifying a container while iterating over it can lead to badness. I would
restore "registries_to_delete".

[Xi Han, 2014/11/28 16:12:56]

I just realize that we might need to delete multiple items rather than one. It
is safe to have the registries_to_delete set. Restored. 

+    return;
   }
 }
 
 void RulesRegistryService::SimulateExtensionUninstalled(
     const std::string& extension_id) {
   NotifyRegistriesHelper(&RulesRegistry::OnExtensionUninstalled, extension_id);
 }
 
 void RulesRegistryService::NotifyRegistriesHelper(
     void (RulesRegistry::*notification_callback)(const std::string&),
@@ skipping 26 matching lines @@
 }
 
 void RulesRegistryService::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     extensions::UninstallReason reason) {
   NotifyRegistriesHelper(&RulesRegistry::OnExtensionUninstalled,
                          extension->id());
 }
 
-void RulesRegistryService::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(content::NOTIFICATION_RENDERER_PROCESS_TERMINATED, type);
-
-  content::RenderProcessHost* process =
-      content::Source<content::RenderProcessHost>(source).ptr();
-  RemoveWebViewRulesRegistries(process->GetID());
-}
-
 }  // namespace extensions