Refactoring: de-couple Extensions from "script injection System" [render side] : 1

extensions/browser/guest_view/web_view/web_view_guest.cc

Index: extensions/browser/guest_view/web_view/web_view_guest.cc
diff --git a/extensions/browser/guest_view/web_view/web_view_guest.cc b/extensions/browser/guest_view/web_view/web_view_guest.cc
index 717770fe44d40121aa73d60011e47262f7f34473..95b13210353881b3d71a784f17f79368be852ec4 100644
--- a/extensions/browser/guest_view/web_view/web_view_guest.cc
+++ b/extensions/browser/guest_view/web_view/web_view_guest.cc
@@ -4,6 +4,7 @@
 
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
 
+#include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -59,6 +60,17 @@ namespace extensions {
 
 namespace {
 
+using GetNextUniqueIDFunction = base::Callback<int(void)>;
+
+using WebViewKey = std::pair<int, int>;
+using WebViewKeyToIDMap = std::map<WebViewKey, int>;
+base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map =
+    LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map =
+     LAZY_INSTANCE_INITIALIZER;
+
+int current_script_injection_instance_id = 0;
+
 std::string WindowOpenDispositionToString(
   WindowOpenDisposition window_open_disposition) {
   switch (window_open_disposition) {
@@ -100,6 +112,28 @@ static std::string TerminationStatusToString(base::TerminationStatus status) {
   return "unknown";
 }
 
+int GetNextScriptInjectionInstanceID() {
+  return ++current_script_injection_instance_id;
+}
+
+int GetOrGenerateUniqueID(int embedder_process_id,
+                          int guest_instance_id,
+                          WebViewKeyToIDMap& id_map,
+                          int default_id,
+                          const GetNextUniqueIDFunction& function) {

[Fady Samuel, 2015/02/10 06:04:12]

This is really weird. I don't know if I like this approach. You could end up
with colliding IDs...

[Fady Samuel, 2015/02/10 15:46:47]

Nevermind, I didn't notice that you're passing in the map. This style is a
little confusing to read.

[Devlin, 2015/02/10 20:59:53]

I agree - it's a little over-verbose and hard to parse, particularly with the
bind statements in the callers (it's probably bad that to call this function
uses almost as many lines of code as the function itself ;)).

How about simplifying this to something like:
bool GetIdFromMap(const WebViewKey& key,
                  const WebViewKeyToIDMap& map,
                  int default_id,
                  int* result) {
  WebViewKeyToIDMap::const_iterator iter = map.end();
  if (!key.first || !key.second)
    *result = default_id;
  else if ((iter = map.find(key)) != map.end())
    *result = iter->second;
  else
    return false;
  return true;
}

and then, e.g.,

int GetOrGenerateScriptInjection() {
  WebViewKey key(embedder_process_id, webview_instance_id);
  WebViewKeyToIDMap& map = script_injection_instance_id_map.Get();
  int id = 0;
  if (!GetIdFromMap(key, map, HostID::kDefaultInstanceId, &id)) {
    id = ++current_script_injection_instance_id;
    map[key] = id;
  }
  return id;
}

[Xi Han, 2015/02/10 23:35:14]

I am happy to remove the bind statement as well. This solution sounds good to
me.

+  if (!(embedder_process_id && guest_instance_id))
+    return default_id;
+
+  WebViewKey key = std::make_pair(embedder_process_id, guest_instance_id);
+  auto it = id_map.find(key);
+  if (it != id_map.end())
+    return it->second;
+
+  int instance_id = function.Run();
+  id_map[key] = instance_id;
+  return instance_id;
+}
+
 std::string GetStoragePartitionIdFromSiteURL(const GURL& site_url) {
   const std::string& partition_id = site_url.query();
   bool persist_storage = site_url.path().find("persist") != std::string::npos;
@@ -181,30 +215,32 @@ bool WebViewGuest::GetGuestPartitionConfigForSite(
 // static
 const char WebViewGuest::Type[] = "webview";
 
-using WebViewKey = std::pair<int, int>;
-using WebViewKeyToIDMap = std::map<WebViewKey, int>;
-static base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map =
-    LAZY_INSTANCE_INITIALIZER;
-
 // static
 int WebViewGuest::GetOrGenerateRulesRegistryID(
     int embedder_process_id,
     int webview_instance_id) {
-  bool is_web_view = embedder_process_id && webview_instance_id;
-  if (!is_web_view)
-    return RulesRegistryService::kDefaultRulesRegistryID;
-
-  WebViewKey key = std::make_pair(embedder_process_id, webview_instance_id);
-  auto it = web_view_key_to_id_map.Get().find(key);
-  if (it != web_view_key_to_id_map.Get().end())
-    return it->second;
+  return GetOrGenerateUniqueID(
+      embedder_process_id,
+      webview_instance_id,
+      web_view_key_to_id_map.Get(),
+      RulesRegistryService::kDefaultRulesRegistryID,
+      base::Bind(&RulesRegistryService::GetNextRulesRegistryID,
+                 base::Unretained(
+                     RulesRegistryService::Get(
+                         content::RenderProcessHost::FromID(
+                             embedder_process_id)->GetBrowserContext()))));
+}
 
-  auto rph = content::RenderProcessHost::FromID(embedder_process_id);
-  int rules_registry_id =
-      RulesRegistryService::Get(rph->GetBrowserContext())->
-          GetNextRulesRegistryID();
-  web_view_key_to_id_map.Get()[key] = rules_registry_id;
-  return rules_registry_id;
+// static
+int WebViewGuest::GetOrGenerateScriptInjectionInstanceID(
+    int embedder_process_id,
+    int guest_instance_id ) {
+  return GetOrGenerateUniqueID(
+      embedder_process_id,
+      guest_instance_id,
+      script_injection_instance_id_map.Get(),
+      HostID::kDefaultInstanceId,
+      base::Bind(&GetNextScriptInjectionInstanceID));
 }
 
 // static
@@ -294,6 +330,10 @@ void WebViewGuest::DidInitialize(const base::DictionaryValue& create_params) {
       owner_web_contents()->GetRenderProcessHost()->GetID(),
       view_instance_id());
 
+  GetOrGenerateScriptInjectionInstanceID(
+      owner_web_contents()->GetRenderProcessHost()->GetID(),
+      guest_instance_id());
+
   // We must install the mapping from guests to WebViews prior to resuming
   // suspended resource loads so that the WebRequest API will catch resource
   // requests.
@@ -321,6 +361,7 @@ void WebViewGuest::EmbedderWillBeDestroyed() {
   WebViewKey key(owner_web_contents()->GetRenderProcessHost()->GetID(),
                  view_instance_id());
   web_view_key_to_id_map.Get().erase(key);
+  script_injection_instance_id_map.Get().erase(key);
 
   content::BrowserThread::PostTask(
       content::BrowserThread::IO,