Refactor renderer-side script injection

Refactor renderer-side script injection.
Instead of having two script injection systems (UserScriptScheduler and
UserScriptSlave), we have a single system (ScriptInjectionManager).

Introduce abstract ScriptInjection class as an object which knows how to inject
itself and whether or not it is allowed, with two implementations
(UserScriptInjection and ProgrammaticScriptInjection).

Primary benefit:
Combine logic for script injections so that a) it's all in one place, b) when
we ask for permission, we can do so analogously.

Additional benefits:
- Reduce dependency of script injection on ExtensionHelper.
- Eliminate ScriptInjection dependency on UserScriptSlave (or
  ScriptInjectionManager).

BUG=382945
TBR=jschuh@chromium.org (name changes to extension_messages.h, no functional changes)
TBR=jochen@chromium.org (two-line change to chrome/renderer/chrome_content_renderer_client.cc)

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

[Devlin, 2014-06-10 16:52:00]

Ben, please take a look.  It might be a bit rough, but should be mostly done.

There are a few parts which have largely similar logic to their previous
implementation, so here's a map to hopefully make the review simpler:
UserScriptScheduler::ExecuteCode(Impl) -> ProgrammaticScriptInjection
ScriptInjection -> UserScriptInjection
UserScriptSlave::UpdateScripts -> UserScriptInjectionList::UpdateScripts
UserScriptSlave::*IsolatedWorld* -> ScriptInjection::*IsolatedWorld*
ExtensionHelper stuff -> ScriptInjectionManager::RVOHelper

With this setup, adding the withheld permissions model should be simple for both
UserScript injections and tabs.executeScript() injections.

[not at google - send to devlin, 2014-06-11 18:26:30]

Here are my comments from the first patchset. I'll continue reviewing on the new
patchset.

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:789:
script_injection_manager_->UpdateUserScripts(scripts, extension_ids);
It would be really nice to make ScriptInjectionManager a RenderProcessObserver,
then you can move this IPC handling into there.

The catch is this call to UpdateActiveExtensions. We basically only need that
for crash reporting, where we say the extensions that were active in this
process, and the extensions which had content scripts.

Side note: the content script part is actually a lie, because it doesn't include
the extensions that have done tabs.executeScript (as you note) and it includes
all content scripts not just those that have run.

So I think there's a call to fix this. It would even be nice to do so in this CL
to reduce the interface load on ScriptInjectionManager (a Manager being told to
update its own state is odd), then expose an Observer interface on
ScriptInjectionManager when the active extensions update there. That would let
you fix the crash reporting in a later CL.

Note that to actually observe the IPCs you need to call AddObserver(this), and
for some reason that doesn't appear to be the pattern. but it could be. In fact,
you could make Dispatcher AddObserver-itself as well, but that's a separate
issue.

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.h (right):

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.h:32: 
comment

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.h:46: const std::set<std::string>&
changed_extensions);
w.r.t the comment in dispatcher.cc, these 2 methods wouldn't be necessary, you'd
just have a method like WhenActiveExtensionsUpdate which takes a base::Callback.
super simple observer interfaces are nice.

[not at google - send to devlin, 2014-06-11 20:21:27]

sending out another small number of comments awaiting the next patchset.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:134: 
(attaching to random place)

generally this file and others could do with some hand-holding comments
scattered around the place.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:222: // Should we be including
extensions we know are from tabs.executeScript()?
yes, and as I mentioned we should only be including user scripts that have
actually run.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:304: } else if
(injection->Allowed(extension)) {
no else after continue

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
File extensions/renderer/user_script_injection_list.h (right):

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:29: class
UserScriptInjectionList {
comment

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:34: // Return the URL to use as
the document url when checking permissions for
Returns

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:45: // to a changed extension,
and update the references of the others.
s/a changed extension/changed extensions in |changed_ids|/

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:48: 
if it's public, document it

[Devlin, 2014-06-11 20:26:13]

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:789:
script_injection_manager_->UpdateUserScripts(scripts, extension_ids);
On 2014/06/11 18:26:29, kalman wrote:
> It would be really nice to make ScriptInjectionManager a
RenderProcessObserver,
> then you can move this IPC handling into there.
> 
> The catch is this call to UpdateActiveExtensions. We basically only need that
> for crash reporting, where we say the extensions that were active in this
> process, and the extensions which had content scripts.
> 
> Side note: the content script part is actually a lie, because it doesn't
include
> the extensions that have done tabs.executeScript (as you note) and it includes
> all content scripts not just those that have run.
> 
> So I think there's a call to fix this. It would even be nice to do so in this
CL
> to reduce the interface load on ScriptInjectionManager (a Manager being told
to
> update its own state is odd), then expose an Observer interface on
> ScriptInjectionManager when the active extensions update there. That would let
> you fix the crash reporting in a later CL.
> 
> Note that to actually observe the IPCs you need to call AddObserver(this), and
> for some reason that doesn't appear to be the pattern. but it could be. In
fact,
> you could make Dispatcher AddObserver-itself as well, but that's a separate
> issue.

Done.

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.h (right):

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.h:32: 
On 2014/06/11 18:26:29, kalman wrote:
> comment

Done.

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.h:46: const std::set<std::string>&
changed_extensions);
On 2014/06/11 18:26:29, kalman wrote:
> w.r.t the comment in dispatcher.cc, these 2 methods wouldn't be necessary,
you'd
> just have a method like WhenActiveExtensionsUpdate which takes a
base::Callback.
> super simple observer interfaces are nice.

Done.

[Mark Dittmer, 2014-06-11 20:47:56]

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

https://codereview.chromium.org/321993003/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:49: void
GetInjections(ScopedVector<ScriptInjection>* injections,
nit: Injections appears to be an output parameter. It should probably go at the
end of the parameter list.

[not at google - send to devlin, 2014-06-11 20:57:50]

gotta run, 2 more comments, will review more when I'm back after meetings

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
File extensions/renderer/script_injection.h (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection.h:73: virtual bool Allowed(const Extension*
extension) = 0;
pls use a Delegate interface, not inheritance.

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection.h:84: Type type() const { return type_; }
exposing Type/type() somewhat defeats the purpose of having this be an abstract
superclass. I'll comment on the place that you do this, so this comment is more
for context; hopefully you can remove Type and type().

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.cc (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:308:
ScopedVector<ScriptInjection>* injections,
It's odd for UserScriptInjectionList, which deals exclusively with
UserScriptInjections, to need to be told about a bunch of injections which it
potentially doesn't care about. This is related to the Type/type() comment. I
think it can be solved by changing the ownership rules:

- UserScriptInjectionList should own its ScriptInjections.
- This method should only take the changed IDs.

There would be a moderate amount of changes needed in ScriptInjectionManager to
handle this, but it seems as simple as UserScriptManager owning its
ProgrammaticScriptInjections and then delegating its ownership to
UserScriptInjectionList.

[Devlin, 2014-06-11 21:40:42]

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:222: // Should we be including
extensions we know are from tabs.executeScript()?
On 2014/06/11 20:21:26, kalman wrote:
> yes, and as I mentioned we should only be including user scripts that have
> actually run.

Delaying 'til a later patch so this maintains current behavior, per in-person
conversation.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:304: } else if
(injection->Allowed(extension)) {
On 2014/06/11 20:21:26, kalman wrote:
> no else after continue

Done.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
File extensions/renderer/user_script_injection_list.h (right):

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:29: class
UserScriptInjectionList {
On 2014/06/11 20:21:26, kalman wrote:
> comment 

Done.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:34: // Return the URL to use as
the document url when checking permissions for
On 2014/06/11 20:21:26, kalman wrote:
> Returns

Done.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:45: // to a changed extension,
and update the references of the others.
On 2014/06/11 20:21:26, kalman wrote:
> s/a changed extension/changed extensions in |changed_ids|/

Done.

https://codereview.chromium.org/321993003/diff/60001/extensions/renderer/user...
extensions/renderer/user_script_injection_list.h:48: 
On 2014/06/11 20:21:26, kalman wrote:
> if it's public, document it

Whoops!  Done.  (I actually prefer the rule "if it's not inline, document it".)

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
File extensions/renderer/script_injection.h (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection.h:73: virtual bool Allowed(const Extension*
extension) = 0;
On 2014/06/11 20:57:50, kalman wrote:
> pls use a Delegate interface, not inheritance.

We can... but why?  It seems that just having Allowed() { return
delegate_->Allowed(); } and Inject() { delegate_->Inject(); } are a bit extreme.
 If we know for a fact that we are always going to delegate it away, and the
entire rest of this class is simple accessors, doesn't it make sense to subclass
instead of delegate?

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection.h:84: Type type() const { return type_; }
On 2014/06/11 20:57:50, kalman wrote:
> exposing Type/type() somewhat defeats the purpose of having this be an
abstract
> superclass. I'll comment on the place that you do this, so this comment is
more
> for context; hopefully you can remove Type and type().

See other comment.

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.cc (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:308:
ScopedVector<ScriptInjection>* injections,
On 2014/06/11 20:57:50, kalman wrote:
> It's odd for UserScriptInjectionList, which deals exclusively with
> UserScriptInjections, to need to be told about a bunch of injections which it
> potentially doesn't care about. This is related to the Type/type() comment. I
> think it can be solved by changing the ownership rules:
> 
> - UserScriptInjectionList should own its ScriptInjections.
> - This method should only take the changed IDs.
> 
> There would be a moderate amount of changes needed in ScriptInjectionManager
to
> handle this, but it seems as simple as UserScriptManager owning its
> ProgrammaticScriptInjections and then delegating its ownership to
> UserScriptInjectionList.

I had thought about this, but it makes a lot of other places messy, too.  Like,
then we need to inform UserScriptInjectionList about permissions being granted,
and UserScriptInjectionList needs to have its own queue of pending injections,
which seems very weird.  Also, then we're back to injecting from two classes,
which I think I'd like to avoid if possible.  I'm trying to have
UserScriptInjectionList be more of a provider than a manager - it's only
complicated by the need to update script references (note that this would be
helped if we had a shared-memory-per-extension model).

[Devlin, 2014-06-11 21:40:43]

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
File extensions/renderer/script_injection.h (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection.h:73: virtual bool Allowed(const Extension*
extension) = 0;
On 2014/06/11 20:57:50, kalman wrote:
> pls use a Delegate interface, not inheritance.

We can... but why?  It seems that just having Allowed() { return
delegate_->Allowed(); } and Inject() { delegate_->Inject(); } are a bit extreme.
 If we know for a fact that we are always going to delegate it away, and the
entire rest of this class is simple accessors, doesn't it make sense to subclass
instead of delegate?

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.cc (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:308:
ScopedVector<ScriptInjection>* injections,
On 2014/06/11 20:57:50, kalman wrote:
> It's odd for UserScriptInjectionList, which deals exclusively with
> UserScriptInjections, to need to be told about a bunch of injections which it
> potentially doesn't care about. This is related to the Type/type() comment. I
> think it can be solved by changing the ownership rules:
> 
> - UserScriptInjectionList should own its ScriptInjections.
> - This method should only take the changed IDs.
> 
> There would be a moderate amount of changes needed in ScriptInjectionManager
to
> handle this, but it seems as simple as UserScriptManager owning its
> ProgrammaticScriptInjections and then delegating its ownership to
> UserScriptInjectionList.

I had thought about this, but it makes a lot of other places messy, too.  Like,
then we need to inform UserScriptInjectionList about permissions being granted,
and UserScriptInjectionList needs to have its own queue of pending injections,
which seems very weird.  Also, then we're back to injecting from two classes,
which I think I'd like to avoid if possible.  I'm trying to have
UserScriptInjectionList be more of a provider than a manager - it's only
complicated by the need to update script references (note that this would be
helped if we had a shared-memory-per-extension model).

[Devlin, 2014-06-12 15:41:02]

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
File extensions/renderer/script_injection.h (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection.h:73: virtual bool Allowed(const Extension*
extension) = 0;
On 2014/06/11 21:40:42, Devlin wrote:
> On 2014/06/11 20:57:50, kalman wrote:
> > pls use a Delegate interface, not inheritance.
> 
> We can... but why?  It seems that just having Allowed() { return
> delegate_->Allowed(); } and Inject() { delegate_->Inject(); } are a bit
extreme.
>  If we know for a fact that we are always going to delegate it away, and the
> entire rest of this class is simple accessors, doesn't it make sense to
subclass
> instead of delegate?

Thinking down the road to how we might want this to look when these classes
share more code, we _could_ have a delegate pattern that kinda makes sense. 
Unfortunately, it's big.

Delegate {
  bool ShouldExecuteInMainWorld();
  bool ShouldIncludeChildFrames();
  bool ExpectResults();
  bool HasJS();
  bool HasCSS();
  std::vector<std::string> GetJS();
  std::vector<std::string> GetCSS();
  void OnInjectionComplete(Status success);
};

Some of these could change (we could say that returning an empty vector from
GetJS() is equivalent to HasJS() = false), but that's the gist of it.  The
upside is then we have one single injection method on ScriptInjection.  The
downside is that performance and line count (and, depending on whom you ask,
complexity) suffer, because many of those criteria are only ever true for one
type of injection.  For instance, programmatic injections only ever have one
script, which is either js or css (instead of having [n >= 0] of both), and user
scripts never execute in the main world.

The alternative is to keep the subclassing and put common methods (e.g.,
ExecuteScriptInIsolatedWorld()) on ScriptInjection, and have the subclasses
utilize those common methods in the Inject() method.  This results in better
performance and line count, but does rely on the subclassing pattern.

In the end, I'll be okay with either one.  I personally think that the delegate
pattern is not quite right here (and, in general, is a bit overused in
Chromium), but I'll defer to your preference.

[Devlin, 2014-06-12 21:24:56]

Ben, take a look at this one.

UserScriptInjections now update their script references by themselves, and
there's an Observer interface on UserScriptInjectionList.  We got rid of that
ScriptInjection::type() nonsense, and the IPC to update scripts goes directly to
the UserScriptInjectionList.  Also, miraculously, this patchset manages to have
a negative net line count.

In a followup, I can look into how to clean up ScriptInjection::Inject more.

[not at google - send to devlin, 2014-06-15 23:53:38]

sorry this took so long.

have some suggestions, trying to keep them as noninvasive as possible.

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injection.cc (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injection.cc:166:
EXTENSION_GROUP_CONTENT_SCRIPTS,  // A blatant lie!
not actually a lie. maybe badly named, but this is just a way to tag contexts
that get created as being content scripts.

<muse>could probably do this another way, by comparing the frame origin to the
context origin. if the context origin is chrome-extension:// and the frame
origin isn't then this is a content script.</muse>

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injection.cc (right):

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injection.cc:96: // Send an error
message indicating we could not execute script.
odd that a method called "Allowed" would have a side-effect. you're relying on
callers of this to only call it exactly once per script injection, which may go
subtly wrong. looking at the call sites it seems to be used correctly, but it's
not obvious. It also has interplay with both Inject (Inject can't be called
after here) and ExtensionNotFound (which is another mutually exclusive method
from the 2 of them).

I have some ideas in script_execution_manager.cc.

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:280: else if
(injection->Allowed(extension))
mention that injections_awaiting_location don't need to go through the
RequestPermissionOrInject flow because the browser has already mediated that.

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:367:
injections_awaiting_location_.push_back(injection.release());
There are 2 fairly easy changes you could make here which would improve the code
substantially.
1. Combine ExtensionNotFound, Allowed, and Inject into a single method
"TryToInject" (or similar). I don't actually think it needs to return anything.
That TryToInject method would need to accept a NULL extension and deal with it
itself, but that's fine.
2. Combine injections_awaiting_location_ and injections_awaiting_location_ into
a single list. The optimisation that you can infer which list to check based on
the event is unnecessarily complicated.


(I'm actually thinking that ScriptInjectionManager shouldn't even exist and the
ScriptInjection instances manage their own Observation of events/IPC and
lifetime... but that's a much bigger change)

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.h (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.h:47: UserScriptInjectionList*
user_script_injection_list() {
it would make more sense to me for Dispatcher to own UserScriptInjectionList
given it depends on it and there's no particular reason why
ScriptInjectionManager should own it.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
File extensions/renderer/user_script_injection.cc (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection.cc:70: inject_css_(inject_css),
see comment in user_script_injection_list; I'd skip holding onto script_id_,
inject_js_, and inject_css_; it just makes more code.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection.cc:160: // will be removed, and
there's no guarantee the backing script still even
s/even //

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.cc (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:171: }
blank line

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:199: kNoProcessId,
inling -1 with those comments "Content scripts are not tab-specific" etc is
cleaner IMO than having kNoTabId constants.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:207:
!script->js_scripts().empty() && script->run_location() == run_location;
this mixing of UserScriptInjection getting its properties either passed in or
read off its UserScript, and/or then caching some of the values, is a little
confusing.
- I wouldn't bother passing in inject_js and inject_css. UserScriptInjection can
calculate "script_->js_scripts().empty()" just fine, likewise css.
- I also just wouldn't bother caching anything you can read straight off
script_, like the script ID.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.h (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.h:34: class
UserScriptInjectionList : public content::RenderProcessObserver {
I would say "nit: this class doesn't have List semantics so it should be called
UserScriptSomethingElse" but I've almost gotten used to the code as it is at the
moment, so I'm not sure it's worth the churn to change it. Note that your
comment is "The UserScriptInjectionList is a collection of UserScripts" so why
not UserScriptCollection.

Or "Set"; it's a Set insofar as the order isn't relevant (it may be
deterministic but it's not relevant) and each UserScript is unique, and "set" is
shorter than "collection".

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.h:45: static GURL
GetDocumentUrlForFrame(blink::WebFrame* frame);
this function is only used from script_injection_manager, so it could go an
anonymouse namespace in there.

I swear it was used elsewhere at some point... how odd.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.h:66: const ExtensionSet*
extensions);
just pass in |extensions| as a constructor/member variable

[Devlin, 2014-06-16 18:11:17]

Not sure I like what making the TryToInject() method did to the ScriptInjection
classes.  Let me know if we want to make a bigger change here to clean it up.

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injection.cc (right):

https://codereview.chromium.org/321993003/diff/120001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injection.cc:166:
EXTENSION_GROUP_CONTENT_SCRIPTS,  // A blatant lie!
On 2014/06/15 23:53:37, kalman wrote:
> not actually a lie. maybe badly named, but this is just a way to tag contexts
> that get created as being content scripts.
> 
> <muse>could probably do this another way, by comparing the frame origin to the
> context origin. if the context origin is chrome-extension:// and the frame
> origin isn't then this is a content script.</muse>

Okay, comment removed.

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injection.cc (right):

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injection.cc:96: // Send an error
message indicating we could not execute script.
On 2014/06/15 23:53:37, kalman wrote:
> odd that a method called "Allowed" would have a side-effect. you're relying on
> callers of this to only call it exactly once per script injection, which may
go
> subtly wrong. looking at the call sites it seems to be used correctly, but
it's
> not obvious. It also has interplay with both Inject (Inject can't be called
> after here) and ExtensionNotFound (which is another mutually exclusive method
> from the 2 of them).
> 
> I have some ideas in script_execution_manager.cc.

Done.

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:280: else if
(injection->Allowed(extension))
On 2014/06/15 23:53:37, kalman wrote:
> mention that injections_awaiting_location don't need to go through the
> RequestPermissionOrInject flow because the browser has already mediated that.

Done.

https://codereview.chromium.org/321993003/diff/140001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:367:
injections_awaiting_location_.push_back(injection.release());
On 2014/06/15 23:53:37, kalman wrote:
> There are 2 fairly easy changes you could make here which would improve the
code
> substantially.
> 1. Combine ExtensionNotFound, Allowed, and Inject into a single method
> "TryToInject" (or similar). I don't actually think it needs to return
anything.
> That TryToInject method would need to accept a NULL extension and deal with it
> itself, but that's fine.
> 2. Combine injections_awaiting_location_ and injections_awaiting_location_
into
> a single list. The optimisation that you can infer which list to check based
on
> the event is unnecessarily complicated.
> 
> 
> (I'm actually thinking that ScriptInjectionManager shouldn't even exist and
the
> ScriptInjection instances manage their own Observation of events/IPC and
> lifetime... but that's a much bigger change)

Done...

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.h (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.h:47: UserScriptInjectionList*
user_script_injection_list() {
On 2014/06/15 23:53:38, kalman wrote:
> it would make more sense to me for Dispatcher to own UserScriptInjectionList
> given it depends on it and there's no particular reason why
> ScriptInjectionManager should own it.

Done.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
File extensions/renderer/user_script_injection.cc (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection.cc:70: inject_css_(inject_css),
On 2014/06/15 23:53:38, kalman wrote:
> see comment in user_script_injection_list; I'd skip holding onto script_id_,
> inject_js_, and inject_css_; it just makes more code.

Done.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection.cc:160: // will be removed, and
there's no guarantee the backing script still even
On 2014/06/15 23:53:38, kalman wrote:
> s/even //

Done.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.cc (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:171: }
On 2014/06/15 23:53:38, kalman wrote:
> blank line

Done.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:199: kNoProcessId,
On 2014/06/15 23:53:38, kalman wrote:
> inling -1 with those comments "Content scripts are not tab-specific" etc is
> cleaner IMO than having kNoTabId constants.

Done.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.cc:207:
!script->js_scripts().empty() && script->run_location() == run_location;
On 2014/06/15 23:53:38, kalman wrote:
> this mixing of UserScriptInjection getting its properties either passed in or
> read off its UserScript, and/or then caching some of the values, is a little
> confusing.
> - I wouldn't bother passing in inject_js and inject_css. UserScriptInjection
can
> calculate "script_->js_scripts().empty()" just fine, likewise css.
> - I also just wouldn't bother caching anything you can read straight off
> script_, like the script ID.

Caching script id is done because when we need to update the script, we don't
want to read the id from the now-dangerously-possibly-deleted script object. 
Added a comment to make that clear.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
File extensions/renderer/user_script_injection_list.h (right):

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.h:34: class
UserScriptInjectionList : public content::RenderProcessObserver {
On 2014/06/15 23:53:38, kalman wrote:
> I would say "nit: this class doesn't have List semantics so it should be
called
> UserScriptSomethingElse" but I've almost gotten used to the code as it is at
the
> moment, so I'm not sure it's worth the churn to change it. Note that your
> comment is "The UserScriptInjectionList is a collection of UserScripts" so why
> not UserScriptCollection.
> 
> Or "Set"; it's a Set insofar as the order isn't relevant (it may be
> deterministic but it's not relevant) and each UserScript is unique, and "set"
is
> shorter than "collection".

Done.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.h:45: static GURL
GetDocumentUrlForFrame(blink::WebFrame* frame);
On 2014/06/15 23:53:38, kalman wrote:
> this function is only used from script_injection_manager, so it could go an
> anonymouse namespace in there.
> 
> I swear it was used elsewhere at some point... how odd.

It was in ScriptInjectionManager, but doesn't need to be.  Moved.

https://codereview.chromium.org/321993003/diff/160001/extensions/renderer/use...
extensions/renderer/user_script_injection_list.h:66: const ExtensionSet*
extensions);
On 2014/06/15 23:53:38, kalman wrote:
> just pass in |extensions| as a constructor/member variable

Done.

[not at google - send to devlin, 2014-06-16 21:35:50]

lgtm, minor comments left.

I requested a few places for more detailed comments. I think that's something
you can apply more of in general.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:222: rvo_helpers_.erase(iter);
is there any reason you haven't written this like a traditional for-loop?

for (iter : rvo_helpers) {
  if (helper == *iter) {
    rvo_helper.erase(iter);
    break;
  }
}

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:240: FrameStatus& frame_status =
frame_statuses_[frame];
add comment like:

"This method may be called to execute scripts at IDLE multiple times, for
example, it gets called from both DidFinishLoad and DidFinishDocumentLoad.
Ensure scripts are only injected once."

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:247: 
add comment like:

"Try to inject scripts that were waiting for the correct document location"

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:261:
ScopedVector<ScriptInjection> user_script_injections;
Add a comment like:

"Try to inject any user scripts that should run for this location. If they don't
complete their injection (for example, waiting for a permission response) then
they will be added to |pending_injections_|.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:75: *iter, web_frame, tab_id,
run_location, document_url, extension);
Have you considered segmenting |scripts_| into 3 lists, start|end|idle?

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

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/use...
extensions/renderer/user_script_set.h:32: // itself from SharedMemory and also
knows how to create ScriptInjections for
s/also knows how to//

[Devlin, 2014-06-18 18:57:06]

Ben, please take a quick look at the changes to ScriptInjectionManager from 9 to
10.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:222: rvo_helpers_.erase(iter);
On 2014/06/16 21:35:49, kalman wrote:
> is there any reason you haven't written this like a traditional for-loop?
> 
> for (iter : rvo_helpers) {
>   if (helper == *iter) {
>     rvo_helper.erase(iter);
>     break;
>   }
> }

Nope, just for fun.  Made it normal.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:240: FrameStatus& frame_status =
frame_statuses_[frame];
On 2014/06/16 21:35:49, kalman wrote:
> add comment like:
> 
> "This method may be called to execute scripts at IDLE multiple times, for
> example, it gets called from both DidFinishLoad and DidFinishDocumentLoad.
> Ensure scripts are only injected once."

Done.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:247: 
On 2014/06/16 21:35:49, kalman wrote:
> add comment like:
> 
> "Try to inject scripts that were waiting for the correct document location"

Done.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:261:
ScopedVector<ScriptInjection> user_script_injections;
On 2014/06/16 21:35:49, kalman wrote:
> Add a comment like:
> 
> "Try to inject any user scripts that should run for this location. If they
don't
> complete their injection (for example, waiting for a permission response) then
> they will be added to |pending_injections_|.

Done.

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:75: *iter, web_frame, tab_id,
run_location, document_url, extension);
On 2014/06/16 21:35:49, kalman wrote:
> Have you considered segmenting |scripts_| into 3 lists, start|end|idle?

yeah, but that's just messy.
- Scripts can execute at multiple locations if they have both javascript (which
can be any) and css (which is always start).
- We'd have to send out all three lists in OnUserScriptsUpdated().

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

https://codereview.chromium.org/321993003/diff/180001/extensions/renderer/use...
extensions/renderer/user_script_set.h:32: // itself from SharedMemory and also
knows how to create ScriptInjections for
On 2014/06/16 21:35:49, kalman wrote:
> s/also knows how to//

Done.

[not at google - send to devlin, 2014-06-18 18:59:33]

lgtm

[Devlin, 2014-06-23 18:28:53]

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

[commit-bot: I haz the power, 2014-06-23 18:29:32]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/321993003/...

[commit-bot: I haz the power, 2014-06-23 21:44:28]

Change committed as 279183