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

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

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

[Mark Dittmer, 2014-07-21 18:06:12]

Jeffrey, please review the main content of this.

Ken, please review changes to IPC messages.

[kenrb, 2014-07-21 19:15:01]

ipc lgtm

[Jeffrey Yasskin, 2014-07-24 21:09:20]

https://codereview.chromium.org/404613006/diff/20001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/404613006/diff/20001/extensions/common/extens...
extensions/common/extension_messages.h:390:
IPC_MESSAGE_CONTROL3(ExtensionMsg_UpdateUserScripts,
How are you going to use the scripts in the single-extension shared-memory
region that this creates? declarativeContent needs to be able to run particular
scripts on each page, depending on what matches, but I don't see any IDs in this
message. What's the plan?

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set.h:34: class UserScriptSet : public
content::RenderProcessObserver {
Stop inheriting from RenderProcessObserver if you don't implement any of its
methods anymore.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set.h:68: bool
OnUpdateUserScripts(base::SharedMemoryHandle shared_memory,
Any time you return a value, you should document what the value means. Yes,
Chrome is terrible at doing this, but that doesn't mean you should propagate the
problem.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:42: // programmatic_scripts -- one
region per script, instantiated lazily;
"lazily" isn't precise. Do you mean that each one is created when the extension
actually uses the declarativeContent API to declare a content script, or that
they're created the first time an extension runs code in the renderer? I think
the first is ok, although we may need to merge some of them later, but for the
second I'm worried about the file descriptor usage.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:69: const
linked_ptr<UserScriptSet> ProgrammaticScripts(
Generally don't return linked_ptr<>s. They're used to be able to put
non-copyable types in STL containers, but you shouldn't use them to share
ownership. Can you return a raw pointer here?

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:72: // Get injections from
|static_scripts| and each of |programmatic_scripts|.
What's this function do with its other arguments?

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:92: scoped_ptr<UserScriptSet>
static_scripts_;
You can store a UserScriptSet by value, rather than through a scoped_ptr.

[Mark Dittmer, 2014-07-25 15:09:05]

Addressed first round of comments: Fix use of scoped and linked ptrs, better
documentation.

https://codereview.chromium.org/404613006/diff/20001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/404613006/diff/20001/extensions/common/extens...
extensions/common/extension_messages.h:390:
IPC_MESSAGE_CONTROL3(ExtensionMsg_UpdateUserScripts,
On 2014/07/24 21:09:19, Jeffrey Yasskin wrote:
> How are you going to use the scripts in the single-extension shared-memory
> region that this creates? declarativeContent needs to be able to run
particular
> scripts on each page, depending on what matches, but I don't see any IDs in
this
> message. What's the plan?

Short answer: This data lives in the UserScript objects in the shared memory
region.

Long answer: Ultimately, this data will either: (a) live in the UserScript
object (matches, excludeMatches, etc.) or (b) be absent and the
UserScript::run_at will be something like "BROWSER_DRIVEN" and an IPC from the
browser that has checked rich URL+CSS conditions will send a "fire this script
now" IPC. This CL only contains renderer-side changes for (a).

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set.h:34: class UserScriptSet : public
content::RenderProcessObserver {
On 2014/07/24 21:09:19, Jeffrey Yasskin wrote:
> Stop inheriting from RenderProcessObserver if you don't implement any of its
> methods anymore.

Done.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set.h:68: bool
OnUpdateUserScripts(base::SharedMemoryHandle shared_memory,
On 2014/07/24 21:09:19, Jeffrey Yasskin wrote:
> Any time you return a value, you should document what the value means. Yes,
> Chrome is terrible at doing this, but that doesn't mean you should propagate
the
> problem.

Done.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:42: // programmatic_scripts -- one
region per script, instantiated lazily;
On 2014/07/24 21:09:20, Jeffrey Yasskin wrote:
> "lazily" isn't precise. Do you mean that each one is created when the
extension
> actually uses the declarativeContent API to declare a content script, or that
> they're created the first time an extension runs code in the renderer? I think
> the first is ok, although we may need to merge some of them later, but for the
> second I'm worried about the file descriptor usage.

Good point on precision of language. I mean the first, so it should be fine. We
can talk about this via chat/email.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:69: const
linked_ptr<UserScriptSet> ProgrammaticScripts(
On 2014/07/24 21:09:20, Jeffrey Yasskin wrote:
> Generally don't return linked_ptr<>s. They're used to be able to put
> non-copyable types in STL containers, but you shouldn't use them to share
> ownership. Can you return a raw pointer here?

Yes. Raw pointers are fine here. Done.

https://codereview.chromium.org/404613006/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:92: scoped_ptr<UserScriptSet>
static_scripts_;
On 2014/07/24 21:09:20, Jeffrey Yasskin wrote:
> You can store a UserScriptSet by value, rather than through a scoped_ptr.

Added clarifying comment. This wraps UserScriptSet::GetInjections(), fetching
all injections for static and programmatic scripts.

[Jeffrey Yasskin, 2014-07-28 17:47:16]

lgtm with a couple nits below.

https://codereview.chromium.org/404613006/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/common/extens...
extensions/common/extension_messages.h:384: // If |owner| is not empty, then the
shared memory handle refers to |owner|'s
In this case, is |changed extensions| ignored, or must it either be empty or
contain exactly |owner|?

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:104:
OnUserScriptsUpdated(changed_extensions, scripts_.get()));
I wonder if it'll make sense to remove OnUserScriptsUpdated from UserScriptSet
entirely, in favor of observing the UserScriptSetManager. No need in this patch.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.h:36: // Provide UserScriptSetManager with
accesss to OnUpdateUserScripts and
s/accesss/access/

[Devlin, 2014-07-28 18:15:28]

Adding myself as a reviewer for this one - still a few things I think need to be
tweeked before it goes in :)

https://codereview.chromium.org/404613006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/404613006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:548: handle_for_process, "",
changed_extensions));
nit: document anonymous string (I think we also tend to use std::string() more
frequently than "").

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:98: if (!UpdateScripts(shared_memory)) {
nit: no brackets on single-line ifs.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.h:38: friend class UserScriptSetManager;
nit: We usually put friend classes in the private: portion of a class (though it
makes no difference, I think).

That said, I think the better way to do this is to just make
OnUpdateUserScripts() a public method here - there's no reason to have it be
private anymore, even if it's only called by one class.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.h:40: class Observer {
Is this class still needed for anything?

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.cc (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:37:
IPC_MESSAGE_HANDLER(ExtensionMsg_UpdateUserScripts, OnUpdateUserScripts)
nit: Even though git cl format doesn't like it, we indent these, like so:
IPC_BEGIN...
  IPC_MESSAGE...
  IPC_MESSAGE...
IPC_END...

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:47: if (it ==
programmatic_scripts_.end()) {
nit: no brackets - although I'd probably opt for a ternary here, anyway:
return it != programmatic_scripts_.end() ? it->second.get() : NULL;

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:68: std::set<std::string>* ids)
const {
Just for safety, let's throw in a DCHECK(ids) here.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:82: if (extension_id != "") {
nit: if (!extension_id.empty())

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:83: if
(programmatic_scripts_.find(extension_id) !=
This strikes me as wrong.
if there IS an entry, replace it
else create the default (NULL) entry.

Should this be (heavily abbreviated):
if (programmatic_scripts_.find(extension_id) == end)
  scripts = new UserScriptSet();
else
  scripts = programmatic_scripts[id]
?

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:93: if (scripts &&
sanity check: Can |scripts| ever be NULL?  It looks like we're always doing
something for it...

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:21: 
nit: we usually make these forward declarations a bit smaller:
namespace IPC {
class Message;
}

(note no "// namespace IPC" and no newlines between brackets.)

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:42: //                        
programmatically declared scripts, instantiated
nit of nits: programmatically-declared

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:60: // ScopedObserver Source
implementation.
nit: Usually "X implementation." refers to a direct superclass.  These are
probably common enough that they don't even need comments, but if you wanted,
I'd just stick with "Adds or removes an observer."

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:65: virtual bool
OnControlMessageReceived(const IPC::Message& message) OVERRIDE;
Make this private.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:69: const UserScriptSet*
ProgrammaticScripts(const ExtensionId& extensionId);
If an accessor is "complex" (colloquially, not technically) and is not inlined
and hacker_style, we usually give it a full name.  In this case,
GetProgrammaticScriptsForExtension().

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:73: //
UserScriptSet::GetInjections(injections, web_frame, tab_id, run_location).
nit: Try not to include implementation details in the comments.  So, in this
case, just saying "Puts all injections from ... into |injections|" is
sufficient.  Note also that this should be "Puts", not "put", as we have a style
to describe methods in an adjectival sense, rather than in a command sense (this
*is* actually in the style guide, but trust me, I have to correct myself on this
one all the time. :))

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:103: ObserverList<Observer>
observers_;
DISALLOW_COPY_AND_ASSIGN()

[Mark Dittmer, 2014-07-28 22:21:29]

Responded to all comments and implemented fixes for each.

https://codereview.chromium.org/404613006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/404613006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:548: handle_for_process, "",
changed_extensions));
On 2014/07/28 18:15:27, Devlin wrote:
> nit: document anonymous string (I think we also tend to use std::string() more
> frequently than "").

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/common/extens...
extensions/common/extension_messages.h:384: // If |owner| is not empty, then the
shared memory handle refers to |owner|'s
On 2014/07/28 17:47:15, Jeffrey Yasskin wrote:
> In this case, is |changed extensions| ignored, or must it either be empty or
> contain exactly |owner|?

|owner| indicates the extension associated with the shared memory object. That
is, it is the key for looking up the correct memory object. The semantics of
|changed_extensions| are the same; any extension that is known to be changed
should be listed.

As it turns out, messages with owner != "" will have exactly one extension
listed in |changed_extensions|. The only context in which owner != "" is a
context in which we wish to notify that the owner extension (and no other
extension) has changed.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:98: if (!UpdateScripts(shared_memory)) {
On 2014/07/28 18:15:27, Devlin wrote:
> nit: no brackets on single-line ifs.

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:104:
OnUserScriptsUpdated(changed_extensions, scripts_.get()));
On 2014/07/28 17:47:16, Jeffrey Yasskin wrote:
> I wonder if it'll make sense to remove OnUserScriptsUpdated from UserScriptSet
> entirely, in favor of observing the UserScriptSetManager. No need in this
patch.

Acknowledged.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.h:36: // Provide UserScriptSetManager with
accesss to OnUpdateUserScripts and
On 2014/07/28 17:47:16, Jeffrey Yasskin wrote:
> s/accesss/access/

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.h:38: friend class UserScriptSetManager;
On 2014/07/28 18:15:27, Devlin wrote:
> nit: We usually put friend classes in the private: portion of a class (though
it
> makes no difference, I think).
> 
> That said, I think the better way to do this is to just make
> OnUpdateUserScripts() a public method here - there's no reason to have it be
> private anymore, even if it's only called by one class.

Done, but this required an accessor for scripts_ as well. I don't see any
problem with that, but if you do we can revert to friending instead.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.h:40: class Observer {
On 2014/07/28 18:15:27, Devlin wrote:
> Is this class still needed for anything?

Yes. The UserScriptInjector still observes each UserScriptSet individually. It
"doesn't know and doesn't care" whether a region is declarative+per-extension or
static/all-extensions. As Jeffrey pointed out elsewhere, we may choose to get
rid of this pattern later, but not in this CL.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:21: 
On 2014/07/28 18:15:28, Devlin wrote:
> nit: we usually make these forward declarations a bit smaller:
> namespace IPC {
> class Message;
> }
> 
> (note no "// namespace IPC" and no newlines between brackets.)

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:42: //                        
programmatically declared scripts, instantiated
On 2014/07/28 18:15:28, Devlin wrote:
> nit of nits: programmatically-declared

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:60: // ScopedObserver Source
implementation.
On 2014/07/28 18:15:28, Devlin wrote:
> nit: Usually "X implementation." refers to a direct superclass.  These are
> probably common enough that they don't even need comments, but if you wanted,
> I'd just stick with "Adds or removes an observer."

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:65: virtual bool
OnControlMessageReceived(const IPC::Message& message) OVERRIDE;
On 2014/07/28 18:15:28, Devlin wrote:
> Make this private.

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:69: const UserScriptSet*
ProgrammaticScripts(const ExtensionId& extensionId);
On 2014/07/28 18:15:28, Devlin wrote:
> If an accessor is "complex" (colloquially, not technically) and is not inlined
> and hacker_style, we usually give it a full name.  In this case,
> GetProgrammaticScriptsForExtension().

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:73: //
UserScriptSet::GetInjections(injections, web_frame, tab_id, run_location).
On 2014/07/28 18:15:28, Devlin wrote:
> nit: Try not to include implementation details in the comments.  So, in this
> case, just saying "Puts all injections from ... into |injections|" is
> sufficient.  Note also that this should be "Puts", not "put", as we have a
style
> to describe methods in an adjectival sense, rather than in a command sense
(this
> *is* actually in the style guide, but trust me, I have to correct myself on
this
> one all the time. :))

It's good to get the details right! Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:103: ObserverList<Observer>
observers_;
On 2014/07/28 18:15:28, Devlin wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

[Mark Dittmer, 2014-07-29 16:25:57]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-07-29 16:27:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/markdittmer@chromium.org/404613006/60001

[Devlin, 2014-07-29 16:34:18]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2014-07-29 16:35:16]

On 2014/07/29 16:34:18, Devlin wrote:
> The CQ bit was unchecked by mailto:rdevlin.cronin@chromium.org

Looks like a few comments were missed in user_script_set_manager.cc.

[Mark Dittmer, 2014-07-29 16:59:31]

Fixed UserScriptMasterManager according to comments. My apologies for the early
commit attempt. Skipped over one file by mistake.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.cc (right):

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:37:
IPC_MESSAGE_HANDLER(ExtensionMsg_UpdateUserScripts, OnUpdateUserScripts)
On 2014/07/28 18:15:27, Devlin wrote:
> nit: Even though git cl format doesn't like it, we indent these, like so:
> IPC_BEGIN...
>   IPC_MESSAGE...
>   IPC_MESSAGE...
> IPC_END...

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:47: if (it ==
programmatic_scripts_.end()) {
On 2014/07/28 18:15:28, Devlin wrote:
> nit: no brackets - although I'd probably opt for a ternary here, anyway:
> return it != programmatic_scripts_.end() ? it->second.get() : NULL;

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:68: std::set<std::string>* ids)
const {
On 2014/07/28 18:15:28, Devlin wrote:
> Just for safety, let's throw in a DCHECK(ids) here.

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:82: if (extension_id != "") {
On 2014/07/28 18:15:27, Devlin wrote:
> nit: if (!extension_id.empty())

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:83: if
(programmatic_scripts_.find(extension_id) !=
On 2014/07/28 18:15:27, Devlin wrote:
> This strikes me as wrong.
> if there IS an entry, replace it
> else create the default (NULL) entry.
> 
> Should this be (heavily abbreviated):
> if (programmatic_scripts_.find(extension_id) == end)
>   scripts = new UserScriptSet();
> else
>   scripts = programmatic_scripts[id]
> ?

Done.

https://codereview.chromium.org/404613006/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.cc:93: if (scripts &&
On 2014/07/28 18:15:27, Devlin wrote:
> sanity check: Can |scripts| ever be NULL?  It looks like we're always doing
> something for it...

Done.

[Devlin, 2014-07-29 17:11:57]

Few more small comments.

https://codereview.chromium.org/404613006/diff/80001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/common/extens...
extensions/common/extension_messages.h:390: // supported for per-extension
programmatically defined script regions; in such
nit of nits: programmatically-defined

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:84: if
(!base::SharedMemory::IsHandleValid(shared_memory)) {
These can (and should) be moved to UserScriptSetManager - they are basically IPC
security checks, and should happen immediately upon receiving the message. 
(Which also means we can probably combine OnUpdateUserScripts and UpdatesScripts
into one method.)

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set.h:50: const std::vector<UserScript*>&
scripts() const { return scripts_.get(); }
nit: accessor should go last (just above "private:")

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set.h:64: // Handle the UpdateUserScripts
extension message. Return true if the
This is kinda lying - it doesn't really handle the message, that's
UserScriptSetManager.  How about "Updates |scripts| given the shared memory
region containing user scripts. Returns true if the scripts were successfully
updated."  Similarly, the method should probably be renamed to simply
"UpdateUserScripts"

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:59: const UserScriptSet*
static_scripts() const { return &static_scripts_; }
accessor right above private:

[Mark Dittmer, 2014-07-29 22:01:14]

Implemented fixes for third round of comments.

https://codereview.chromium.org/404613006/diff/80001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/common/extens...
extensions/common/extension_messages.h:390: // supported for per-extension
programmatically defined script regions; in such
On 2014/07/29 17:11:57, Devlin wrote:
> nit of nits: programmatically-defined

Done.

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:84: if
(!base::SharedMemory::IsHandleValid(shared_memory)) {
On 2014/07/29 17:11:57, Devlin wrote:
> These can (and should) be moved to UserScriptSetManager - they are basically
IPC
> security checks, and should happen immediately upon receiving the message. 
> (Which also means we can probably combine OnUpdateUserScripts and
UpdatesScripts
> into one method.)

Done. Kept the OnUpdateUserScripts method name+signature.

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set.h:50: const std::vector<UserScript*>&
scripts() const { return scripts_.get(); }
On 2014/07/29 17:11:57, Devlin wrote:
> nit: accessor should go last (just above "private:")

Done.

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set.h:64: // Handle the UpdateUserScripts
extension message. Return true if the
On 2014/07/29 17:11:57, Devlin wrote:
> This is kinda lying - it doesn't really handle the message, that's
> UserScriptSetManager.  How about "Updates |scripts| given the shared memory
> region containing user scripts. Returns true if the scripts were successfully
> updated."  Similarly, the method should probably be renamed to simply
> "UpdateUserScripts"

Done.

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/80001/extensions/renderer/user...
extensions/renderer/user_script_set_manager.h:59: const UserScriptSet*
static_scripts() const { return &static_scripts_; }
On 2014/07/29 17:11:57, Devlin wrote:
> accessor right above private:

Done.

[Devlin, 2014-07-29 22:06:40]

LGTM! =)

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

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
extensions/renderer/user_script_set.h:44: virtual ~UserScriptSet();
nit: no longer need virtual destructor

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
extensions/renderer/user_script_set.h:64: bool
OnUpdateUserScripts(base::SharedMemoryHandle shared_memory,
nit: just call this UpdateUserScripts.

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
extensions/renderer/user_script_set_manager.h:70: // |programmatic_scripts|.
nit: programmatic_scripts_ (trailing underscore)

[Mark Dittmer, 2014-07-29 22:20:41]

Fixes for last round of comments patched in.

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

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
extensions/renderer/user_script_set.h:44: virtual ~UserScriptSet();
On 2014/07/29 22:06:39, Devlin wrote:
> nit: no longer need virtual destructor

Done.

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
extensions/renderer/user_script_set.h:64: bool
OnUpdateUserScripts(base::SharedMemoryHandle shared_memory,
On 2014/07/29 22:06:39, Devlin wrote:
> nit: just call this UpdateUserScripts.

Done.

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
File extensions/renderer/user_script_set_manager.h (right):

https://codereview.chromium.org/404613006/diff/100001/extensions/renderer/use...
extensions/renderer/user_script_set_manager.h:70: // |programmatic_scripts|.
On 2014/07/29 22:06:39, Devlin wrote:
> nit: programmatic_scripts_ (trailing underscore)

Done.

[Mark Dittmer, 2014-07-29 22:20:53]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-07-29 22:23:30]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/markdittmer@chromium.org/404613006/120001

[commit-bot: I haz the power, 2014-07-30 06:27:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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-07-30 09:52:05]

Change committed as 286457