Implement a module system for the extension bindings JS.

Implement a module system for the extension bindings JS.


BUG=104100
TEST=existing browser tests


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

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

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

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

[koz (OOO until 15th September), 2012-02-13 06:42:39]

This change removes ExtensionDispatcher and friends' dependency on
v8::Extensions and introduces an implementation of the node.js module system
which revolves around use of a require() function. By side-stepping the
v8::Extension mechanism this change also brings the benefit of allowing
ChromeV8Extensions to have the same life-cycle as ChromeV8Contexts, and allows
us to handle native function calls from JS in instance methods instead of static
functions.

The logic for conditionally allowing v8::Extensions to be injected has been
lifted into JS in the bootstrap.js file.

I've tried to keep this change as small as possible for ease of reviewing and
merging, but I'm happy to include more changes as you guys see fit.

[koz (OOO until 15th September), 2012-02-13 07:07:55]

Also note that this change relies on an uncommitted WebKit change to plumb
extension_group into DidCreateScriptContext().

[Aaron Boodman, 2012-02-13 23:24:14]

In general, I like the direction this is going.

But I'm finding it hard to review in detail, because of the way that
ChromeV8Extension is now halfway between two worlds. It still subclasses
v8::Extension and has lots of bits of the old design in it.

Can you make this first review easier by creating a new peer to
ChromeV8Extension that will eventually replace it? ChromeV8Extension was a
terrible naming idea anyway, so this is an opportunity to fix that. Maybe
extensions::Binding or something...

Anyway, make a new peer to ChromeV8Extension that will eventually replace it,
and implement the new design cleanly from scratch in that class. Implement one
binding as a sample/test and send that out.

Once we get the core checked in, then you can have a subsequent CL that updates
all the bindings to use the new system and deletes ChromeV8Extension.

[Aaron Boodman, 2012-02-13 23:25:31]

On 2012/02/13 23:24:14, Aaron Boodman wrote:
> In general, I like the direction this is going.
> 
> But I'm finding it hard to review in detail, because of the way that
> ChromeV8Extension is now halfway between two worlds. It still subclasses
> v8::Extension and has lots of bits of the old design in it.
> 
> Can you make this first review easier by creating a new peer to
> ChromeV8Extension that will eventually replace it? ChromeV8Extension was a
> terrible naming idea anyway, so this is an opportunity to fix that. Maybe
> extensions::Binding or something...
> 
> Anyway, make a new peer to ChromeV8Extension that will eventually replace it,
> and implement the new design cleanly from scratch in that class. Implement one
> binding as a sample/test and send that out.
> 
> Once we get the core checked in, then you can have a subsequent CL that
updates
> all the bindings to use the new system and deletes ChromeV8Extension.

I'm hoping that this strategy will allow us to focus tighter on the review of
just the core of this new system, which is the important bit. Once we're all
happy with it, then you can just bash through the  porting of all the bindings
and Ben and I can pay less attention to those changes.

[Aaron Boodman, 2012-02-16 01:54:42]

Pretty sweet.

I need to think some more about the permissions aspect...

It would be nice if there was a stronger connection between native handlers and
namespace, or permission, or something. So that each handler didn't have to be
careful to check permissions manually.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/app_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/app_bindings.cc:81: if (name == "GetIsInstalled") {
It would be really nice to avoid this dance in every ChromeV8Extension impl. It
should be possible to hold a map of name->member function pointer in the base
class. It's kind of a mess of template crap, but the advantage is that
subclasses can just say:

Register("foo", &MyClass::DoFoo)

and DoFoo magically gets called.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/app_bindings.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/app_bindings.h:26: class AppBindings : public
ChromeV8Extension, public ChromeV8ExtensionHandler {
This is multiple inheritance, which is typically discouraged in Chrome. Is there
a way to factor this such that the handler is a member of the extension?

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_context.cc:33:
natives_(v8::Persistent<v8::Object>::New(v8::Object::New())),
Seems like this object should be created in v8_context_. In order to do that you
will need to create it in the constructor inside a v8::Context::Scope for
v8_context_.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_context.cc:45:
STLDeleteElements(&extensions_);
Use base/memory/linked_ptr instead of doing this manually.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_context.h:85: void
RegisterExtension(ChromeV8Extension* extension);
Needs comments.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_context.h:99: v8::Persistent<v8::Object>
natives_;
needs comments.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_extension.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.cc:23: #include <iostream>
These seem unneeded.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_extension.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:29: class ChromeV8Extension :
public v8::Extension {
The inheritance on v8::Extension no longer makes sense.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:29: class ChromeV8Extension :
public v8::Extension {
Now that this is no longer inheriting from v8::Extension, it makes even less
sense to have the conflicting name. Could we rename it Binding or something?

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:37: ChromeV8Extension(const
char* name,
Do we still use the name and dependency params?

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:46: void
ContextWillBeReleased(ChromeV8Context* context);
This might no longer be needed.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:60: static
v8::Handle<v8::Value> Router(const v8::Arguments& args);
This should not be exposed as interface. It can be in an anonymous namespace in
the cc file.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:82: void
RouteFunctionHere(const char* name, v8::Handle<v8::Object> object);
I think something like AddNativeFunction would be a more clear name. Also the
params seem more naturally ordered: object, name, handler.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_extension_handler.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension_handler.h:18: class
ChromeV8ExtensionHandler : public IPC::Channel::Listener {
There should at least be a TODO to rename this class since it no longer has
anything to do with 'handling' chrome v8 extensions. It's just a handy base
class for things that want to do routed IPC.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:7: var
GetChromeHidden = natives.GetChromeHidden;
I'm still not crazy about the magical names. Did you try my suggestion of
wrapping in a function?

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/chrome_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/chrome_private_custom_bindings.js:7: var
GetChromeHidden = natives.GetChromeHidden;
As long as you're changing these files, you might as well just use natives.Foo
directly instead of creating this alias. Most of them are only used once.

Also, since we're not using the fancy v8 native function technology anymore,
distinguishing these functions specially with a leading capital letter doesn't
seem worthwhile. It would look nicer to name them the same as any other
function, with camelCase.

[Aaron Boodman, 2012-02-16 01:55:32]

On 2012/02/16 01:54:42, Aaron Boodman wrote:
> Pretty sweet.
> 
> I need to think some more about the permissions aspect...
> 
> It would be nice if there was a stronger connection between native handlers
and
> namespace, or permission, or something. So that each handler didn't have to be
> careful to check permissions manually.

This was meant to be sent to the newer review, sorry.

[koz (OOO until 15th September), 2012-02-27 01:06:14]

Thanks aa!

Now that ModuleSystem has landed, this patch is substantially changed. I've
responded to the comments that are still applicable.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/app_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/app_bindings.cc:81: if (name == "GetIsInstalled") {
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> It would be really nice to avoid this dance in every ChromeV8Extension impl.
It
> should be possible to hold a map of name->member function pointer in the base
> class. It's kind of a mess of template crap, but the advantage is that
> subclasses can just say:
> 
> Register("foo", &MyClass::DoFoo)
> 
> and DoFoo magically gets called.

The new ModuleSystem handles this.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/app_bindings.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/app_bindings.h:26: class AppBindings : public
ChromeV8Extension, public ChromeV8ExtensionHandler {
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> This is multiple inheritance, which is typically discouraged in Chrome. Is
there
> a way to factor this such that the handler is a member of the extension?

It's possible, but it would be quite messy. It's probably better to merge
ChromeV8ExtensionHandler into ChromeV8Extension, for which I've added a TODO.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_context.cc:45:
STLDeleteElements(&extensions_);
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> Use base/memory/linked_ptr instead of doing this manually.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_extension.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.cc:23: #include <iostream>
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> These seem unneeded.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_extension.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:29: class ChromeV8Extension :
public v8::Extension {
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> Now that this is no longer inheriting from v8::Extension, it makes even less
> sense to have the conflicting name. Could we rename it Binding or something?

I've added a TODO for this.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:37: ChromeV8Extension(const
char* name,
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> Do we still use the name and dependency params?

Removed.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:46: void
ContextWillBeReleased(ChromeV8Context* context);
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> This might no longer be needed.

Removed.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension.h:60: static
v8::Handle<v8::Value> Router(const v8::Arguments& args);
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> This should not be exposed as interface. It can be in an anonymous namespace
in
> the cc file.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_v8_extension_handler.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_v8_extension_handler.h:18: class
ChromeV8ExtensionHandler : public IPC::Channel::Listener {
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> There should at least be a TODO to rename this class since it no longer has
> anything to do with 'handling' chrome v8 extensions. It's just a handy base
> class for things that want to do routed IPC.

I've got a TODO to merge this into ChromeV8Extension (and a TODO to rename that
class).

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:7: var
GetChromeHidden = natives.GetChromeHidden;
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> I'm still not crazy about the magical names. Did you try my suggestion of
> wrapping in a function?

No - wrapping it in a function would definitely work, but I think it's just
extra boilerplate and an extra source of potential bugs. I think it's okay to
have these variables just appear in scope here - almost all JS environments
execute JS in a context where extra variables are available (eg: JS on the web,
JS in extensions, node) and they wouldn't be improved by requiring developers to
enumerate those variables as parameters in an anonymous function.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/chrome_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/chrome_private_custom_bindings.js:7: var
GetChromeHidden = natives.GetChromeHidden;
On 2012/02/16 01:54:42, Aaron Boodman wrote:
> As long as you're changing these files, you might as well just use natives.Foo
> directly instead of creating this alias. Most of them are only used once.
> 
> Also, since we're not using the fancy v8 native function technology anymore,
> distinguishing these functions specially with a leading capital letter doesn't
> seem worthwhile. It would look nicer to name them the same as any other
> function, with camelCase.

I don't know, I kind of like having the native functions defined up-front. And
there's just as much reason to have the names have a leading capital now as
there was before - they still denote native functions, they're just set up in
different ways.

[Aaron Boodman, 2012-02-28 02:42:59]

+mpcomplete - I'd like a second set of eyes on this.

As we discussed, please disable require() and requireNative() after the initial
setup. This is a good mitigation against leaking those functions.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
File chrome/renderer/extensions/app_bindings.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/extensi...
chrome/renderer/extensions/app_bindings.h:26: class AppBindings : public
ChromeV8Extension, public ChromeV8ExtensionHandler {
On 2012/02/27 01:06:14, koz wrote:
> On 2012/02/16 01:54:42, Aaron Boodman wrote:
> > This is multiple inheritance, which is typically discouraged in Chrome. Is
> there
> > a way to factor this such that the handler is a member of the extension?
> 
> It's possible, but it would be quite messy. It's probably better to merge
> ChromeV8ExtensionHandler into ChromeV8Extension, for which I've added a TODO.

In that case, I prefer the multiple inheritance.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:7: var
GetChromeHidden = natives.GetChromeHidden;
On 2012/02/27 01:06:14, koz wrote:
> On 2012/02/16 01:54:42, Aaron Boodman wrote:
> > I'm still not crazy about the magical names. Did you try my suggestion of
> > wrapping in a function?
> 
> No - wrapping it in a function would definitely work, but I think it's just
> extra boilerplate and an extra source of potential bugs. I think it's okay to
> have these variables just appear in scope here - almost all JS environments
> execute JS in a context where extra variables are available (eg: JS on the
web,
> JS in extensions, node) and they wouldn't be improved by requiring developers
to
> enumerate those variables as parameters in an anonymous function.

OK, I see your point of view.

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/chrome_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/1/chrome/renderer/resourc...
chrome/renderer/resources/extensions/chrome_private_custom_bindings.js:7: var
GetChromeHidden = natives.GetChromeHidden;
On 2012/02/27 01:06:14, koz wrote:
> On 2012/02/16 01:54:42, Aaron Boodman wrote:
> > As long as you're changing these files, you might as well just use
natives.Foo
> > directly instead of creating this alias. Most of them are only used once.
> > 
> > Also, since we're not using the fancy v8 native function technology anymore,
> > distinguishing these functions specially with a leading capital letter
doesn't
> > seem worthwhile. It would look nicer to name them the same as any other
> > function, with camelCase.
> 
> I don't know, I kind of like having the native functions defined up-front. And
> there's just as much reason to have the names have a leading capital now as
> there was before - they still denote native functions, they're just set up in
> different ways.

OK. Elsewhere in this review, I pointed out unix_hacker names also don't adhere
to our js conventions. How do you feel about changing those?

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/app_bindings.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/app_bindings.h:29: // IPC::Channel::Listener
There's no need to have this be public unless you intend to call it from tests
or something.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/chrome_v8_context.h:97: void
set_module_system(scoped_ptr<ModuleSystem> module_system) {
Should go up higher w/ other setters.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/chrome_v8_extension.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/chrome_v8_extension.h:30: // TODO(koz): Rename this
to ChromeNativeHandler.
What about "NativeJavaScriptModule"? (You could rename ModuleSystem to
JavaScriptModuleSystem)

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:294: bool
ExtensionDispatcher::AllowScriptExtension(
Can't this entire method and all related code be deleted now? We aren't use v8
extensions anymore.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:356: static
std::map<std::string, base::StringPiece>* source_map = NULL;
Sadly, we do not static non-pod types in Chrome. You have to use LazyInstance
instead. Search the codebase for examples.

Alternately, I believe only one instance of ExtensionDispatcher gets created
per-process. In that case, there is really no need for this to be static.
source_map could just be an instance member.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
File chrome/renderer/resources/extensions/chrome_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
chrome/renderer/resources/extensions/chrome_private_custom_bindings.js:9: var
chrome_private = requireNative('chrome_private');
We don't usually use unix_hacker style names in JavaScript.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
File chrome/renderer/resources/extensions/context_menus_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
chrome/renderer/resources/extensions/context_menus_custom_bindings.js:9: var
context_menus = requireNative('context_menus');
contextMenus? Or contextMenusModule?

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:6: var natives =
requireNative('event_bindings');
eventBindingsNatives?

[Matt Perry, 2012-02-28 20:34:19]

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/app_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/app_bindings.cc:64:
base::Bind(&AppBindings::GetIsInstalled, base::Unretained(this)));
indent should be 4 spaces

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/event_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/event_bindings.cc:55:
RouteStaticFunction("DetachEvent", &DetachEvent);
Both of these access the ExtensionImpl via
GetFromArguments<ExtensionImpl>(args); Would they not need to do so if they
became member functions, and used RouteFunction?

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/event_bindings.cc:162: return new
ExtensionImpl(dispatcher, listener_counts);
I don't understand this set of changes. Why change the ownership of
listener_counts? It will leak on shutdown. That may anger the memory bots. And
why return a new ExtensionImpl each time instead of caching it in a static var?

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:467:
scoped_ptr<NativeHandler>(new AppBindings(this, context)));
+2 indent

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:467:
scoped_ptr<NativeHandler>(new AppBindings(this, context)));
fyi: make_scoped_ptr(new X) looks nicer

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:471:
scoped_ptr<NativeHandler>(EventBindings::Get(this)));
+2 indent

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:535:
context->set_module_system(scoped_ptr<ModuleSystem>(module_system));
Why is there one module system per context, rather than one shared one per
process? Looks like every module system is the same.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/nat...
File chrome/renderer/native_handler.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/nat...
chrome/renderer/native_handler.cc:31: handler_functions_.push_back(function);
You use this pattern a few times. Do you know about V8's WeakReferences? You can
set up a callback for when a V8 object is garbage collected, so you can clean up
associated C++ data structures.

[koz (OOO until 15th September), 2012-03-01 03:41:55]

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/app_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/app_bindings.cc:64:
base::Bind(&AppBindings::GetIsInstalled, base::Unretained(this)));
On 2012/02/28 20:34:20, Matt Perry wrote:
> indent should be 4 spaces

Done.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/app_bindings.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/app_bindings.h:29: // IPC::Channel::Listener
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> There's no need to have this be public unless you intend to call it from tests
> or something.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/chrome_v8_context.h:97: void
set_module_system(scoped_ptr<ModuleSystem> module_system) {
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> Should go up higher w/ other setters.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/chrome_v8_extension.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/chrome_v8_extension.h:30: // TODO(koz): Rename this
to ChromeNativeHandler.
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> What about "NativeJavaScriptModule"? (You could rename ModuleSystem to
> JavaScriptModuleSystem)

Oh yeah, nice. How about:

ModuleSystem => JavaScriptModuleSystem
NativeHandler => NativeJavaScriptModule
ChromeV8Extension => ExtensionNativeModule

?

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/event_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/event_bindings.cc:55:
RouteStaticFunction("DetachEvent", &DetachEvent);
On 2012/02/28 20:34:20, Matt Perry wrote:
> Both of these access the ExtensionImpl via
> GetFromArguments<ExtensionImpl>(args); Would they not need to do so if they
> became member functions, and used RouteFunction?

Yes, that's right. I was planning on making those conversions in a separate
change to reduce the number of delta lines in this one.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/event_bindings.cc:162: return new
ExtensionImpl(dispatcher, listener_counts);
On 2012/02/28 20:34:20, Matt Perry wrote:
> I don't understand this set of changes. Why change the ownership of
> listener_counts? It will leak on shutdown. That may anger the memory bots. And
> why return a new ExtensionImpl each time instead of caching it in a static
var?

With this change ChromeV8Extensions are no longer singletons, so their state is
now per-context instead of per-process, so to preserve the semantics here
listener_counts_ needs to be shared across all the instances.

I've changed this to use LazyInstance (as per aa's comment elsewhere).

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:294: bool
ExtensionDispatcher::AllowScriptExtension(
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> Can't this entire method and all related code be deleted now? We aren't use v8
> extensions anymore.

Yes, it can. I've removed the custom binding permission check out into a
different function which is now invoked from the JS. Though this may be
redundant (because SGB::GetExtensionAPIDefinition() should only return apis that
are permitted in the calling context), it's isomorphic to what the code does
now.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:356: static
std::map<std::string, base::StringPiece>* source_map = NULL;
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> Sadly, we do not static non-pod types in Chrome. You have to use LazyInstance
> instead. Search the codebase for examples.
> 
> Alternately, I believe only one instance of ExtensionDispatcher gets created
> per-process. In that case, there is really no need for this to be static.
> source_map could just be an instance member.

I've made it an instance member.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:467:
scoped_ptr<NativeHandler>(new AppBindings(this, context)));
On 2012/02/28 20:34:20, Matt Perry wrote:
> +2 indent

Done.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:467:
scoped_ptr<NativeHandler>(new AppBindings(this, context)));
On 2012/02/28 20:34:20, Matt Perry wrote:
> fyi: make_scoped_ptr(new X) looks nicer

I think it doesn't work here because we need to upcast the arguments.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:471:
scoped_ptr<NativeHandler>(EventBindings::Get(this)));
On 2012/02/28 20:34:20, Matt Perry wrote:
> +2 indent

Done.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:535:
context->set_module_system(scoped_ptr<ModuleSystem>(module_system));
On 2012/02/28 20:34:20, Matt Perry wrote:
> Why is there one module system per context, rather than one shared one per
> process? Looks like every module system is the same.

ModuleSystem injects JS into a particular context and holds on to that context's
require() function, so it can be called through Require(), so ModuleSystem is
tied to the context it is created in. The code that ModuleSystems inject (the
source map) is shared between them.

Also, having each ModuleSystem be separate means that NativeHandlers are scoped
to each context, rather than being static, which means we can migrate away from
static methods in bindings classes towards member functions.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/nat...
File chrome/renderer/native_handler.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/nat...
chrome/renderer/native_handler.cc:31: handler_functions_.push_back(function);
On 2012/02/28 20:34:20, Matt Perry wrote:
> You use this pattern a few times. Do you know about V8's WeakReferences? You
can
> set up a callback for when a V8 object is garbage collected, so you can clean
up
> associated C++ data structures.

I didn't know about that, thanks. I've put a TODO to investigate using that
instead of a vector of linked_ptrs.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
File chrome/renderer/resources/extensions/chrome_private_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
chrome/renderer/resources/extensions/chrome_private_custom_bindings.js:9: var
chrome_private = requireNative('chrome_private');
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> We don't usually use unix_hacker style names in JavaScript.

Fixed here and elsewhere.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
File chrome/renderer/resources/extensions/context_menus_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
chrome/renderer/resources/extensions/context_menus_custom_bindings.js:9: var
context_menus = requireNative('context_menus');
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> contextMenus? Or contextMenusModule?

Done.

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/12001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:6: var natives =
requireNative('event_bindings');
On 2012/02/28 02:42:59, Aaron Boodman wrote:
> eventBindingsNatives?

Done, here and elsewhere.

[Aaron Boodman, 2012-03-01 07:24:25]

Ever closer.

On the upside, I feel like I really get this change now.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/chrome_v8_extension_handler.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/chrome_v8_extension_handler.h:17: // Base class for
context-scoped handlers used with ChromeV8Extension.
TODO(koz): Rename/refactor this somehow. Maybe just pull it into
ChromeV8Extension.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:320: base::StringPiece
GetResource(int resource_id) {
Dead code?

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:324: SourceMap*
GetOrCreateSourceMap() {
Dead code.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:334: class
ChromeHiddenNativeHandler : public NativeHandler {
Global definitions in a cc file should be in an anonymous namespace at top of
file.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:515: ModuleSystem*
module_system = new ModuleSystem(&source_map_);
Make this a scoped_ptr, and use module_system.Pass() below. That way if/when
this code gets refactored, it is less likely to become wrong.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
File chrome/renderer/module_system.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
chrome/renderer/module_system.h:52: void DisableInjection();
It's only GetNative() that needs to be disabled; this can become
set_natives_enabled().

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
chrome/renderer/module_system.h:55: typedef std::map<std::string,
linked_ptr<NativeHandler> > NativeHandlerMap;
.append("\n");

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
File chrome/renderer/resources/extensions/setup_bindings.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:11: require('app');
Why do this in JavaScript? It seems to end up being more complex than just doing
it here in C++. Control passes from C++ to a static chunk of JS, back into C++.
It also requires the ContextInfo native.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:15:
require('json_schema');
Have schema_generated_bindings just require this?

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:16:
require('event_bindings');
Have miscellaneous_bindings and schema_generated_bindings each require this?

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:22:
sgb.GetExtensionAPIDefinition().forEach(function(apiDef) {
I wonder if this could be part of SchemaGeneratedBindings. For each module, try
to load a custom bindings file for it.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
File chrome/renderer/resources/require.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/require.js:36: var wrappedSource = wrap(source);
Unless v8 does something fancy internally, it seems like this will end up making
a copy of the big string that you went to such trouble to avoid allocating in
the first place.

If you just required the modules to be wrapped in the source, it would avoid
this problem, and you could make the interface between require.js and
ModuleSystem a bit simpler by just having Run(id), not GetSource(id) and
Run(id).

Alternately, there could be some build-time magic to do the wrapping. Or, we
could just say screw it and allocate the strings once and cache them across the
process thereafter.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/sou...
File chrome/renderer/source_map.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/sou...
chrome/renderer/source_map.h:13: class SourceMap {
This interface is really just a detail of ModuleSystem. Nest it there?
Otherwise, it needs to be included in chrome_renderer.gypi.

[Aaron Boodman, 2012-03-01 07:30:32]

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
File chrome/renderer/resources/require.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/require.js:36: var wrappedSource = wrap(source);
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Unless v8 does something fancy internally, it seems like this will end up
making
> a copy of the big string that you went to such trouble to avoid allocating in
> the first place.
> 
> If you just required the modules to be wrapped in the source, it would avoid
> this problem, and you could make the interface between require.js and
> ModuleSystem a bit simpler by just having Run(id), not GetSource(id) and
> Run(id).
> 
> Alternately, there could be some build-time magic to do the wrapping. Or, we
> could just say screw it and allocate the strings once and cache them across
the
> process thereafter.

I feel pretty strongly that the right answer here is to wrap the code within the
source of each module. That would simplify things.

[Matt Perry, 2012-03-01 23:23:06]

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/event_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/event_bindings.cc:154: g_listener_counts =
LAZY_INSTANCE_INITIALIZER;
If this is going to be a global, why not just have ExtensionImpl access it
directly rather than passing a pointer to it.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:300: const std::string&
custom_binding_api_name,
indent is wonky here

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
File chrome/renderer/module_system.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
chrome/renderer/module_system.h:32: // object cached.
Could you add a quick comment that explains this is per-context? and the same in
NativeHandler?

[koz (OOO until 15th September), 2012-03-02 01:13:58]

That's good. Hopefully this means that this module system is easier to
understand than the v8::Extension stuff :-)

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/chrome_v8_extension_handler.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/chrome_v8_extension_handler.h:17: // Base class for
context-scoped handlers used with ChromeV8Extension.
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> TODO(koz): Rename/refactor this somehow. Maybe just pull it into
> ChromeV8Extension.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/event_bindings.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/event_bindings.cc:154: g_listener_counts =
LAZY_INSTANCE_INITIALIZER;
On 2012/03/01 23:23:06, Matt Perry wrote:
> If this is going to be a global, why not just have ExtensionImpl access it
> directly rather than passing a pointer to it.

Good point. Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:300: const std::string&
custom_binding_api_name,
On 2012/03/01 23:23:06, Matt Perry wrote:
> indent is wonky here

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:320: base::StringPiece
GetResource(int resource_id) {
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Dead code?

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:324: SourceMap*
GetOrCreateSourceMap() {
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Dead code.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:334: class
ChromeHiddenNativeHandler : public NativeHandler {
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Global definitions in a cc file should be in an anonymous namespace at top of
> file.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:515: ModuleSystem*
module_system = new ModuleSystem(&source_map_);
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Make this a scoped_ptr, and use module_system.Pass() below. That way if/when
> this code gets refactored, it is less likely to become wrong.

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
File chrome/renderer/module_system.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
chrome/renderer/module_system.h:32: // object cached.
On 2012/03/01 23:23:06, Matt Perry wrote:
> Could you add a quick comment that explains this is per-context? and the same
in
> NativeHandler?

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
chrome/renderer/module_system.h:52: void DisableInjection();
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> It's only GetNative() that needs to be disabled; this can become
> set_natives_enabled().

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/mod...
chrome/renderer/module_system.h:55: typedef std::map<std::string,
linked_ptr<NativeHandler> > NativeHandlerMap;
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> .append("\n");

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
File chrome/renderer/resources/extensions/setup_bindings.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:11: require('app');
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Why do this in JavaScript? It seems to end up being more complex than just
doing
> it here in C++. Control passes from C++ to a static chunk of JS, back into
C++.
> It also requires the ContextInfo native.

I think it makes it easier to understand what's happening this way - all the
code that sets up bindings happens in setup_bindings.js. Better to set up some
native handlers, then jump in to JS to do the rest of the job than to have C++
require various JS files, which is a C++/JS dance anyway.

Besides, this may not always be so trivial. Right now all this file does is
require() a bunch of modules that perform work as a side-effect, but ideally
modules don't perform any work when they are require()d and it would be the
responsibility of this file to invoke the schema generation explicitly, which
would be much harder to do in C++ than in JS.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:15:
require('json_schema');
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Have schema_generated_bindings just require this?

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:16:
require('event_bindings');
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> Have miscellaneous_bindings and schema_generated_bindings each require this?

Done.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/extensions/setup_bindings.js:22:
sgb.GetExtensionAPIDefinition().forEach(function(apiDef) {
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> I wonder if this could be part of SchemaGeneratedBindings. For each module,
try
> to load a custom bindings file for it.

Yeah, it could be, but with this change I'd rather it be isomorphic to how it
works currently. Right now setup_bindings.js mirrors what happens with the
current v8::Extension based thing: load some modules which set things in
chromeHidden, then for each custom binding we check if we can inject that too,
then we dispatch an onLoad() event from C++ to have the generation actually
occur.

Right now there's no need to set things up as listeners on onLoad() as all this
code can be run synchronously, but I think that would be better as its own
isolated change.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
File chrome/renderer/resources/require.js (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/res...
chrome/renderer/resources/require.js:36: var wrappedSource = wrap(source);
On 2012/03/01 07:30:32, Aaron Boodman wrote:
> On 2012/03/01 07:24:26, Aaron Boodman wrote:
> > Unless v8 does something fancy internally, it seems like this will end up
> making
> > a copy of the big string that you went to such trouble to avoid allocating
in
> > the first place.
> > 
> > If you just required the modules to be wrapped in the source, it would avoid
> > this problem, and you could make the interface between require.js and
> > ModuleSystem a bit simpler by just having Run(id), not GetSource(id) and
> > Run(id).
> > 
> > Alternately, there could be some build-time magic to do the wrapping. Or, we
> > could just say screw it and allocate the strings once and cache them across
> the
> > process thereafter.
> 
> I feel pretty strongly that the right answer here is to wrap the code within
the
> source of each module. That would simplify things.

As discussed, I've reimplemented with "new Function(...)" on the JS side.

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/sou...
File chrome/renderer/source_map.h (right):

https://chromiumcodereview.appspot.com/9386001/diff/30001/chrome/renderer/sou...
chrome/renderer/source_map.h:13: class SourceMap {
On 2012/03/01 07:24:26, Aaron Boodman wrote:
> This interface is really just a detail of ModuleSystem. Nest it there?
> Otherwise, it needs to be included in chrome_renderer.gypi.

Done.

[Matt Perry, 2012-03-02 02:15:49]

http://codereview.chromium.org/9386001/diff/34004/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/apitest.js (right):

http://codereview.chromium.org/9386001/diff/34004/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/apitest.js:8: (function() {
This sort of extra wrapping can go away in all the files, right?

[koz (OOO until 15th September), 2012-03-02 02:23:06]

http://codereview.chromium.org/9386001/diff/34004/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/apitest.js (right):

http://codereview.chromium.org/9386001/diff/34004/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/apitest.js:8: (function() {
On 2012/03/02 02:15:49, Matt Perry wrote:
> This sort of extra wrapping can go away in all the files, right?

Yes, that's right. I've removed it from all the files, but I've left the
indenting the same for those that were indented, for blame's sake.

[not at google - send to devlin, 2012-03-02 02:23:59]

http://codereview.chromium.org/9386001/diff/34004/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/apitest.js (right):

http://codereview.chromium.org/9386001/diff/34004/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/apitest.js:8: (function() {
On 2012/03/02 02:23:06, koz wrote:
> On 2012/03/02 02:15:49, Matt Perry wrote:
> > This sort of extra wrapping can go away in all the files, right?
> 
> Yes, that's right. I've removed it from all the files, but I've left the
> indenting the same for those that were indented, for blame's sake.

As in, who to blame for unnecessary indentation?

[Aaron Boodman, 2012-03-02 03:14:44]

lgtm

land it

[Matt Perry, 2012-03-02 03:23:00]

lgtm

[commit-bot: I haz the power, 2012-03-06 04:23:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/43003

[commit-bot: I haz the power, 2012-03-06 07:16:35]

Change committed as 125132

[Dan Beam, 2012-03-06 09:20:18]

koz: I believe this CL has broken Win 7 Sync bots as they've been running
red/orange since your commit -
http://build.chromium.org/p/chromium/buildstatus?builder=Win7%20Sync&number=1....

Failing tests:
TwoClientSessionsSyncTest.SingleClientChanged
TwoClientSessionsSyncTest.SingleClientEnabledEncryptionAndChanged
TwoClientSessionsSyncTest.BothChanged
TwoClientSessionsSyncTest.FirstChangesAndSetsPassphrase
TwoClientSessionsSyncTest.FirstChangesWhileSecondWaitingForPassphrase
TwoClientSessionsSyncTest.SecondChangesAfterEncrAndPassphraseChange

[commit-bot: I haz the power, 2012-03-09 02:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/51001

[commit-bot: I haz the power, 2012-03-09 04:36:05]

Change committed as 125801

[commit-bot: I haz the power, 2012-03-12 22:52:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/59001

[koz (OOO until 15th September), 2012-03-12 22:53:54]

On 2012/03/12 22:52:01, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/koz%40chromium.org/9386001/59001

Third time's a charm...

[commit-bot: I haz the power, 2012-03-13 02:09:44]

Change committed as 126306

[commit-bot: I haz the power, 2012-03-16 00:31:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/67001

[commit-bot: I haz the power, 2012-03-16 00:32:06]

Can't apply patch for file chrome/renderer/extensions/miscellaneous_bindings.cc.
While running patch -p1 --forward --force;
patching file chrome/renderer/extensions/miscellaneous_bindings.cc
Hunk #1 FAILED at 62.
Hunk #2 succeeded at 186 (offset 31 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/renderer/extensions/miscellaneous_bindings.cc.rej

[commit-bot: I haz the power, 2012-03-16 00:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/71001

[commit-bot: I haz the power, 2012-03-16 04:43:57]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-03-16 04:44:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/71001

[commit-bot: I haz the power, 2012-03-16 04:46:14]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-03-16 04:50:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9386001/71001

[commit-bot: I haz the power, 2012-03-16 06:25:33]

Change committed as 127117

[Timur Iskhodzhanov, 2012-03-16 15:30:33]

FYI, this has introduced quite a few leaks:
http://code.google.com/p/chromium/issues/detail?id=118575