Declarative content scripts: Browser-side: per-extension shared memory regions (lazily loaded)

chrome/browser/extensions/shared_user_script_master.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 "chrome/browser/extensions/shared_user_script_master.h"
+
+#include <set>

[Devlin, 2014/08/04 18:33:24]

This is already included in the .h

[Mark Dittmer, 2014/08/05 20:33:18]

Got it. I thought that "it's okay to depend on [same name].h includes" only
applied to our source, but that's not the case.

+
+#include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
+
+namespace extensions {
+
+SharedUserScriptMaster::SharedUserScriptMaster(Profile* profile)
+    : loader_(profile,
+              "" /* owner_extension_id */,
+              true /* listen_for_extension_system_loaded */),
+      profile_(profile),
+      extension_registry_observer_(this) {
+  extension_registry_observer_.Add(ExtensionRegistry::Get(profile_));
+}
+
+SharedUserScriptMaster::~SharedUserScriptMaster() {
+}
+
+void SharedUserScriptMaster::OnExtensionLoaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension) {
+  loader_.AddScripts(GetScriptsMetadata(extension));
+}
+
+void SharedUserScriptMaster::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    UnloadedExtensionInfo::Reason reason) {
+  loader_.RemoveScripts(GetScriptsMetadata(extension));
+}
+
+const std::set<UserScript>& SharedUserScriptMaster::GetScriptsMetadata(
+    const Extension* extension) {
+  const ExtensionId& id = extension->id();
+  if (scripts_metadata_.find(id) != scripts_metadata_.end())

[Devlin, 2014/08/04 18:33:24]

this is a double-lookup.  Prefer:
map::const_iterator iter = metadata_.find(id);
if (iter != metadata_.end())
  return iter->second;

[Mark Dittmer, 2014/08/05 20:33:18]

Done.

+    return scripts_metadata_[id];
+
+  scripts_metadata_[id] = std::set<UserScript>();
+  bool incognito_enabled = util::IsIncognitoEnabled(id, profile_);
+  const UserScriptList& fetched_scripts =
+      ContentScriptsInfo::GetContentScripts(extension);
+  std::set<UserScript>& stored_scripts = scripts_metadata_[id];

[Devlin, 2014/08/04 18:33:24]

again, another double lookup here.  Let's initialize the set first, and then add
it to the map.

[Mark Dittmer, 2014/08/05 20:33:18]

I'm not sure how this is a double-lookup. In order to return a ref to the right
object, we need one operation to store a set (line 46), and one line to lookup a
ref to the stored set (line 50). Do we gain anything by doing those back-to-back
-- i.e., putting user_script_metadata_[id] = ...; return
user_script_metadata_[id] at the end of the method?

[Devlin, 2014/08/05 21:47:26]

sorry, phrased that incorrectly.  It's only a double lookup because we set it on
46 (which involves a "lookup" to find out where to put it), and then look it up
on line 50.  This can be marginally better by using insert() and using the
iterator returned to avoid the second lookup, but then you need to do make a
std::pair for insertion and...meh.  This is fine. :)

[Mark Dittmer, 2014/08/06 15:32:24]

Acknowledged.

+  for (UserScriptList::const_iterator it = fetched_scripts.begin();
+       it != fetched_scripts.end();
+       ++it) {
+    UserScript script = *it;
+    script.set_incognito_enabled(incognito_enabled);
+    stored_scripts.insert(script);
+  }
+
+  return stored_scripts;
+}
+
+}  // namespace extensions