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

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

BUG=377978
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289969

[Mark Dittmer, 2014-07-24 21:06:20]

Ken, would you please review the small changes to extensions/common?

Jeffrey, would you please review the changes to chrome/browser/extensions?

[kenrb, 2014-07-24 21:09:04]

On 2014/07/24 21:06:20, Mark Dittmer wrote:
> Ken, would you please review the small changes to extensions/common?
> 
> Jeffrey, would you please review the changes to chrome/browser/extensions?

I would like to help, but I'm not an owner for extensions/common except for the
IPC-specific files (*_messages_*).

[Mark Dittmer, 2014-07-25 14:08:57]

On 2014/07/24 21:09:04, kenrb wrote:
> On 2014/07/24 21:06:20, Mark Dittmer wrote:
> > Ken, would you please review the small changes to extensions/common?
> > 
> > Jeffrey, would you please review the changes to chrome/browser/extensions?
> 
> I would like to help, but I'm not an owner for extensions/common except for
the
> IPC-specific files (*_messages_*).

My mistake, Ken. Jeffrey can review the whole thing. He's an extensions
base-directory owner.

[Devlin, 2014-07-30 21:20:39]

Got about halfway through, but I think that the UserScriptMaster class design
can be a bit cleaner, so gonna wait on that before reviewing the rest in full
detail.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:22:
owner_extension_id_ = extension_id;
init this in the initialization list.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:35: switch (type) {
If you only expect one notification, it's cleaner to do:
DCHECK_EQ(NOTIFICATION_TYPE, type);

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:51: DCHECK(false);
We usually spell this NOTREACHED(). (though moot with above comment)

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:57: if
(is_loading()) {
if (IsSingleLineIfStatement())
  RemoveBrackets();

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:65: if
(scripts_to_add_.find(script) != scripts_to_add_.end()) {
brackets

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:65: if
(scripts_to_add_.find(script) != scripts_to_add_.end()) {
For sets, prefer:
if (scripts_to_add_.count(script) != 0)
...

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
nit: we don't use (c) anymore (I don't know why, but there's some complicated
legal reason ;))

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:10: #include
"base/scoped_observer.h"
unused?

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:28: virtual void
Observe(int type,
Let's make this private.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:44: // Start loading
scripts into shared memory region.
Replace this comment with just UserScriptMaster implementation.  We try to only
document these in one place so that if they ever change, we only have to update
one comment.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:49: void
ResetPendingLoad() {
This and ResetPendingLoadData should probably both be in the .cc (more style
than anything else, since they are both very simple).

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/shared_user_script_master.cc:85: SendUpdate(process,
(This comment is in a random spot)

To me, this whole class hierarchy seems a bit ugly.  In particular, it's very
difficult to tell what's each class is doing.  Can we clean this up?  In
particular, try to avoid the use of protected member variables and code that
looks a lot alike (for instance, the code here and in
DeclarativeUserScriptMaster::Observe is very similar).

If there's only a small change in the two methods (for instance, in StartLoad()
it looks like the biggest change is the scripts we pass), it's typically fairly
easy to just factor that small piece into a helper method, like
GetScriptsToLoad(), which each implementation can override, rather than the full
method.

This is pretty vague, so if you want more specifics on how we might be able to
clean this up, lemme know and we'll work it out.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/shared_user_script_master.h:12: #include
"extensions/browser/extension_registry.h"
Can forward declare this instead.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/shared_user_script_master.h:18: class BrowserContext;
namespace content {
class BrowserContext;
}

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/shared_user_script_master.h:26: class
SharedUserScriptMaster : public UserScriptMaster,
add class comments

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/shared_user_script_master.h:33: //
content::NotificationObserver implementation.
move overloaded methods to private.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/shared_user_script_master.h:46: virtual void
StartLoad() OVERRIDE;
UserScriptMaster implementation.

[Mark Dittmer, 2014-07-31 18:37:33]

Uploaded refactor of relationship between UserScriptMaster and its subclasses.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:22:
owner_extension_id_ = extension_id;
On 2014/07/30 21:20:38, Devlin wrote:
> init this in the initialization list.

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:35: switch (type) {
On 2014/07/30 21:20:38, Devlin wrote:
> If you only expect one notification, it's cleaner to do:
> DCHECK_EQ(NOTIFICATION_TYPE, type);

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:51: DCHECK(false);
On 2014/07/30 21:20:38, Devlin wrote:
> We usually spell this NOTREACHED(). (though moot with above comment)

Acknowledged.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:65: if
(scripts_to_add_.find(script) != scripts_to_add_.end()) {
On 2014/07/30 21:20:38, Devlin wrote:
> For sets, prefer:
> if (scripts_to_add_.count(script) != 0)
> ...

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.cc:99:
new_user_scripts.erase(scripts_to_remove_.begin(), scripts_to_remove_.end());
This is wrong because std::set::erase cannot take input iterators from another
collection. Fixed with iteration over scripts_to_remove_ and repeated calls to
erase(*scripts_to_remove_iterator).

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/07/30 21:20:38, Devlin wrote:
> nit: we don't use (c) anymore (I don't know why, but there's some complicated
> legal reason ;))

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:10: #include
"base/scoped_observer.h"
On 2014/07/30 21:20:38, Devlin wrote:
> unused?

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:28: virtual void
Observe(int type,
On 2014/07/30 21:20:38, Devlin wrote:
> Let's make this private.

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:44: // Start loading
scripts into shared memory region.
On 2014/07/30 21:20:38, Devlin wrote:
> Replace this comment with just UserScriptMaster implementation.  We try to
only
> document these in one place so that if they ever change, we only have to
update
> one comment.

Done.

https://codereview.chromium.org/420543002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_master.h:49: void
ResetPendingLoad() {
On 2014/07/30 21:20:38, Devlin wrote:
> This and ResetPendingLoadData should probably both be in the .cc (more style
> than anything else, since they are both very simple).

Done.

[Devlin, 2014-07-31 20:32:24]

Halfway through again.  The refactor already looks a ton better, but I think we
can actually make it better yet.

What if:
- We rename UserScriptMaster to UserScriptLoader.
- We make UserScriptLoader own the UserScripts again (my bad)
- We make UserScriptLoader have somewhat similar logic to
DeclarativeUserScriptMaster, in that it keeps track of individual scripts that
are added and removed.
- We put AddUserScript(s), RemoveUserScript(s), and StartLoad() methods on
UserScriptLoader.
Decl/SharedUserScriptMaster each contain a UserScriptLoader, and update it as
they see the relevant events (extension loaded, unloaded, scripts added/removed
declaratively).

WDYT?  It gives each class a (somewhat) more distinct purpose, and clears up the
crossover we have now between USM, DUSM, and SUSM.  But there might be a big
wrench in the design that I'm not seeing. ;)

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.cc:47: AttemptLoad();
Seems to me there's no reason to attempt the load if it was just removed from
the scripts_to_add_ list, right?  Similarly, no reason to reload if the script
is not present in user_scripts_?  And if clear_scripts_ is set to true, we want
to not add this to scripts_to_remove_?

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.cc:64:
new_user_scripts.insert(user_scripts_->begin(), user_scripts_->end());
If we wanted to be cool, we would do:
user_scripts_ = STLSetUnion(STLSetDifference(user_scripts_, scripts_to_remove_),
                            scripts_to_add_);

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:35: //
UserScriptMaster implementation.
make this private.

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:39: //
UserScriptMaster implementation.
Make all of these private.

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:52: // Reset
UserScriptMaster::pending_load_ based on our data.
Prefer: "Signal a pending load if any scripts have been added or removed" or
something like that.

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:19: 
namespace content {
class BrowserContext;
}

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:35: virtual void
OnExtensionLoaded(content::BrowserContext* browser_context,
private

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:46: // UserScriptMaster
implementation.
private

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:55: void
OnExtensionSystemReady();
comment

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master.cc:419: GetExtensionsInfo(),
Wasn't this passed into the function?

[Mark Dittmer, 2014-08-04 01:18:17]

Refactor UserScriptMaster into UserScriptLoader, managed by
SharedUserScriptMaster and DeclarativeUserScriptMaster. Also, using
std::set<UserScript> instead of UserScriptList for the most part. Unfortunately,
we already have a UserScriptSet, which is a renderer-side-only object that does
a lot more than just store a set of UserScripts.

[Devlin, 2014-08-04 18:33:27]

This is *much* cleaner!  Very nice!

We should also add tests, though.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.cc:9: #include
"chrome/browser/profiles/profile.h"
Do we need this?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:10: #include
"extensions/common/user_script.h"
Can just forward-declare UserScript here.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:16: class
DeclarativeUserScriptMaster {
class comments

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:511: return
user_script_master_manager_.get()
I might be missing it... but when are these created?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:44: // ^--- shared
Let's just move this above the sharedUserScriptMaster line. :)

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:7: #include <set>
This is already included in the .h

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:43: if
(scripts_metadata_.find(id) != scripts_metadata_.end())
this is a double-lookup.  Prefer:
map::const_iterator iter = metadata_.find(id);
if (iter != metadata_.end())
  return iter->second;

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:50: std::set<UserScript>&
stored_scripts = scripts_metadata_[id];
again, another double lookup here.  Let's initialize the set first, and then add
it to the map.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:12: #include
"content/public/browser/notification_observer.h"
don't need either of the notification_*.h

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:14: #include
"extensions/browser/extension_registry.h"
forward-declare ExtensionRegistry.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:19: namespace content {
nit:
namespace content {
class BrowserContext;
}

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:29: class
SharedUserScriptMaster : public ExtensionRegistryObserver {
class comments

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:35: virtual void
OnExtensionLoaded(content::BrowserContext* browser_context,
privatize.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:73:
->GetComponentExtensionResourceManager()
nit: I'm sure that this is just git cl format, but please avoid style changes
like this, because it just makes noise in the git/svn history.  Exceptions for
if it's an egregious style mistake.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:145:
const_cast<UserScript::File&>(script->js_scripts()[k]);
Why do we need the const cast?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:356: extension_system_ready_ =
false;
This was already set in the constructor.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:376: if
(removed_scripts_.count(*it) > 0)
We don't need these ifs - we won't remove a nonexistent element, and can't add
two of the same element.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:388: if
(added_scripts_.count(*it) > 0)
ditto

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:425: void
UserScriptLoader::AttemptLoad() {
Shouldn't we also check extension_system_ready_?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:436: // If scripts were marked
for clearing before adding+removal, then clear them.
adding and removing

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:441: for
(std::set<UserScript>::iterator it = removed_scripts_.begin();
Is there ever a case in which we need to remove scripts if |clear_scripts_| is
true?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:444: if
(user_scripts_->count(*it) != 0)
again, no reason for the if

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:552: if
(changed_extensions_.count(it->extension_id()) == 0)
no ifs

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:38: // has installed.  Lives on
the UI thread.
Update this comment to document the purpose of this vs. the ScriptMasters

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:49: explicit
UserScriptLoader(Profile* profile,
This no longer needs explicit :)

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:50: ExtensionId
owner_extension_id,
const ExtensionId&

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:58: bool ScriptsReady() const {
return shared_memory_.get() != NULL; }
Since all the callers of this are being updated anyway, let's make this
hacker_style().

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:60: void AddScripts(const
std::set<UserScript>& scripts);
comments

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:121: // Boolean that indicates
whether the the collection of scripts should be
Since this has type bool, I think we can just say "Indicates whether..."

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader_unittest.cc:119: "\n"
here again, let's avoid these small non-functional changes

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_master_manager.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.cc:23:
content::BrowserContext* browser_context,
Why?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.cc:32: if
(declarative_masters_.find(id) != declarative_masters_.end()) {
We could just have the DeclUSM listen for this.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_master_manager.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.h:20: class BrowserContext;
condense

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.h:28: class
UserScriptMasterManager : public ExtensionRegistryObserver {
So...Do we need this class?  Or could we just have ExtensionSystem hold the
UserScriptMasters?

https://codereview.chromium.org/420543002/diff/120001/extensions/common/user_...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/420543002/diff/120001/extensions/common/user_...
extensions/common/user_script.h:293: typedef std::vector<UserScript>
UserScriptList;
Delete

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:499:
std::make_pair(mojo::kValidatorModuleName, IDR_MOJO_VALIDATOR_JS));
??

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.h:44: virtual ~UserScriptSet();
?

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.h:64: bool
OnUpdateUserScripts(base::SharedMemoryHandle shared_memory,
?

[Mark Dittmer, 2014-08-05 20:33:20]

Addressed all the latest comments. Added many code comments, got rid of
UserScriptMasterManager, etc.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.cc:9: #include
"chrome/browser/profiles/profile.h"
On 2014/08/04 18:33:24, Devlin wrote:
> Do we need this?

s'pose not.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:10: #include
"extensions/common/user_script.h"
On 2014/08/04 18:33:24, Devlin wrote:
> Can just forward-declare UserScript here.

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:16: class
DeclarativeUserScriptMaster {
On 2014/08/04 18:33:24, Devlin wrote:
> class comments

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:322:
user_script_master_manager_.reset(new UserScriptMasterManager(profile_));
user_script_master_manager_ instantiated here.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:511: return
user_script_master_manager_.get()
On 2014/08/04 18:33:24, Devlin wrote:
> I might be missing it... but when are these created?

See comment on line 322.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:44: // ^--- shared
On 2014/08/04 18:33:24, Devlin wrote:
> Let's just move this above the sharedUserScriptMaster line. :)

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:7: #include <set>
On 2014/08/04 18:33:24, Devlin wrote:
> This is already included in the .h

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.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:43: if
(scripts_metadata_.find(id) != scripts_metadata_.end())
On 2014/08/04 18:33:24, Devlin wrote:
> this is a double-lookup.  Prefer:
> map::const_iterator iter = metadata_.find(id);
> if (iter != metadata_.end())
>   return iter->second;

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:50: std::set<UserScript>&
stored_scripts = scripts_metadata_[id];
On 2014/08/04 18:33:24, Devlin wrote:
> again, another double lookup here.  Let's initialize the set first, and then
add
> it to the map.

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?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:12: #include
"content/public/browser/notification_observer.h"
On 2014/08/04 18:33:24, Devlin wrote:
> don't need either of the notification_*.h

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:14: #include
"extensions/browser/extension_registry.h"
On 2014/08/04 18:33:24, Devlin wrote:
> forward-declare ExtensionRegistry.

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:19: namespace content {
On 2014/08/04 18:33:24, Devlin wrote:
> nit:
> namespace content {
> class BrowserContext;
> }

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:29: class
SharedUserScriptMaster : public ExtensionRegistryObserver {
On 2014/08/04 18:33:25, Devlin wrote:
> class comments

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:35: virtual void
OnExtensionLoaded(content::BrowserContext* browser_context,
On 2014/08/04 18:33:24, Devlin wrote:
> privatize.

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:73:
->GetComponentExtensionResourceManager()
On 2014/08/04 18:33:25, Devlin wrote:
> nit: I'm sure that this is just git cl format, but please avoid style changes
> like this, because it just makes noise in the git/svn history.  Exceptions for
> if it's an egregious style mistake.

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:145:
const_cast<UserScript::File&>(script->js_scripts()[k]);
On 2014/08/04 18:33:25, Devlin wrote:
> Why do we need the const cast?

std::set const'ifies its iterators because it's concerned about the user
changing data that is used for (in)equality operations. The const'ness of the
UserScript in the iterator bleeds out to the UserScript::File. In this case, we
are safe because this code path will never call set_id() on the UserScript in
the set; in fact, we only de-const the UserScript::File (which is not used for
(in)equality operations on UserScripts).

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:356: extension_system_ready_ =
false;
On 2014/08/04 18:33:25, Devlin wrote:
> This was already set in the constructor.

True. I was thinking that the code looks more explicit and consistent when it's
set in both the if and the else. Should I remove it from the initializer list,
or ignore my instincts and remove this line?

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:376: if
(removed_scripts_.count(*it) > 0)
On 2014/08/04 18:33:25, Devlin wrote:
> We don't need these ifs - we won't remove a nonexistent element, and can't add
> two of the same element.

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:388: if
(added_scripts_.count(*it) > 0)
On 2014/08/04 18:33:25, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:425: void
UserScriptLoader::AttemptLoad() {
On 2014/08/04 18:33:25, Devlin wrote:
> Shouldn't we also check extension_system_ready_?

Yes! Good catch.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:436: // If scripts were marked
for clearing before adding+removal, then clear them.
On 2014/08/04 18:33:25, Devlin wrote:
> adding and removing

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:441: for
(std::set<UserScript>::iterator it = removed_scripts_.begin();
On 2014/08/04 18:33:25, Devlin wrote:
> Is there ever a case in which we need to remove scripts if |clear_scripts_| is
> true?

No. Good point.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:444: if
(user_scripts_->count(*it) != 0)
On 2014/08/04 18:33:25, Devlin wrote:
> again, no reason for the if

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:552: if
(changed_extensions_.count(it->extension_id()) == 0)
On 2014/08/04 18:33:25, Devlin wrote:
> no ifs

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:38: // has installed.  Lives on
the UI thread.
On 2014/08/04 18:33:26, Devlin wrote:
> Update this comment to document the purpose of this vs. the ScriptMasters

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:49: explicit
UserScriptLoader(Profile* profile,
On 2014/08/04 18:33:25, Devlin wrote:
> This no longer needs explicit :)

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:50: ExtensionId
owner_extension_id,
On 2014/08/04 18:33:26, Devlin wrote:
> const ExtensionId&

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:58: bool ScriptsReady() const {
return shared_memory_.get() != NULL; }
On 2014/08/04 18:33:26, Devlin wrote:
> Since all the callers of this are being updated anyway, let's make this
> hacker_style().

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:60: void AddScripts(const
std::set<UserScript>& scripts);
On 2014/08/04 18:33:25, Devlin wrote:
> comments

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:121: // Boolean that indicates
whether the the collection of scripts should be
On 2014/08/04 18:33:25, Devlin wrote:
> Since this has type bool, I think we can just say "Indicates whether..."

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader_unittest.cc:119: "\n"
On 2014/08/04 18:33:26, Devlin wrote:
> here again, let's avoid these small non-functional changes

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_master_manager.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/08/04 18:33:26, Devlin wrote:
> no (c)

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.cc:23:
content::BrowserContext* browser_context,
On 2014/08/04 18:33:26, Devlin wrote:
> Why?

Good question. I erroneously presumed that OnExtension(Un)Loaded were pure
virtual, but they are not.

Removed.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.cc:32: if
(declarative_masters_.find(id) != declarative_masters_.end()) {
On 2014/08/04 18:33:26, Devlin wrote:
> We could just have the DeclUSM listen for this.

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_master_manager.h (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/08/04 18:33:26, Devlin wrote:
> no (c)

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.h:20: class BrowserContext;
On 2014/08/04 18:33:26, Devlin wrote:
> condense

Done.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_master_manager.h:28: class
UserScriptMasterManager : public ExtensionRegistryObserver {
On 2014/08/04 18:33:26, Devlin wrote:
> So...Do we need this class?  Or could we just have ExtensionSystem hold the
> UserScriptMasters?

You're right. We can get rid of this class. Done.

https://codereview.chromium.org/420543002/diff/120001/extensions/common/user_...
File extensions/common/user_script.h (right):

https://codereview.chromium.org/420543002/diff/120001/extensions/common/user_...
extensions/common/user_script.h:293: typedef std::vector<UserScript>
UserScriptList;
On 2014/08/04 18:33:26, Devlin wrote:
> Delete

Done.

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:499:
std::make_pair(mojo::kValidatorModuleName, IDR_MOJO_VALIDATOR_JS));
On 2014/08/04 18:33:26, Devlin wrote:
> ??

Merge mistake. Reverted.

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.h:44: virtual ~UserScriptSet();
On 2014/08/04 18:33:26, Devlin wrote:
> ?

Must have been an error during a merge. Reverted.

https://codereview.chromium.org/420543002/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.h:64: bool
OnUpdateUserScripts(base::SharedMemoryHandle shared_memory,
On 2014/08/04 18:33:26, Devlin wrote:
> ?

ditto

[Devlin, 2014-08-05 21:47:28]

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:50: std::set<UserScript>&
stored_scripts = scripts_metadata_[id];
On 2014/08/05 20:33:18, Mark Dittmer wrote:
> On 2014/08/04 18:33:24, Devlin wrote:
> > again, another double lookup here.  Let's initialize the set first, and then
> add
> > it to the map.
> 
> 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?

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. :)

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:145:
const_cast<UserScript::File&>(script->js_scripts()[k]);
On 2014/08/05 20:33:19, Mark Dittmer wrote:
> On 2014/08/04 18:33:25, Devlin wrote:
> > Why do we need the const cast?
> 
> std::set const'ifies its iterators because it's concerned about the user
> changing data that is used for (in)equality operations. The const'ness of the
> UserScript in the iterator bleeds out to the UserScript::File. In this case,
we
> are safe because this code path will never call set_id() on the UserScript in
> the set; in fact, we only de-const the UserScript::File (which is not used for
> (in)equality operations on UserScripts).

Ah.  Right you are.  Unfortunately, any kind of modification to the value while
iterating over a set results in undefined behavior (unless you were to make the
modified member mutable) [1].

This... is ugly.  The best solution I can think of is probably to convert
|user_scripts_| back to a vector.  This may make adding/removing marginally
slower, but not too bad given |user_scripts_| should be fairly small, and
addition/removal should be fairly rare.  If we do this, we should also convert
|removed_scripts_| and |added_scripts_| to sets of int64 to save some space.

Any other ideas?

[1] This comes from jyasskin@, C++ guru supreme.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:356: extension_system_ready_ =
false;
On 2014/08/05 20:33:19, Mark Dittmer wrote:
> On 2014/08/04 18:33:25, Devlin wrote:
> > This was already set in the constructor.
> 
> True. I was thinking that the code looks more explicit and consistent when
it's
> set in both the if and the else. Should I remove it from the initializer list,
> or ignore my instincts and remove this line?

Remove this line.  It's pretty common practice to set something to a default and
the modify as needed, and we shouldn't set it needlessly.  (And, of course,
uninitialized variables are a) scary and b) against style)  Sorry, instincts. ;)

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.cc:9: #include
"chrome/browser/profiles/profile.h"
don't think we actually need this.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:10: #include
"extensions/browser/extension_registry.h"
Just need a forward declare here.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:31: virtual void
OnExtensionUnloaded(
Privatize.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:53: // and notifying
renderers of scripts in shared memory.
s/scripts in shared memory/script updates

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:56: // Listen to
unloaded notifications.
Eh, not really a useful comment here (since we can see what methods it
overloads).  Let's just remove it.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:508: if
(!declarative_user_script_masters_.get(extension_id)) {
This will be moot if we make this a scoped vector, but it would be better to do:
DUSM* master = dusms_.get(extension_id);
if (!master)
  master = dusms_.set(extension_id, make_scoped_ptr(new DUSM(profile_,
extension_id)).second;
return master;

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:133:
base::ScopedPtrHashMap<ExtensionId, DeclarativeUserScriptMaster>
y'know, I don't really think this is the right choice here, since we're never
gonna have that many of these (at absolute most, what, 100?)  If you like light
reading, maybe peruse [1], but the bottom line is this should probably just be a
[scoped]vector.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sm...

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:134:
declarative_user_script_masters_;
indent

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:16: "" /*
owner_extension_id */,
nit: slightly prefer std::string()

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:36:
loader_.RemoveScripts(GetScriptsMetadata(extension));
Shouldn't we remove the scripts from scripts_metadata_ here?

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:14: #include
"chrome/browser/extensions/shared_user_script_master.h"
We don't actually need this, right?

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:437: // Remove scripts marked
for removal.
Let's lose this and the "Add scripts marked for addition" comment.  Should be
self-explanatory (and clear code is always better than clear comments, because
it's guaranteed to be current).

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:544: void
UserScriptLoader::ComputeChangedExtensions() {
It looks like this does away with the empty-set-means-all-extensions case.  Do
we need to update the IPC/renderer for this?

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:558: void
UserScriptLoader::UpdateExtensionsInfo() {
Do we need to worry about removing outdated extension infos?

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:60: base::SharedMemory*
GetSharedMemory() const { return shared_memory_.get(); }
Do we need this?

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:63: bool scripts_ready() const {
return shared_memory_.get() != NULL; }
nit: prefer accessors right above private:

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:71: // Clear the set of scripts
managed by this loader; i.e., make this set be
lose the i.e..  I think "clear the set" is suitably clear.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:93:
scoped_ptr<base::SharedMemory> shared_memory);
newline

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader_unittest.cc:95: "" /*
owner_extension_id */,
slightly prefer std::string()

[Mark Dittmer, 2014-08-06 15:32:26]

Addressed outstanding comments. Only question back is whether or not
UserScriptLoader should become an ExtensionRegistryObserver; see inline comment.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:50: std::set<UserScript>&
stored_scripts = scripts_metadata_[id];
On 2014/08/05 21:47:26, Devlin wrote:
> On 2014/08/05 20:33:18, Mark Dittmer wrote:
> > On 2014/08/04 18:33:24, Devlin wrote:
> > > again, another double lookup here.  Let's initialize the set first, and
then
> > add
> > > it to the map.
> > 
> > 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?
> 
> 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. :)

Acknowledged.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:145:
const_cast<UserScript::File&>(script->js_scripts()[k]);
On 2014/08/05 21:47:26, Devlin wrote:
> On 2014/08/05 20:33:19, Mark Dittmer wrote:
> > On 2014/08/04 18:33:25, Devlin wrote:
> > > Why do we need the const cast?
> > 
> > std::set const'ifies its iterators because it's concerned about the user
> > changing data that is used for (in)equality operations. The const'ness of
the
> > UserScript in the iterator bleeds out to the UserScript::File. In this case,
> we
> > are safe because this code path will never call set_id() on the UserScript
in
> > the set; in fact, we only de-const the UserScript::File (which is not used
for
> > (in)equality operations on UserScripts).
> 
> Ah.  Right you are.  Unfortunately, any kind of modification to the value
while
> iterating over a set results in undefined behavior (unless you were to make
the
> modified member mutable) [1].
> 
> This... is ugly.  The best solution I can think of is probably to convert
> |user_scripts_| back to a vector.  This may make adding/removing marginally
> slower, but not too bad given |user_scripts_| should be fairly small, and
> addition/removal should be fairly rare.  If we do this, we should also convert
> |removed_scripts_| and |added_scripts_| to sets of int64 to save some space.
> 
> Any other ideas?
> 
> [1] This comes from jyasskin@, C++ guru supreme.

I don't think we can get away with added/removed as int64s. User scripts that
arrive while the UI thread does not have ownership of user_scripts_ may contain
metadata that needs to be copied to user_scripts_ later.

For now, I've changed user_scripts_ to UserScriptList, but left the other
collections as std::set<UserScript>.

https://codereview.chromium.org/420543002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:356: extension_system_ready_ =
false;
On 2014/08/05 21:47:26, Devlin wrote:
> On 2014/08/05 20:33:19, Mark Dittmer wrote:
> > On 2014/08/04 18:33:25, Devlin wrote:
> > > This was already set in the constructor.
> > 
> > True. I was thinking that the code looks more explicit and consistent when
> it's
> > set in both the if and the else. Should I remove it from the initializer
list,
> > or ignore my instincts and remove this line?
> 
> Remove this line.  It's pretty common practice to set something to a default
and
> the modify as needed, and we shouldn't set it needlessly.  (And, of course,
> uninitialized variables are a) scary and b) against style)  Sorry, instincts.
;)

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.cc:9: #include
"chrome/browser/profiles/profile.h"
On 2014/08/05 21:47:26, Devlin wrote:
> don't think we actually need this.

Unfortunately, we do.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:10: #include
"extensions/browser/extension_registry.h"
On 2014/08/05 21:47:26, Devlin wrote:
> Just need a forward declare here.

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:31: virtual void
OnExtensionUnloaded(
On 2014/08/05 21:47:26, Devlin wrote:
> Privatize.

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:53: // and notifying
renderers of scripts in shared memory.
On 2014/08/05 21:47:26, Devlin wrote:
> s/scripts in shared memory/script updates

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:56: // Listen to
unloaded notifications.
On 2014/08/05 21:47:26, Devlin wrote:
> Eh, not really a useful comment here (since we can see what methods it
> overloads).  Let's just remove it.

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:508: if
(!declarative_user_script_masters_.get(extension_id)) {
On 2014/08/05 21:47:26, Devlin wrote:
> This will be moot if we make this a scoped vector, but it would be better to
do:
> DUSM* master = dusms_.get(extension_id);
> if (!master)
>   master = dusms_.set(extension_id, make_scoped_ptr(new DUSM(profile_,
> extension_id)).second;
> return master;

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:133:
base::ScopedPtrHashMap<ExtensionId, DeclarativeUserScriptMaster>
On 2014/08/05 21:47:27, Devlin wrote:
> y'know, I don't really think this is the right choice here, since we're never
> gonna have that many of these (at absolute most, what, 100?)  If you like
light
> reading, maybe peruse [1], but the bottom line is this should probably just be
a
> [scoped]vector.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sm...

Got it. SmallMap doesn't play nice with ScopedPtrHashMap anyway. Changed to
ScopedPtr; accessor iterates over vector.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:134:
declarative_user_script_masters_;
On 2014/08/05 21:47:26, Devlin wrote:
> indent

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:16: "" /*
owner_extension_id */,
On 2014/08/05 21:47:27, Devlin wrote:
> nit: slightly prefer std::string()

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:36:
loader_.RemoveScripts(GetScriptsMetadata(extension));
On 2014/08/05 21:47:27, Devlin wrote:
> Shouldn't we remove the scripts from scripts_metadata_ here?

Yes! Good catch. Otherwise, we won't reload potentially changed manifest data
when an extension is unloaded, then loaded.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:14: #include
"chrome/browser/extensions/shared_user_script_master.h"
On 2014/08/05 21:47:27, Devlin wrote:
> We don't actually need this, right?

Right.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:437: // Remove scripts marked
for removal.
On 2014/08/05 21:47:27, Devlin wrote:
> Let's lose this and the "Add scripts marked for addition" comment.  Should be
> self-explanatory (and clear code is always better than clear comments, because
> it's guaranteed to be current).

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:544: void
UserScriptLoader::ComputeChangedExtensions() {
On 2014/08/05 21:47:27, Devlin wrote:
> It looks like this does away with the empty-set-means-all-extensions case.  Do
> we need to update the IPC/renderer for this?

I don't think so. This does the same thing we did before. The case where we
deliberately sent an empty set still exists up in Observe().

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:558: void
UserScriptLoader::UpdateExtensionsInfo() {
On 2014/08/05 21:47:27, Devlin wrote:
> Do we need to worry about removing outdated extension infos?

Excellent point. I've implemented this with a public
ClearExtensionInfo(ExtensionId) that is invoked by the OnExtensionUnload of both
UserScriptMaster variants. Do you think this is a good solution, or should this
bookkeeping be the responsibility of the loader (in which case it should become
an ExtensionRegistryObserver).

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:60: base::SharedMemory*
GetSharedMemory() const { return shared_memory_.get(); }
On 2014/08/05 21:47:27, Devlin wrote:
> Do we need this?

Not really. Removed.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:63: bool scripts_ready() const {
return shared_memory_.get() != NULL; }
On 2014/08/05 21:47:27, Devlin wrote:
> nit: prefer accessors right above private:

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:71: // Clear the set of scripts
managed by this loader; i.e., make this set be
On 2014/08/05 21:47:27, Devlin wrote:
> lose the i.e..  I think "clear the set" is suitably clear.

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:93:
scoped_ptr<base::SharedMemory> shared_memory);
On 2014/08/05 21:47:27, Devlin wrote:
> newline

Done.

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader_unittest.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader_unittest.cc:95: "" /*
owner_extension_id */,
On 2014/08/05 21:47:27, Devlin wrote:
> slightly prefer std::string()

Done.

[Devlin, 2014-08-06 15:59:26]

Getting close!

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:558: void
UserScriptLoader::UpdateExtensionsInfo() {
On 2014/08/06 15:32:25, Mark Dittmer wrote:
> On 2014/08/05 21:47:27, Devlin wrote:
> > Do we need to worry about removing outdated extension infos?
> 
> Excellent point. I've implemented this with a public
> ClearExtensionInfo(ExtensionId) that is invoked by the OnExtensionUnload of
both
> UserScriptMaster variants. Do you think this is a good solution, or should
this
> bookkeeping be the responsibility of the loader (in which case it should
become
> an ExtensionRegistryObserver).

It should probably be the job of UserScriptLoader.  As mentioned before, making
something an observer is pretty cheap when just using the basic pattern, and
it's simpler/less fragile than saying "And be sure that this gets called when an
extension is unloaded".

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:508:
DeclarativeUserScriptMaster* master = NULL;
Is there any risk with initializing these before the ExtensionSystem is ready?

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:70: virtual
DeclarativeUserScriptMaster*
nit: This is shared too, right?

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:8: #include
"chrome/browser/profiles/profile.h"
Don't need this.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:49: scripts_metadata_[id]
= std::set<UserScript>();
I'm somewhat dubious of the benefit of storing all these scripts in memory.  We
only need them in loaded and unloaded, so it might be better to just fetch them
each time (and for unloaded, we don't need to go through the trouble of setting
the incognito bit).  But I'll leave it up to you.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:61: // Listen to extension
load, unloaded notifications.
nit: Kill this comment.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:17: #include
"chrome/browser/extensions/extension_util.h"
Don't need this.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:20: #include
"chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
or this.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:133: const std::set<UserScript>&
added_scripts,
This can be std::set<int64>, right?

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:451: added_scripts_.erase(*it);
Why are we removing from |added_scripts_| here, and not |user_scripts_|?

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:70: void ClearExtensionInfo(const
ExtensionId& extension_id);
Per other comment, remove this.

[Mark Dittmer, 2014-08-06 22:08:19]

Next round of comments addressed. Just one question posed about initialization
order of Extension objects and ExtensionSystem initialization (see inline
comment).

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:558: void
UserScriptLoader::UpdateExtensionsInfo() {
On 2014/08/06 15:59:25, Devlin wrote:
> On 2014/08/06 15:32:25, Mark Dittmer wrote:
> > On 2014/08/05 21:47:27, Devlin wrote:
> > > Do we need to worry about removing outdated extension infos?
> > 
> > Excellent point. I've implemented this with a public
> > ClearExtensionInfo(ExtensionId) that is invoked by the OnExtensionUnload of
> both
> > UserScriptMaster variants. Do you think this is a good solution, or should
> this
> > bookkeeping be the responsibility of the loader (in which case it should
> become
> > an ExtensionRegistryObserver).
> 
> It should probably be the job of UserScriptLoader.  As mentioned before,
making
> something an observer is pretty cheap when just using the basic pattern, and
> it's simpler/less fragile than saying "And be sure that this gets called when
an
> extension is unloaded".

Right. Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:508:
DeclarativeUserScriptMaster* master = NULL;
On 2014/08/06 15:59:25, Devlin wrote:
> Is there any risk with initializing these before the ExtensionSystem is ready?

I don't think so. These are only fetched in the context of an extension that is
declaratively adding/removing scripts. I presume that extensions are not
initialized before the extension system is ready. Is that not the case?

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:70: virtual
DeclarativeUserScriptMaster*
On 2014/08/06 15:59:25, Devlin wrote:
> nit: This is shared too, right?

Yes. Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:8: #include
"chrome/browser/profiles/profile.h"
On 2014/08/06 15:59:25, Devlin wrote:
> Don't need this.

Do too :-P  For profile_.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:49: scripts_metadata_[id]
= std::set<UserScript>();
On 2014/08/06 15:59:25, Devlin wrote:
> I'm somewhat dubious of the benefit of storing all these scripts in memory. 
We
> only need them in loaded and unloaded, so it might be better to just fetch
them
> each time (and for unloaded, we don't need to go through the trouble of
setting
> the incognito bit).  But I'll leave it up to you.

Got rid of scripts_metadata_, but I am leaving the incognito part alone. The
lookup shouldn't be expensive, and branching on a variable, set_incognito, makes
this simple function needlessly complex -- either we check set_incognito on
every iteration, or we duplicate nearly the whole function with and without
set_incognito_enabled().

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.h:61: // Listen to extension
load, unloaded notifications.
On 2014/08/06 15:59:25, Devlin wrote:
> nit: Kill this comment.

Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:17: #include
"chrome/browser/extensions/extension_util.h"
On 2014/08/06 15:59:25, Devlin wrote:
> Don't need this.

Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:20: #include
"chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
On 2014/08/06 15:59:25, Devlin wrote:
> or this.

Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:133: const std::set<UserScript>&
added_scripts,
On 2014/08/06 15:59:25, Devlin wrote:
> This can be std::set<int64>, right?

Sure. I mean the extra data is only kept in memory long enough to load script
contents, but we can reduce it to IDs back in StartLoad() and
LoadScriptsForTest().

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:451: added_scripts_.erase(*it);
On 2014/08/06 15:59:25, Devlin wrote:
> Why are we removing from |added_scripts_| here, and not |user_scripts_|?

Because we wish to avoid adding duplicates of the same script to the
user_scripts_ list. This is achieved by removing duplicates from added_scripts_,
then adding the whole added_scripts_ collection to user_scripts_.

Maybe I should re-add a comment here to explain what's going on :)

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:70: void ClearExtensionInfo(const
ExtensionId& extension_id);
On 2014/08/06 15:59:25, Devlin wrote:
> Per other comment, remove this.

Done.

[Devlin, 2014-08-07 15:46:32]

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:508:
DeclarativeUserScriptMaster* master = NULL;
On 2014/08/06 22:08:18, Mark Dittmer wrote:
> On 2014/08/06 15:59:25, Devlin wrote:
> > Is there any risk with initializing these before the ExtensionSystem is
ready?
> 
> I don't think so. These are only fetched in the context of an extension that
is
> declaratively adding/removing scripts. I presume that extensions are not
> initialized before the extension system is ready. Is that not the case?

It should be.  Perhaps we should DCHECK that, though, to be safe. :)

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:8: #include
"chrome/browser/profiles/profile.h"
On 2014/08/06 22:08:18, Mark Dittmer wrote:
> On 2014/08/06 15:59:25, Devlin wrote:
> > Don't need this.
> 
> Do too :-P  For profile_.

We never really use profile_, so just knowing that "something" is a profile is
sufficient (via a forward declare).  But we actually *do* need it for
ExtensionRegistry::Get(), because it takes a BrowserContext, so we need to know
that a profile is a browser context, so we need profile.h.  D'oh.  My bad. :)

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:49: scripts_metadata_[id]
= std::set<UserScript>();
On 2014/08/06 22:08:18, Mark Dittmer wrote:
> On 2014/08/06 15:59:25, Devlin wrote:
> > I'm somewhat dubious of the benefit of storing all these scripts in memory. 
> We
> > only need them in loaded and unloaded, so it might be better to just fetch
> them
> > each time (and for unloaded, we don't need to go through the trouble of
> setting
> > the incognito bit).  But I'll leave it up to you.
> 
> Got rid of scripts_metadata_, but I am leaving the incognito part alone. The
> lookup shouldn't be expensive, and branching on a variable, set_incognito,
makes
> this simple function needlessly complex -- either we check set_incognito on
> every iteration, or we duplicate nearly the whole function with and without
> set_incognito_enabled().

Works for me.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:133: const std::set<UserScript>&
added_scripts,
On 2014/08/06 22:08:18, Mark Dittmer wrote:
> On 2014/08/06 15:59:25, Devlin wrote:
> > This can be std::set<int64>, right?
> 
> Sure. I mean the extra data is only kept in memory long enough to load script
> contents, but we can reduce it to IDs back in StartLoad() and
> LoadScriptsForTest().

But |added_scripts| isn't actually passed by reference (that wouldn't be thread
safe, right?), so it's copied behind-the-scenes.  Thus, having a set of int64
might result in a "significant" speed up.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:451: added_scripts_.erase(*it);
On 2014/08/06 22:08:18, Mark Dittmer wrote:
> On 2014/08/06 15:59:25, Devlin wrote:
> > Why are we removing from |added_scripts_| here, and not |user_scripts_|?
> 
> Because we wish to avoid adding duplicates of the same script to the
> user_scripts_ list. This is achieved by removing duplicates from
added_scripts_,
> then adding the whole added_scripts_ collection to user_scripts_.
> 
> Maybe I should re-add a comment here to explain what's going on :)

Buuuuuuuuuuut... :P

We pass added_scripts_ to LoadScriptsOnFileThread, and scripts are only
(re)loaded if they are in that set. So if it's in |added_scripts_| because it
needs to be reloaded, then it won't be with the current loop. :)

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:463: added_scripts_.erase(*it);
See other comment

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:533:
SendUpdate(i.GetCurrentValue(), shared_memory_.get(), changed_extensions_);
I think we actually have to tweak this a bit, because I don't think
changed_extensions_ is always right.

Consider:
AddScript(1);  // ExtensionID = 1
StartLoad();  // Loads script 1
// changed_extensions_ is now {1}
AddScript(2);  // ExtensionID = 2
OnScriptsLoaded();  // Sees pending load, starts load again, loads script 2
StartLoad();
// changed_extensions_ is now {2}
OnScriptsLoaded();
SendUpdate();
// Sends update with changed_exts of {2}, even though it should be {1, 2}

Without thinking about this too much (so please double check my logic), it seems
like just waiting to clear |changed_extensions_| until after we send the update
might work.

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:80: // Initiates script load when
we have been waiting for the extension system
nit: Move this below the inherited methods.

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:129: // Set of scripts from
currently-installed extensions we should load.
back to a list. :)

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:146: std::set<ExtensionId>
changed_extensions_;
We actually have an ExtensionIdSet in extension.h.

[Mark Dittmer, 2014-08-11 14:08:05]

Addressed latest round of comments:

DCHECK xtn system ready; keep all added_scripts_; careful changed_extensions_
management.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:508:
DeclarativeUserScriptMaster* master = NULL;
On 2014/08/07 15:46:31, Devlin wrote:
> On 2014/08/06 22:08:18, Mark Dittmer wrote:
> > On 2014/08/06 15:59:25, Devlin wrote:
> > > Is there any risk with initializing these before the ExtensionSystem is
> ready?
> > 
> > I don't think so. These are only fetched in the context of an extension that
> is
> > declaratively adding/removing scripts. I presume that extensions are not
> > initialized before the extension system is ready. Is that not the case?
> 
> It should be.  Perhaps we should DCHECK that, though, to be safe. :)

Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/shared_user_script_master.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:8: #include
"chrome/browser/profiles/profile.h"
On 2014/08/07 15:46:31, Devlin wrote:
> On 2014/08/06 22:08:18, Mark Dittmer wrote:
> > On 2014/08/06 15:59:25, Devlin wrote:
> > > Don't need this.
> > 
> > Do too :-P  For profile_.
> 
> We never really use profile_, so just knowing that "something" is a profile is
> sufficient (via a forward declare).  But we actually *do* need it for
> ExtensionRegistry::Get(), because it takes a BrowserContext, so we need to
know
> that a profile is a browser context, so we need profile.h.  D'oh.  My bad. :)

Acknowledged.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/shared_user_script_master.cc:49: scripts_metadata_[id]
= std::set<UserScript>();
On 2014/08/07 15:46:31, Devlin wrote:
> On 2014/08/06 22:08:18, Mark Dittmer wrote:
> > On 2014/08/06 15:59:25, Devlin wrote:
> > > I'm somewhat dubious of the benefit of storing all these scripts in
memory. 
> > We
> > > only need them in loaded and unloaded, so it might be better to just fetch
> > them
> > > each time (and for unloaded, we don't need to go through the trouble of
> > setting
> > > the incognito bit).  But I'll leave it up to you.
> > 
> > Got rid of scripts_metadata_, but I am leaving the incognito part alone. The
> > lookup shouldn't be expensive, and branching on a variable, set_incognito,
> makes
> > this simple function needlessly complex -- either we check set_incognito on
> > every iteration, or we duplicate nearly the whole function with and without
> > set_incognito_enabled().
> 
> Works for me.

Acknowledged.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:133: const std::set<UserScript>&
added_scripts,
On 2014/08/07 15:46:31, Devlin wrote:
> On 2014/08/06 22:08:18, Mark Dittmer wrote:
> > On 2014/08/06 15:59:25, Devlin wrote:
> > > This can be std::set<int64>, right?
> > 
> > Sure. I mean the extra data is only kept in memory long enough to load
script
> > contents, but we can reduce it to IDs back in StartLoad() and
> > LoadScriptsForTest().
> 
> But |added_scripts| isn't actually passed by reference (that wouldn't be
thread
> safe, right?), so it's copied behind-the-scenes.  Thus, having a set of int64
> might result in a "significant" speed up.

Good point. Done.

https://codereview.chromium.org/420543002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:451: added_scripts_.erase(*it);
On 2014/08/07 15:46:31, Devlin wrote:
> On 2014/08/06 22:08:18, Mark Dittmer wrote:
> > On 2014/08/06 15:59:25, Devlin wrote:
> > > Why are we removing from |added_scripts_| here, and not |user_scripts_|?
> > 
> > Because we wish to avoid adding duplicates of the same script to the
> > user_scripts_ list. This is achieved by removing duplicates from
> added_scripts_,
> > then adding the whole added_scripts_ collection to user_scripts_.
> > 
> > Maybe I should re-add a comment here to explain what's going on :)
> 
> Buuuuuuuuuuut... :P
> 
> We pass added_scripts_ to LoadScriptsOnFileThread, and scripts are only
> (re)loaded if they are in that set. So if it's in |added_scripts_| because it
> needs to be reloaded, then it won't be with the current loop. :)

Right you are. I was neglecting the reload case. Switched it to insert all
scripts from added_scripts_ into user_scripts_.

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:463: added_scripts_.erase(*it);
On 2014/08/07 15:46:32, Devlin wrote:
> See other comment

Done.

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:533:
SendUpdate(i.GetCurrentValue(), shared_memory_.get(), changed_extensions_);
On 2014/08/07 15:46:32, Devlin wrote:
> I think we actually have to tweak this a bit, because I don't think
> changed_extensions_ is always right.
> 
> Consider:
> AddScript(1);  // ExtensionID = 1
> StartLoad();  // Loads script 1
> // changed_extensions_ is now {1}
> AddScript(2);  // ExtensionID = 2
> OnScriptsLoaded();  // Sees pending load, starts load again, loads script 2
> StartLoad();
> // changed_extensions_ is now {2}
> OnScriptsLoaded();
> SendUpdate();
> // Sends update with changed_exts of {2}, even though it should be {1, 2}
> 
> Without thinking about this too much (so please double check my logic), it
seems
> like just waiting to clear |changed_extensions_| until after we send the
update
> might work.

I think you're right; changed_extensions_ should track all changes between
for(all renderers) { SendUpdate(renderer); }. I replaced
ComputeChangedExtensions() with ExpandChangedExtensions(scripts), and added
ClearChangedExtensions(). The latter is invoked after our for(all renderers) {
... } loop.

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:129: // Set of scripts from
currently-installed extensions we should load.
On 2014/08/07 15:46:32, Devlin wrote:
> back to a list. :)

Done.

https://codereview.chromium.org/420543002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:146: std::set<ExtensionId>
changed_extensions_;
On 2014/08/07 15:46:32, Devlin wrote:
> We actually have an ExtensionIdSet in extension.h.

Done.

[Devlin, 2014-08-11 20:36:37]

LGTM after the nits are fixed.

Not sure if Jeffrey wants to take a look, too.

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:22: // operations are
delegated, and provides and interface for adding, removing,
s/and interface/an interface

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:581:
ExtensionRegistry::Get(profile_)
nit: cache ExtensionRegistry above the for loop.

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:113: void
ClearChangedExtensions() { changed_extensions_.clear(); }
This is a bit excessive.  Just inline changed_extensions_.clear() where this is
called.

[Jeffrey Yasskin, 2014-08-11 21:36:59]

I'm happy to trust Devlin for this CL.

[Mark Dittmer, 2014-08-12 17:08:54]

Fixed final nits and cleaned up whitespace noise.

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_master.h (right):

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_master.h:22: // operations are
delegated, and provides and interface for adding, removing,
On 2014/08/11 20:36:37, Devlin wrote:
> s/and interface/an interface

Done.

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:581:
ExtensionRegistry::Get(profile_)
On 2014/08/11 20:36:37, Devlin wrote:
> nit: cache ExtensionRegistry above the for loop.

Done.

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:113: void
ClearChangedExtensions() { changed_extensions_.clear(); }
On 2014/08/11 20:36:37, Devlin wrote:
> This is a bit excessive.  Just inline changed_extensions_.clear() where this
is
> called.

Done.

[Mark Dittmer, 2014-08-12 17:09:08]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-08-12 17:12:51]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/markdittmer@chromium.org/420543002/240001

[commit-bot: I haz the power, 2014-08-12 21:58:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-12 22:42:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-12 22:42:18]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mark Dittmer, 2014-08-14 21:53:55]

Fixed bugs:

1. Erroneous AttemptLoad() in UserScriptLoader::Observe()
2. StartLoad()'ing when there are no scripts to add/remove/clear

[Devlin, 2014-08-14 22:07:08]

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:443: if
(extension_system_ready_)
this looks wrong...why not is_loading()?

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:95: // Return whether or not it
is possible that calls to AddScripts(),
nit: Returns

[Mark Dittmer, 2014-08-14 22:28:21]

Fix nit and logic error in comments.

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:443: if
(extension_system_ready_)
On 2014/08/14 22:07:08, Devlin wrote:
> this looks wrong...why not is_loading()?

Done.

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.h (right):

https://codereview.chromium.org/420543002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.h:95: // Return whether or not it
is possible that calls to AddScripts(),
On 2014/08/14 22:07:08, Devlin wrote:
> nit: Returns

Done.

[Devlin, 2014-08-14 22:34:00]

lgtm if bots are happy

https://codereview.chromium.org/420543002/diff/300001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/300001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:429: return
(added_scripts_.size() ||  // Scripts ready to be added.
nit: let's just put the comments at the top.
// Scripts may have changed if there are new scripts added,
// scripts removed, or if scripts were cleared and either
// 1...
// 2...
return added_scripts.size() || removed_scripts.size() ||
       (clear_scripts_ &&
        (is_loading() || user_scripts_.size()));

[Mark Dittmer, 2014-08-15 00:25:58]

Fix nit and rebase from master. Trying to commit...

https://codereview.chromium.org/420543002/diff/300001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_loader.cc (right):

https://codereview.chromium.org/420543002/diff/300001/chrome/browser/extensio...
chrome/browser/extensions/user_script_loader.cc:429: return
(added_scripts_.size() ||  // Scripts ready to be added.
On 2014/08/14 22:33:59, Devlin wrote:
> nit: let's just put the comments at the top.
> // Scripts may have changed if there are new scripts added,
> // scripts removed, or if scripts were cleared and either
> // 1...
> // 2...
> return added_scripts.size() || removed_scripts.size() ||
>        (clear_scripts_ &&
>         (is_loading() || user_scripts_.size()));

Done.

[Mark Dittmer, 2014-08-15 00:26:09]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-08-15 00:28:15]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/markdittmer@chromium.org/420543002/310001

[commit-bot: I haz the power, 2014-08-15 06:41:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-15 06:47:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 06:47:35]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Mark Dittmer, 2014-08-15 14:12:06]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-08-15 14:14:30]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/markdittmer@chromium.org/420543002/310001

[commit-bot: I haz the power, 2014-08-15 19:15:32]

Committed patchset #17 (310001) as 289969