Extract executeScript-like functionality into a single ExtensionScriptExecutor class.

Extract executeScript-like functionality into a single ExtensionScriptExecutorImpl class,
and convert all but one* existing uses of ExtensionMsg_ExecuteCode_Params to use it.

This is so that we can extend the implementation of ExtensionScriptExecutor to notify
the UI when extensions have executed scripts, and potentially have control over whether
scripts are actually executed.

* the file not converted is chrome/browser/chromeos/accessibility/accessibility_util.cc
since it has its own script loading logic and I don't feel the need to change that.
Plus, we probably don't want any special script execution logic applying to the loading
of chromevox.

BUG=127988
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136831

[not at google - send to devlin, 2012-05-10 12:55:28]

aa: extension stuff
avi: chrome/browser/ui/tab_contents stuff

More concretely than the commit message: my next step will be to add a class
which manages what page actions to show in the UI, and make that implement
ExtensionScriptExecutor (among other things).  The actual behaviour will be
delegated to ExtensionScriptExecutorImpl, but on success it will show a
notification in the page action area.  Simple!

(all behind a flag, of course)

[not at google - send to devlin, 2012-05-10 13:09:16]

On seconds thoughts maybe the chromevox stuff _should_ go through this class.
+dmazzoni until tomorrow.

[not at google - send to devlin, 2012-05-11 00:32:05]

.

[not at google - send to devlin, 2012-05-11 00:41:55]

.

[not at google - send to devlin, 2012-05-11 00:43:22]

On third thoughts, not going to touch the stuff in accessibility. Commit message
has been updated to explain why.

Also mpcomplete to review rather than aa.

http://codereview.chromium.org/10383104/diff/9001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/10383104/diff/9001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:818: width =
size.width();
hah found a bug!

[koz (OOO until 15th September), 2012-05-11 01:09:15]

lgtm

Cool change.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:818: width =
size.width();
Good spot!

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.h:125: TabContentsWrapper*
tab_contents_;
I was going to suggest renaming this to tab_contents_wrapper_ (which would
create a lot of wrapping, ho!), but apparently TabContentsWrapper is going to be
renamed to TabContents, so maybe this name is okay.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_executor.h (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor.h:20: // Interface for executing
extension content scripts (e.g. executeScript) as
Woah, an interface! What is this, Java? :-P

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor.h:36: virtual void ExecuteScript(const
std::string& extension_id,
I notice that the call sites here become less readable because these parameters
are now unnamed. Up to you but I think it wouldn't be the end of the world if
this took an ExtensionMsg_ExecuteCode_Params. It would also mean you don't have
to worry about the arguments here matching up with those in the IPC struct.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_executor_impl.cc (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor_impl.cc:23: class Handler : public
content::WebContentsObserver {
Could you please add a comment here explaining the lifetime of this class?

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor_impl.cc:32: Observe(web_contents);
Maybe a comment like

"Wait for WebContentsDestroyed() where we delete ourselves."

[Avi (use Gerrit), 2012-05-11 04:21:42]

lgtm with comment

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:17: #include
"chrome/browser/extensions/script_executor.h"
You don't use this file directly; you can skip including it.

[Matt Perry, 2012-05-11 22:47:38]

cool, lgtm

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_executor_impl.cc (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor_impl.cc:21: const char*
kRendererDestroyed = "Renderer destroyed";
maybe "The tab was closed." would be more human readable.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:236:
scoped_ptr<extensions::ScriptExecutor> extensions_script_executor_;
nit: extension_script_executor_ for consistency

[Matt Perry, 2012-05-11 22:49:01]

Also, don't forget to update the BUG number.

BTW, TEST= is for QA, so that message isn't very useful. See
https://sites.google.com/a/chromium.org/dev/developers/contributing-code?pli=...

[not at google - send to devlin, 2012-05-14 02:26:47]

make enums, address other comments

[not at google - send to devlin, 2012-05-14 02:27:16]

going to rebase then submit

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.h:125: TabContentsWrapper*
tab_contents_;
On 2012/05/11 01:09:16, koz wrote:
> I was going to suggest renaming this to tab_contents_wrapper_ (which would
> create a lot of wrapping, ho!), but apparently TabContentsWrapper is going to
be
> renamed to TabContents, so maybe this name is okay.

Yeah, seems to be pretty standard thing to call this. That, or "wrapper" or
"contents" or...

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_executor.h (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor.h:20: // Interface for executing
extension content scripts (e.g. executeScript) as
On 2012/05/11 01:09:16, koz wrote:
> Woah, an interface! What is this, Java? :-P

I wish!

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor.h:36: virtual void ExecuteScript(const
std::string& extension_id,
On 2012/05/11 01:09:16, koz wrote:
> I notice that the call sites here become less readable because these
parameters
> are now unnamed. Up to you but I think it wouldn't be the end of the world if
> this took an ExtensionMsg_ExecuteCode_Params. It would also mean you don't
have
> to worry about the arguments here matching up with those in the IPC struct.

I agree these parameters are confusing; particularly the bools. I don't want to
pass in the IPC directly because there are some fields I'll want to pull out and
treat separately (e.g. extension_id) and some that only make sense to set from
within here (e.g. request_id).

I'll make all the bools into enums instead. That should make things much nicer.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_executor_impl.cc (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor_impl.cc:21: const char*
kRendererDestroyed = "Renderer destroyed";
On 2012/05/11 22:47:38, Matt Perry wrote:
> maybe "The tab was closed." would be more human readable.

Done.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor_impl.cc:23: class Handler : public
content::WebContentsObserver {
On 2012/05/11 01:09:16, koz wrote:
> Could you please add a comment here explaining the lifetime of this class?

Done.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor_impl.cc:32: Observe(web_contents);
On 2012/05/11 01:09:16, koz wrote:
> Maybe a comment like
> 
> "Wait for WebContentsDestroyed() where we delete ourselves."

It would be OnMessageReceived too, but yeah, realised I can just do this
automatically in the constructor/destructor by calling the WebContentsObserver
constructor properly.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:17: #include
"chrome/browser/extensions/script_executor.h"
On 2012/05/11 04:21:42, Avi wrote:
> You don't use this file directly; you can skip including it.

Done.

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10383104/diff/8002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:236:
scoped_ptr<extensions::ScriptExecutor> extensions_script_executor_;
On 2012/05/11 22:47:38, Matt Perry wrote:
> nit: extension_script_executor_ for consistency

Done.

[not at google - send to devlin, 2012-05-14 02:28:18]

rebase

[commit-bot: I haz the power, 2012-05-14 02:28:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10383104/1026

[commit-bot: I haz the power, 2012-05-14 05:52:00]

Change committed as 136831