Implement <webview>.addContentScript/removeContentScript API [1]

extensions/browser/guest_view/web_view/web_view_content_script_manager.cc

Index: extensions/browser/guest_view/web_view/web_view_content_script_manager.cc
diff --git a/extensions/browser/guest_view/web_view/web_view_content_script_manager.cc b/extensions/browser/guest_view/web_view/web_view_content_script_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..17b8b58cf31a53af95bceb53df5ad6cf4ebcd7ec
--- /dev/null
+++ b/extensions/browser/guest_view/web_view/web_view_content_script_manager.cc
@@ -0,0 +1,273 @@
+// Copyright 2015 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_content_script_manager.h"
+
+#include "base/lazy_instance.h"
+#include "base/memory/linked_ptr.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_details.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "extensions/browser/declarative_user_script_manager.h"
+#include "extensions/browser/declarative_user_script_master.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/browser/guest_view/guest_view_manager.h"
+#include "extensions/browser/guest_view/web_view/web_view_constants.h"
+#include "extensions/browser/guest_view/web_view/web_view_guest.h"
+#include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
+
+using content::BrowserThread;
+
+namespace {

[Devlin, 2015/04/13 19:21:48]

nit: This is long enough that there should be newlines around the namespace { }
and it should end in }  // namespace.

[Xi Han, 2015/04/14 19:05:51]

Done.

+bool RemoveContentScriptsForGuest(content::BrowserContext* browser_context,
+                                  HostID host_id,

[Devlin, 2015/04/13 19:21:48]

const &

[Xi Han, 2015/04/14 19:05:51]

Done.

+                                  content::WebContents* embedder_web_contents,
+                                  content::WebContents* guest_web_contents) {
+  auto guest = extensions::WebViewGuest::FromWebContents(guest_web_contents);
+  if (!guest)
+    return false;
+
+  extensions::WebViewContentScriptManager* manager =
+      extensions::WebViewContentScriptManager::Get(browser_context);
+  if (!manager)

[Devlin, 2015/04/13 19:21:48]

We always pass in the same BrowserContext, and we do it from the manager - so
how could this fail?  That said, we might actually be able to get rid of this. 
See below.

[Xi Han, 2015/04/14 19:05:51]

Use a DCHECK instead.

+    return false;
+
+  manager->RemoveContentScripts(embedder_web_contents,
+                                guest->view_instance_id(), host_id,
+                                std::vector<std::string>());
+  return false;
+}
+}
+
+namespace extensions {
+
+// This observer ensures that the content scripts added by the guest are removed
+// when its embedder goes away.
+class WebViewContentScriptManager::OwnerWebContentsObserver

[Devlin, 2015/04/13 19:21:48]

Document lifetime.

[Xi Han, 2015/04/14 19:05:51]

Done.

+    : public content::WebContentsObserver {
+ public:
+  OwnerWebContentsObserver(content::WebContents* embedder_web_contents,
+                           const HostID& host_id,
+                           WebViewContentScriptManager* manager)
+      : WebContentsObserver(embedder_web_contents),
+        host_id_(host_id),
+        web_view_content_script_manager_(manager) {}
+  ~OwnerWebContentsObserver() override {}
+
+  // WebContentsObserver:
+  void WebContentsDestroyed() override {
+    // If the embedder is destroyed then remove all the content scripts of the
+    // guest.
+    RemoveContentScripts();
+  }
+  void DidNavigateMainFrame(
+      const content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& params) override {
+    // If the embedder navigates to a different page then destroy the guest.
+    if (details.is_navigation_to_different_page())
+      RemoveContentScripts();
+  }
+  void RenderProcessGone(base::TerminationStatus status) override {
+    // If the embedder crashes, then destroy the guest.
+    RemoveContentScripts();
+  }
+
+ private:
+  void RemoveContentScripts() {
+    web_view_content_script_manager_->RemoveContentScripts(web_contents(),

[Devlin, 2015/04/13 19:21:47]

Document that this object can be deleted after this line.

[Xi Han, 2015/04/14 19:05:51]

Done.

+                                                           host_id_);
+  }
+
+  HostID host_id_;
+  WebViewContentScriptManager* web_view_content_script_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(OwnerWebContentsObserver);
+};
+
+WebViewContentScriptManager::WebViewContentScriptManager(
+    content::BrowserContext* browser_context)
+    : browser_context_(browser_context) {
+}
+
+WebViewContentScriptManager::~WebViewContentScriptManager() {
+}
+
+WebViewContentScriptManager* WebViewContentScriptManager::Get(
+    content::BrowserContext* browser_context) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  WebViewContentScriptManager* manager =
+      static_cast<WebViewContentScriptManager*>(browser_context->GetUserData(
+          webview::kWebViewContentScriptManagerKeyName));
+  if (!manager) {
+    manager = new WebViewContentScriptManager(browser_context);
+    browser_context->SetUserData(webview::kWebViewContentScriptManagerKeyName,
+                                 manager);
+  }
+  return manager;
+}
+
+void WebViewContentScriptManager::AddContentScripts(
+    content::WebContents* embedder_web_contents,
+    int view_instance_id,
+    const HostID& host_id,
+    const std::map<std::string, UserScript>& scripts) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(embedder_web_contents);
+
+  DeclarativeUserScriptMaster* master =
+      ExtensionSystem::Get(browser_context_)
+          ->declarative_user_script_manager()
+          ->GetDeclarativeUserScriptMasterByID(host_id);
+  DCHECK(master);
+
+  // We need to update WebViewRenderState in the IO thread if the guest exists.
+  std::set<int> ids_to_add;
+
+  int embedder_process_id =
+      embedder_web_contents->GetRenderProcessHost()->GetID();
+  GuestMapKey key = std::pair<int, int>(embedder_process_id, view_instance_id);
+  GuestContentScriptMap::iterator iter = guest_content_script_map_.find(key);
+
+  // Step 1: finds the entry in guest_content_script_map_ by the given |key|.
+  // If there isn't any content script added for the given guest yet, insert an
+  // empty map first.
+  if (iter == guest_content_script_map_.end()) {
+    iter = guest_content_script_map_.insert(
+        iter, std::pair<GuestMapKey, ContentScriptMap>(
+                  key, std::map<std::string, UserScript>()));

[Devlin, 2015/04/13 19:21:48]

s/std::map<std::string, UserScript>()/ContentScriptMap()

[Xi Han, 2015/04/14 19:05:52]

Done.

+  }
+
+  // Step 2: updates the guest_content_script_map_, and adds content scripts to
+  // the master.
+  ContentScriptMap& map = iter->second;
+  for (const std::pair<std::string, UserScript>& pair : scripts) {
+    auto map_iter = map.find(pair.first);
+    // If a content script has the same name as the new one, removes the old
+    // script first, and insert the new one.
+    if (map_iter != map.end()) {
+      master->RemoveScript(map_iter->second);

[Devlin, 2015/04/13 19:21:48]

We do an AttemptLoad() in UserScriptLoader after each call to
Remove/AddScript().  I think we should expose a batch method on the
UserScriptMaster to make this more efficient.

[Xi Han, 2015/04/14 19:05:51]

Added. 

+      map.erase(map_iter);
+    }
+    map.insert(pair);
+    ids_to_add.insert(pair.second.id());
+    master->AddScript(pair.second);
+  }
+
+  // Step 3: creates owner web contents observer for the given
+  // |embedder_web_contents| if it doesn't exist.
+  auto observers_map_iter =
+      owner_web_contents_observer_map_.find(embedder_web_contents);
+  if (observers_map_iter == owner_web_contents_observer_map_.end()) {

[Devlin, 2015/04/13 19:21:48]

no reason to cache observers_map_iter, so just inline it in the if.

[Xi Han, 2015/04/14 19:05:51]

Good catch, updated.

+    linked_ptr<OwnerWebContentsObserver> observer(
+        new OwnerWebContentsObserver(embedder_web_contents, host_id, this));
+    owner_web_contents_observer_map_[embedder_web_contents] = observer;

[Devlin, 2015/04/13 19:21:48]

(See below to make sense of this)
observer->add_instance_id(view_instance_id);

[Xi Han, 2015/04/14 19:05:51]

Done.

+  }
+
+  // Step 4: updates WebViewRenderState in the IO thread.
+  if (!ids_to_add.empty()) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&WebViewRendererState::AddContentScriptIDs,
+                   base::Unretained(WebViewRendererState::GetInstance()),

[Devlin, 2015/04/13 19:21:48]

document why this unretained is safe.

[Xi Han, 2015/04/14 19:05:51]

Done.

+                   embedder_process_id, view_instance_id, ids_to_add));
+  }
+}
+
+void WebViewContentScriptManager::RemoveContentScripts(
+    content::WebContents* embedder_web_contents,
+    int view_instance_id,
+    const HostID& host_id,
+    const std::vector<std::string>& script_name_list) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  int embedder_process_id =
+      embedder_web_contents->GetRenderProcessHost()->GetID();
+  GuestMapKey key = std::pair<int, int>(embedder_process_id, view_instance_id);
+  GuestContentScriptMap::iterator script_map_iter =
+      guest_content_script_map_.find(key);
+  if (script_map_iter == guest_content_script_map_.end())
+    return;
+
+  DeclarativeUserScriptMaster* master =
+      ExtensionSystem::Get(browser_context_)
+          ->declarative_user_script_manager()
+          ->GetDeclarativeUserScriptMasterByID(host_id);
+  CHECK(master);
+
+  // We need to update WebViewRenderState in the IO thread if the guest exists.
+  std::set<int> ids_to_delete;
+
+  std::vector<std::string> names_to_delete;
+
+  // Step 1: removed content scripts from |master| and updates
+  // |guest_content_script_map_|
+  std::map<std::string, UserScript>& map = script_map_iter->second;
+  // If the |script_name_list| is empty, all the content scripts added by the
+  // guest will be removed; otherwise, removes the scripts in the
+  // |script_name_list|.
+  if (script_name_list.empty()) {
+    for (const std::pair<std::string, UserScript>& iter : map)
+      names_to_delete.push_back(iter.first);
+  } else {
+    names_to_delete = script_name_list;

[Devlin, 2015/04/13 19:21:48]

this is a potentially expensive copy.  Use std::vector::swap instead.

[Xi Han, 2015/04/14 19:05:51]

The disadvantage is that we have to pass in std::vector<std::string>, rather
than const std::vector<std::string>& for script_name_list. I still prefer
passing in a const &.

[Devlin, 2015/04/14 23:04:57]

To get around that:
std::vector<std::string> generated_list;
const std::vector<std::string>* names_to_delete = nullptr;
if (script_name_list.empty()) {
  for (const std::pair<std::string, UserScript>& iter : map)
    generated_list.push_back(iter.first);
  names_to_delete = &generated_list;
} else {
  names_to_delete = &script_name_list;
}

Though actually, since you're just going to then turn around and look up all the
names, perhaps the better thing to do is:

if (script_name_list.empty()) {
  for (const std::pair<std::string, UserScript>& iter : map) {
    scripts_to_delete.insert(iter->second);
    ids_to_delete.insert(iter->second.id());
  }
  map.clear();
} else {
  for (const std::string& name : script_name_list) {
    ContentScriptMap::iterator iter = map.find(name);
    if (iter == map.end())
      continue;
    scripts_to_delete.insert(iter->second);
    ids_to_delete.insert(iter->second.id());
    map.erase(iter);
  }
}

Though it has a bit of duplicate code, it saves you some duplicate map lookups,
and is actually not many (if any) more lines than what you have now.

[Xi Han, 2015/04/15 19:43:55]

The second suggestion was just my previous implementation. Revert back.

+  }
+
+  for (const std::string& name : names_to_delete) {
+    ContentScriptMap::iterator iter = map.find(name);
+    if (iter == map.end())
+      continue;
+    const UserScript& script = iter->second;
+    ids_to_delete.insert(script.id());
+    master->RemoveScript(script);
+    map.erase(iter);
+  }
+
+  // Step 2: Update WebViewRenderState in the IO thread.
+  if (!ids_to_delete.empty()) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&WebViewRendererState::RemoveContentScriptIDs,
+                   base::Unretained(WebViewRendererState::GetInstance()),
+                   embedder_process_id, view_instance_id, ids_to_delete));
+  }
+}
+
+void WebViewContentScriptManager::RemoveContentScripts(
+    content::WebContents* embedder_web_contents,
+    const HostID& host_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Step 1: removes content scripts of all the guests embedded.

[Devlin, 2015/04/13 19:21:48]

Why not instead store a std::set of view_instance_ids on the
OwnerWebContentsObservers, and then pass that in the RemoveContentScripts
method.  That way, you can just loop over that, instead of having to do multiple
lookups.

[Xi Han, 2015/04/14 19:05:52]

This is a great idea, the removal becomes much easier.

+  auto guest_view_manager =
+      GuestViewManager::FromBrowserContext(browser_context_);
+  guest_view_manager->ForEachGuest(
+      embedder_web_contents,
+      base::Bind(&RemoveContentScriptsForGuest, browser_context_, host_id,
+                 embedder_web_contents));
+
+  // Step 2: removes the observer of the given |embedder_web_contents|.
+  owner_web_contents_observer_map_.erase(embedder_web_contents);
+}
+
+std::set<int> WebViewContentScriptManager::GetContentScriptIDSet(
+    int embedder_process_id,
+    int view_instance_id) {
+  std::set<int> ids;
+
+  GuestMapKey key = std::pair<int, int>(embedder_process_id, view_instance_id);
+  GuestContentScriptMap::const_iterator iter =
+      guest_content_script_map_.find(key);
+  if (iter == guest_content_script_map_.end())
+    return ids;
+  const ContentScriptMap& map = iter->second;
+  for (const auto& pair : map)
+    ids.insert(pair.second.id());
+
+  return ids;
+}
+
+}  // namespace extensions