Make app_custom_bindings.js lazily evaluated so it doesn't execute on every page load.

Make app and webstore bindings lazily evaluated so they don't execute JS on every page load.


BUG=104100
TEST=existing browser tests

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

[koz (OOO until 15th September), 2012-03-23 05:21:55]

As per kalman's suggestion I've merged
https://chromiumcodereview.appspot.com/9838044/ into this change.

[not at google - send to devlin, 2012-03-25 22:42:58]

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:125:
module_system->SetLazyField(chrome, "appNotifications", "app",
add TODO to pull appNotifications out into its own lazy field?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:137: }
I guess this isn't scalable to arbitrary APIs.

Not worth worrying about this now since any further optimisation would only
apply to extensions -- which have a high likelihood of using APIs which they
request permission for (with the exception of automatically-granted APIs, so
maybe somewhat useful).

I guess we can just handwrite any further APIs opened up to all webpages here,
but it would be nice to have a mechanism arbitrarily able to be generated.

Probably worth noting that or adding a TODO?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:491: 
could just have the typedef here?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:494:
optimized_bindings["webstore"] = InstallWebstoreBindings;
lazy_bindings?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:538: if
(optimized_bindings.count(*i) > 0) {
nit: use find() here, ->second below.

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:541:
global->Get(v8::String::New("chrome"))->ToObject();
re-use the reference to "chrome" from the one created above? (line 501)

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:546: }
nit: else not continue

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.h (right):

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.h:109: v8::Handle<v8::Object>
chrome_hidden);
I don't think this needs to be in the header file (see comment in
extension_dispatcher.cc)

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
File chrome/renderer/module_system.cc (right):

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
chrome/renderer/module_system.cc:35: module_system_->set_natives_enabled(true);
This won't work with nested "natives enabled" scopes. Making it work with nested
scoped would be a bit of a pain, you'd need to make natives enabled a counter
rather than a boolean or something.

I would say "fine to just CHECK that natives is disabled" and that would be
equivalent if GetterRouter below can't be called while natives is enabled, but
is that true? What if an extension references "chrome.app" in its startup
sequence?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
chrome/renderer/module_system.cc:108:
parameters->Get(v8::String::New(kModuleName))->ToString())->ToObject();
(see comment above)

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
File chrome/renderer/module_system_unittest.cc (right):

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
chrome/renderer/module_system_unittest.cc:200: "assert.AssertTrue(object.thing.x
== 5);"
Once again, could you have a global "count" and check that it's undefined before
evaluating object.thing, and 1 thereafter?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
chrome/renderer/module_system_unittest.cc:201: "assert.AssertTrue(object.thing.y
== 10);"
I don't see the advantage in having x and y be both integers. Perhaps to mix
things up make y a function that returns 10?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/module_syste...
chrome/renderer/module_system_unittest.cc:220: "assert.AssertTrue(object.thing.x
== 5);");
If this was instead

var x = object.thing.x;
var assert = requireNative('assert');
assert.AssertTrue(x == 5);

would that trigger the possibly-a-bug that I mentioned earlier?

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

http://codereview.chromium.org/9835039/diff/5008/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/app_custom_bindings.js:56:
exports.chromeApp = app;
could you add like "must match the names in extension_dispatcher.cc" or
something like that (same for webstore_custom_bindings.js).

[koz (OOO until 15th September), 2012-03-26 01:36:09]

Bah, it looks like I commit the wrong patch the other day, so I lost some of
your comments from the previous code review. I've added them here.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:125:
module_system->SetLazyField(chrome, "appNotifications", "app",
On 2012/03/25 22:42:58, kalman wrote:
> add TODO to pull appNotifications out into its own lazy field?

The 'app' module has variables in it that are shared between
chrome.appNotifications and chromeHidden.app, so they need to come from the same
module.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:137: }
On 2012/03/25 22:42:58, kalman wrote:
> I guess this isn't scalable to arbitrary APIs.
> 
> Not worth worrying about this now since any further optimisation would only
> apply to extensions -- which have a high likelihood of using APIs which they
> request permission for (with the exception of automatically-granted APIs, so
> maybe somewhat useful).
> 
> I guess we can just handwrite any further APIs opened up to all webpages here,
> but it would be nice to have a mechanism arbitrarily able to be generated.
> 
> Probably worth noting that or adding a TODO?

You mean being able to make arbitrary modules lazy without having custom code
like this? It could be possible with a convention, but that seems like more
magic than its worth.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:491: 
On 2012/03/25 22:42:58, kalman wrote:
> could just have the typedef here?

Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:494:
optimized_bindings["webstore"] = InstallWebstoreBindings;
On 2012/03/25 22:42:58, kalman wrote:
> lazy_bindings?

Nice. Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:538: if
(optimized_bindings.count(*i) > 0) {
On 2012/03/25 22:42:58, kalman wrote:
> nit: use find() here, ->second below.

Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:541:
global->Get(v8::String::New("chrome"))->ToObject();
On 2012/03/25 22:42:58, kalman wrote:
> re-use the reference to "chrome" from the one created above? (line 501)

I think better to not. This method is already way too long so better to have
each part as modular as possible.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:546: }
On 2012/03/25 22:42:58, kalman wrote:
> nit: else not continue

Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
File chrome/renderer/extensions/extension_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.h:109: v8::Handle<v8::Object>
chrome_hidden);
On 2012/03/25 22:42:58, kalman wrote:
> I don't think this needs to be in the header file (see comment in
> extension_dispatcher.cc)

Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
File chrome/renderer/module_system.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
chrome/renderer/module_system.cc:35: module_system_->set_natives_enabled(true);
On 2012/03/25 22:42:58, kalman wrote:
> This won't work with nested "natives enabled" scopes. Making it work with
nested
> scoped would be a bit of a pain, you'd need to make natives enabled a counter
> rather than a boolean or something.
> 
> I would say "fine to just CHECK that natives is disabled" and that would be
> equivalent if GetterRouter below can't be called while natives is enabled, but
> is that true? What if an extension references "chrome.app" in its startup
> sequence?

Good point. I've made it a counter and removed the set_natives_enabled() method.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
chrome/renderer/module_system.cc:108:
parameters->Get(v8::String::New(kModuleName))->ToString())->ToObject();
On 2012/03/25 22:42:58, kalman wrote:
> (see comment above)

Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
File chrome/renderer/module_system_unittest.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:200: "assert.AssertTrue(object.thing.x
== 5);"
On 2012/03/25 22:42:58, kalman wrote:
> Once again, could you have a global "count" and check that it's undefined
before
> evaluating object.thing, and 1 thereafter?

Done in a separate test.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:201: "assert.AssertTrue(object.thing.y
== 10);"
On 2012/03/25 22:42:58, kalman wrote:
> I don't see the advantage in having x and y be both integers. Perhaps to mix
> things up make y a function that returns 10?

Done.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:220: "assert.AssertTrue(object.thing.x
== 5);");
On 2012/03/25 22:42:58, kalman wrote:
> If this was instead
> 
> var x = object.thing.x;
> var assert = requireNative('assert');
> assert.AssertTrue(x == 5);
> 
> would that trigger the possibly-a-bug that I mentioned earlier?

Yeah, it would if you wrap it in a NativesEnabledScope. I'll add that as a
separate test.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/reso...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/reso...
chrome/renderer/resources/extensions/app_custom_bindings.js:56:
exports.chromeApp = app;
On 2012/03/25 22:42:58, kalman wrote:
> could you add like "must match the names in extension_dispatcher.cc" or
> something like that (same for webstore_custom_bindings.js).

Done.

[not at google - send to devlin, 2012-03-26 02:29:49]

lgtm

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:125:
module_system->SetLazyField(chrome, "appNotifications", "app",
On 2012/03/26 01:36:09, koz wrote:
> On 2012/03/25 22:42:58, kalman wrote:
> > add TODO to pull appNotifications out into its own lazy field?
> 
> The 'app' module has variables in it that are shared between
> chrome.appNotifications and chromeHidden.app, so they need to come from the
same
> module.

I don't think they do really; rather I think that stuff is incorrectly on
chromeHidden.app when it should be on chromeHidden.appNotifications.

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:137: }
On 2012/03/26 01:36:09, koz wrote:
> On 2012/03/25 22:42:58, kalman wrote:
> > I guess this isn't scalable to arbitrary APIs.
> > 
> > Not worth worrying about this now since any further optimisation would only
> > apply to extensions -- which have a high likelihood of using APIs which they
> > request permission for (with the exception of automatically-granted APIs, so
> > maybe somewhat useful).
> > 
> > I guess we can just handwrite any further APIs opened up to all webpages
here,
> > but it would be nice to have a mechanism arbitrarily able to be generated.
> > 
> > Probably worth noting that or adding a TODO?
> 
> You mean being able to make arbitrary modules lazy without having custom code
> like this? It could be possible with a convention, but that seems like more
> magic than its worth.

Yeah agreed.

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
File chrome/renderer/module_system.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system.cc:40: CHECK(module_system_->natives_enabled_ >=
0);
CHECK_GE(natives_enabled, 0) would give better logs (if indeed this could fail)

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
File chrome/renderer/module_system.h (right):

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system.h:49: ModuleSystem* module_system_;
Come to think of it this is still subtly flawed, since copying or assigning one
of these won't increment the counter, but destruction will still decrement it.
So maybe you should DISALLOW_COPY_AND_ASSIGN (and come to think of it so should
ModuleSystem).

Or... you could do something totally sweet and make those increment the counter,
and then have ModuleSystem construct and return its own NativesEnabledScopes!

Just kidding. Kinda.

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
File chrome/renderer/module_system_unittest.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:41: : counter_(0) {
nit: on same line as CounterNatives()

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:115: void ExpectNoAssertionsMade() {
This is an unfortunate name in the context of gtest, where I read this initially
as EXPECT_FALSE(assertions_made).

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

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/5008/chrome/renderer/exte...
chrome/renderer/extensions/extension_dispatcher.cc:125:
module_system->SetLazyField(chrome, "appNotifications", "app",
On 2012/03/26 02:29:49, kalman wrote:
> On 2012/03/26 01:36:09, koz wrote:
> > On 2012/03/25 22:42:58, kalman wrote:
> > > add TODO to pull appNotifications out into its own lazy field?
> > 
> > The 'app' module has variables in it that are shared between
> > chrome.appNotifications and chromeHidden.app, so they need to come from the
> same
> > module.
> 
> I don't think they do really; rather I think that stuff is incorrectly on
> chromeHidden.app when it should be on chromeHidden.appNotifications.

Ah, true. Though looking at the code I see that there's already a TODO for you
to split that out into its own module (which implies it will have its own lazy
field).

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
File chrome/renderer/module_system.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system.cc:40: CHECK(module_system_->natives_enabled_ >=
0);
On 2012/03/26 02:29:49, kalman wrote:
> CHECK_GE(natives_enabled, 0) would give better logs (if indeed this could
fail)

Done.

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
File chrome/renderer/module_system.h (right):

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system.h:49: ModuleSystem* module_system_;
On 2012/03/26 02:29:49, kalman wrote:
> Come to think of it this is still subtly flawed, since copying or assigning
one
> of these won't increment the counter, but destruction will still decrement it.
> So maybe you should DISALLOW_COPY_AND_ASSIGN (and come to think of it so
should
> ModuleSystem).
> 
> Or... you could do something totally sweet and make those increment the
counter,
> and then have ModuleSystem construct and return its own NativesEnabledScopes!
> 
> Just kidding. Kinda.

lol, maybe I'll just add the DISALLOW_COPY_AND_ASSIGN for now...

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
File chrome/renderer/module_system_unittest.cc (right):

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:41: : counter_(0) {
On 2012/03/26 02:29:49, kalman wrote:
> nit: on same line as CounterNatives()

Done.

https://chromiumcodereview.appspot.com/9835039/diff/7003/chrome/renderer/modu...
chrome/renderer/module_system_unittest.cc:115: void ExpectNoAssertionsMade() {
On 2012/03/26 02:29:49, kalman wrote:
> This is an unfortunate name in the context of gtest, where I read this
initially
> as EXPECT_FALSE(assertions_made).

Yeah. I can't think of a better name that's not super verbose though.

[Aaron Boodman, 2012-03-26 04:09:57]

lgtm

[commit-bot: I haz the power, 2012-03-26 22:40:05]

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

[commit-bot: I haz the power, 2012-03-26 23:53:15]

Try job failure for 9835039-8003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-27 02:41:30]

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

[commit-bot: I haz the power, 2012-03-27 04:50:33]

Try job failure for 9835039-8003 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-27 04:57:54]

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

[commit-bot: I haz the power, 2012-03-27 07:17:25]

Try job failure for 9835039-8003 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-27 07:26:49]

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

[commit-bot: I haz the power, 2012-03-27 08:45:16]

Change committed as 129162