Move most of ChromeV8Context to a base ScriptContext

Move most of ChromeV8Context to a base ScriptContext

This also moves ModuleSystem, RequestSender,
ObjectBackedNativeHandler, and SafeBuiltins
over to //extensions.

Other renderer code is updated minimally to support
these changes.

BUG=359836
TBR=sky@chromium.org for chrome/test
TBR=jamescook@chromium.org for apps/shell
TBR=maruel@chromium.org for PRESUBMIT.py

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

[Ken Rockot(use gerrit already), 2014-04-11 05:12:09]

Ok, here's an alternative approach to the other CL. No interface separation. It
moves most of ChromeV8Context into ScriptContext, and also brings ModuleSystem
et al along with it.

The negative here is it touches a lot more files and there's an annoying albeit
safe downcast in the pepper code (ScriptContext -> ChromeV8Context).

Positive is no weird intermediate abstractions. LMKWYT.

[not at google - send to devlin, 2014-04-11 15:38:55]

yep much happier about this change.

https://codereview.chromium.org/234413005/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/234413005/diff/1/PRESUBMIT.py#newcode244
PRESUBMIT.py:244: r'extensions[/\\]renderer[/\\]safe_builtins\.*',
what does this change mean?

https://codereview.chromium.org/234413005/diff/1/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/234413005/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_v8_context.h:33: class ChromeV8Context :
public ScriptContext {
I'm more positive about this change. It seems well-possible that the rest of the
functionality in here, with the right changes to how request sending is done,
could mean deleting this class entirely.

https://codereview.chromium.org/234413005/diff/1/chrome/renderer/extensions/p...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/234413005/diff/1/chrome/renderer/extensions/p...
chrome/renderer/extensions/pepper_request_natives.cc:29: ChromeV8Context*
chrome_context = static_cast<ChromeV8Context*>(context());
could you add a note here explaining this? maybe file a bug about factoring the
Source stuff better with request sender and referencing it here?

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/console.cc
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/console....
extensions/renderer/console.cc:43:
helper->dispatcher()->v8_context_set().GetByV8Context(context_)) {
cool. it seems like you should eventually be able to pull dispatcher out of the
equation here and remove the chrome dependencies easily enough.

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/module_s...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/module_s...
extensions/renderer/module_system.cc:56: if (GetCurrentChannel() <=
chrome::VersionInfo::CHANNEL_DEV)
you could abstract away this decision somewhere easily enough I suppose.

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/script_c...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/script_c...
extensions/renderer/script_context.h:97: virtual GURL GetURL() const = 0;
you could move the GetDataSourceURLForFrame method into a utility basically
anywhere. It's arbitrarily on UserScriptSlave. You could even move it in here,
then make GetURL use that.

Up to you whether to do that in this change, but it would mean not needing to
make this pure-virtual, since it's pretty weird that this needs to be
pure-virtual.

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/script_c...
extensions/renderer/script_context.h:103: const extensions::Feature& api) = 0;
either way it looks like this doesn't need to be pure-virtual since it can use
GetURL() internally.

[Ken Rockot(use gerrit already), 2014-04-11 17:02:18]

Rad.

https://codereview.chromium.org/234413005/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/234413005/diff/1/PRESUBMIT.py#newcode244
PRESUBMIT.py:244: r'extensions[/\\]renderer[/\\]safe_builtins\.*',
On 2014/04/11 15:38:55, kalman wrote:
> what does this change mean?

It's a whitelist for the moved code. Spoke with jochen@ - moving existing code
which uses v8::Extension is OK, but the whitelist is necessary for it to pass
presubmit.

https://codereview.chromium.org/234413005/diff/1/chrome/renderer/extensions/p...
File chrome/renderer/extensions/pepper_request_natives.cc (right):

https://codereview.chromium.org/234413005/diff/1/chrome/renderer/extensions/p...
chrome/renderer/extensions/pepper_request_natives.cc:29: ChromeV8Context*
chrome_context = static_cast<ChromeV8Context*>(context());
On 2014/04/11 15:38:55, kalman wrote:
> could you add a note here explaining this? maybe file a bug about factoring
the
> Source stuff better with request sender and referencing it here?

Done.

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/module_s...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/module_s...
extensions/renderer/module_system.cc:56: if (GetCurrentChannel() <=
chrome::VersionInfo::CHANNEL_DEV)
On 2014/04/11 15:38:55, kalman wrote:
> you could abstract away this decision somewhere easily enough I suppose.

Yep, that's the plan

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/script_c...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/script_c...
extensions/renderer/script_context.h:97: virtual GURL GetURL() const = 0;
On 2014/04/11 15:38:55, kalman wrote:
> you could move the GetDataSourceURLForFrame method into a utility basically
> anywhere. It's arbitrarily on UserScriptSlave. You could even move it in here,
> then make GetURL use that.
> 
> Up to you whether to do that in this change, but it would mean not needing to
> make this pure-virtual, since it's pretty weird that this needs to be
> pure-virtual.

Done.

https://codereview.chromium.org/234413005/diff/1/extensions/renderer/script_c...
extensions/renderer/script_context.h:103: const extensions::Feature& api) = 0;
On 2014/04/11 15:38:55, kalman wrote:
> either way it looks like this doesn't need to be pure-virtual since it can use
> GetURL() internally.

Done.

[Ken Rockot(use gerrit already), 2014-04-11 17:05:27]

https://codereview.chromium.org/234413005/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/234413005/diff/20001/extensions/renderer/scri...
extensions/renderer/script_context.cc:171: const std::string& error) {
No reason this shouldn't have been moved already anyway. I think it was only
left in ChromeV8Context from before I convinced myself to move ModuleSystem in
this CL.

[not at google - send to devlin, 2014-04-11 20:22:17]

lgtm

[Ken Rockot(use gerrit already), 2014-04-11 21:41:08]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-04-11 21:41:16]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-04-11 21:43:31]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-11 21:44:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/234413005/2

[commit-bot: I haz the power, 2014-04-11 23:07:50]

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

[commit-bot: I haz the power, 2014-04-11 23:07:51]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Ken Rockot(use gerrit already), 2014-04-11 23:10:31]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-11 23:11:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/234413005/2

[commit-bot: I haz the power, 2014-04-12 00:50:05]

Change committed as 263429