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

extensions/browser/guest_view/web_view/web_view_guest.cc

 // Copyright 2014 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/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"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 35 matching lines @@
 
 using base::UserMetricsAction;
 using content::RenderFrameHost;
 using content::ResourceType;
 using content::WebContents;
 
 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) {
     case IGNORE_ACTION:
       return "ignore";
     case SAVE_TO_DISK:
       return "save_to_disk";
     case CURRENT_TAB:
       return "current_tab";
     case NEW_BACKGROUND_TAB:
@@ skipping 21 matching lines @@
       return "killed";
     case base::TERMINATION_STATUS_PROCESS_CRASHED:
       return "crashed";
     case base::TERMINATION_STATUS_MAX_ENUM:
       break;
   }
   NOTREACHED() << "Unknown Termination 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;
   return (persist_storage ? webview::kPersistPrefix : "") + partition_id;
 }
 
 void ParsePartitionParam(const base::DictionaryValue& create_params,
                          std::string* storage_partition_id,
                          bool* persist_storage) {
   std::string partition_str;
@@ skipping 61 matching lines @@
   // The partition name is user supplied value, which we have encoded when the
   // URL was created, so it needs to be decoded.
   *partition_name =
       net::UnescapeURLComponent(site.query(), net::UnescapeRule::NORMAL);
   return true;
 }
 
 // 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;
-
-  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;
+  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()))));
 }
 
 // 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
 int WebViewGuest::GetViewInstanceId(WebContents* contents) {
   auto guest = FromWebContents(contents);
   if (!guest)
     return guestview::kInstanceIDNone;
 
   return guest->view_instance_id();
 }
 
 bool WebViewGuest::CanRunInDetachedState() const {
   return true;
@@ skipping 66 matching lines @@
                               content::Source<WebContents>(web_contents()));
 
   if (web_view_guest_delegate_)
     web_view_guest_delegate_->OnDidInitialize();
   AttachWebViewHelpers(web_contents());
 
   rules_registry_id_ = GetOrGenerateRulesRegistryID(
       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.
   PushWebViewStateToIOThread();
 
   ApplyAttributes(create_params);
 }
 
 void WebViewGuest::AttachWebViewHelpers(WebContents* contents) {
   if (web_view_guest_delegate_)
     web_view_guest_delegate_->OnAttachWebViewHelpers(contents);
   web_view_permission_helper_.reset(new WebViewPermissionHelper(this));
 }
 
 void WebViewGuest::DidStopLoading() {
   scoped_ptr<base::DictionaryValue> args(new base::DictionaryValue());
   DispatchEventToView(
       new GuestViewBase::Event(webview::kEventLoadStop, args.Pass()));
 }
 
 void WebViewGuest::EmbedderWillBeDestroyed() {
   // Clean up rules registries for the webview.
   RulesRegistryService::Get(browser_context())
       ->RemoveRulesRegistriesByID(rules_registry_id_);
   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,
       FROM_HERE,
       base::Bind(
           &RemoveWebViewEventListenersOnIOThread,
           browser_context(),
           owner_extension_id(),
           owner_web_contents()->GetRenderProcessHost()->GetID(),
           view_instance_id()));
@@ skipping 881 matching lines @@
       WebViewGuest::From(owner_web_contents()->GetRenderProcessHost()->GetID(),
                          new_window_instance_id);
   if (!guest)
     return;
 
   if (!allow)
     guest->Destroy();
 }
 
 }  // namespace extensions