Prevent chrome.app JSON schema from loading on every page

Prevent chrome.app JSON schema from loading on every page

The app API along with app.window and app.runtime have been converted to use
the feature system. Bindings are not added to the chrome object for unavailable
APIs that are children of available APIs. For example, if chrome.app is
available, we will not add lazy bindings to chrome for app.window and
app.runtime. This eliminates the need to load the app schema, because we no
longer need to get chrome.app to add the app.runtime and app.window bindings.

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

[cduvall, 2013-04-04 01:07:20]

Just realized that web pages will still have to load _manifest_features.json and
_permission_features.json, since all the feature types are tied to the same
lazily initialized global. I'm also working on splitting these up, so only the
needed files are loaded (e.g. for web pages, only _api_feature.json). Do you
think I should finish that before we try register the bindings in web page
contexts again?

[not at google - send to devlin, 2013-04-04 01:30:26]

lgtm with nits

https://codereview.chromium.org/13604005/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/13604005/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/_api_features.json:9: "contexts":
["blessed_extension", "unblessed_extension", "content_script", "web_page"],
80 characters

https://codereview.chromium.org/13604005/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/_api_features.json:10: "matches": [ "<all_urls>" ]
no spaces around this (sorry...)

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:692: std::string current_name;
ok... I think I understand what you're doing here. If an API has a parent, check
its availability and all its parents' availability. If the parent is available
and it isn't, don't install the bindings, but leave the object there.

Maybe you could explain something along those lines in the comment. It's pretty
subtle. And give the canonical example (apps) - well actually that's the only
example right?

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:693: bool only_parent_available =
false;
only_ancestor_available?

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:695: current_name += (i ? ".": "") +
split[i];
ancestor_name?

[not at google - send to devlin, 2013-04-04 01:32:22]

On 2013/04/04 01:07:20, cduvall wrote:
> Just realized that web pages will still have to load _manifest_features.json
and
> _permission_features.json, since all the feature types are tied to the same
> lazily initialized global. I'm also working on splitting these up, so only the
> needed files are loaded (e.g. for web pages, only _api_feature.json). Do you
> think I should finish that before we try register the bindings in web page
> contexts again?

Yep, that's a pretty simple change and it makes sense to submit first.

[cduvall, 2013-04-11 00:02:56]

Just waiting on https://codereview.chromium.org/13997002/ to go in.

https://codereview.chromium.org/13604005/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/13604005/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/_api_features.json:9: "contexts":
["blessed_extension", "unblessed_extension", "content_script", "web_page"],
On 2013/04/04 01:30:26, kalman wrote:
> 80 characters

Done.

https://codereview.chromium.org/13604005/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/_api_features.json:10: "matches": [ "<all_urls>" ]
On 2013/04/04 01:30:26, kalman wrote:
> no spaces around this (sorry...)

Done.

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:692: std::string current_name;
On 2013/04/04 01:30:26, kalman wrote:
> ok... I think I understand what you're doing here. If an API has a parent,
check
> its availability and all its parents' availability. If the parent is available
> and it isn't, don't install the bindings, but leave the object there.
> 
> Maybe you could explain something along those lines in the comment. It's
pretty
> subtle. And give the canonical example (apps) - well actually that's the only
> example right?

Done.

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:693: bool only_parent_available =
false;
On 2013/04/04 01:30:26, kalman wrote:
> only_ancestor_available?

Done.

https://codereview.chromium.org/13604005/diff/8001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:695: current_name += (i ? ".": "") +
split[i];
On 2013/04/04 01:30:26, kalman wrote:
> ancestor_name?

Done.

[commit-bot: I haz the power, 2013-04-11 23:20:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/18001

[commit-bot: I haz the power, 2013-04-12 00:01:29]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-04-12 00:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/33001

[commit-bot: I haz the power, 2013-04-12 02:11:31]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-12 02:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/53001

[commit-bot: I haz the power, 2013-04-12 03:22:34]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[not at google - send to devlin, 2013-04-12 03:52:44]

lgtm

https://codereview.chromium.org/13604005/diff/69006/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api_unittest.cc (left):

https://codereview.chromium.org/13604005/diff/69006/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:388:
"chrome-extension://fakeextension"));
change to TRUE?

[cduvall, 2013-04-12 03:54:49]

https://codereview.chromium.org/13604005/diff/69006/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api_unittest.cc (left):

https://codereview.chromium.org/13604005/diff/69006/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:388:
"chrome-extension://fakeextension"));
On 2013/04/12 03:52:44, kalman wrote:
> change to TRUE?

Done.

[commit-bot: I haz the power, 2013-04-12 03:55:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/70002

[commit-bot: I haz the power, 2013-04-12 05:21:28]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-12 05:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/70002

[commit-bot: I haz the power, 2013-04-12 06:57:24]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-18 00:08:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/89001

[commit-bot: I haz the power, 2013-04-18 03:06:04]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-18 03:33:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/89001

[commit-bot: I haz the power, 2013-04-18 07:33:14]

Change committed as 194837

[cduvall, 2013-04-19 00:46:50]

This gives web pages access to APIs they should have access to, but does not
register bindings for all the APIs if they should not be available to a web page
context.

[not at google - send to devlin, 2013-04-19 01:07:35]

lgtm

[commit-bot: I haz the power, 2013-04-19 01:08:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/76004

[commit-bot: I haz the power, 2013-04-19 12:56:52]

Change committed as 195143

[not at google - send to devlin, 2013-04-19 14:34:36]

Ok, much better, but still a tiny regression e.g.

http://build.chromium.org/f/chromium/perf/linux-release/intl1/report.html?his...

I still need to revert though :( but perhaps with those two changes (in
particular the json one) it'll be better.

https://chromiumcodereview.appspot.com/13604005/diff/76004/chrome/renderer/ex...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/13604005/diff/76004/chrome/renderer/ex...
chrome/renderer/extensions/dispatcher.cc:1012:
InstallBindings(module_system.get(), v8_context, "webstore");
We should be able to remove these two lines since app and webstore are injected
via RegisterSchemaGeneratedBindings now.

https://chromiumcodereview.appspot.com/13604005/diff/76004/chrome/renderer/ex...
chrome/renderer/extensions/dispatcher.cc:1015: module_system->Require("json"); 
// see paranoid comment in json.js
I'd be interested in seeing whether moving this Require("json") inside an "if
(extension)" helps anything. It wouldn't be a permanent solution, but it would
give data.

[not at google - send to devlin, 2013-04-19 18:43:42]

https://chromiumcodereview.appspot.com/13604005/diff/76004/chrome/common/exte...
File chrome/common/extensions/api/_api_features.json (right):

https://chromiumcodereview.appspot.com/13604005/diff/76004/chrome/common/exte...
chrome/common/extensions/api/_api_features.json:8: "extension_types":
["hosted_app", "extension"],
this also needs packaged_app

[cduvall, 2013-04-24 01:34:01]

https://codereview.chromium.org/13604005/diff/76004/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/13604005/diff/76004/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:8: "extension_types":
["hosted_app", "extension"],
On 2013/04/19 18:43:42, kalman wrote:
> this also needs packaged_app

Done.

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1012:
InstallBindings(module_system.get(), v8_context, "webstore");
On 2013/04/19 14:34:36, kalman wrote:
> We should be able to remove these two lines since app and webstore are
injected
> via RegisterSchemaGeneratedBindings now.

Done.

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1015: module_system->Require("json"); 
// see paranoid comment in json.js
On 2013/04/19 14:34:36, kalman wrote:
> I'd be interested in seeing whether moving this Require("json") inside an "if
> (extension)" helps anything. It wouldn't be a permanent solution, but it would
> give data.

Deleting this Require ended up not failing any tests. Maybe something changed
that made it not necessary any more?

[not at google - send to devlin, 2013-04-24 04:09:09]

lgtm after you add back the json require behind is_web_context :)

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1015: module_system->Require("json"); 
// see paranoid comment in json.js
On 2013/04/24 01:34:01, cduvall wrote:
> On 2013/04/19 14:34:36, kalman wrote:
> > I'd be interested in seeing whether moving this Require("json") inside an
"if
> > (extension)" helps anything. It wouldn't be a permanent solution, but it
would
> > give data.
> 
> Deleting this Require ended up not failing any tests. Maybe something changed
> that made it not necessary any more?

No tests? I would actually expect it to break one of the Messaging tests since
that sets up the JSON problem. Maybe some of this code ends up executing first.

But, putting this behind a !web_page_context should be safe. For now at least.
For launching the externally_connectable stuff, we'll need to do something like
the separate context thing.

[cduvall, 2013-04-25 00:12:10]

I had to change CrazyExtensionTest to use chrome.app instead of chrome.webstore,
because now that the webstore bindings aren't set up in every context it failed.

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/13604005/diff/76004/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1015: module_system->Require("json"); 
// see paranoid comment in json.js
On 2013/04/24 04:09:10, kalman wrote:
> On 2013/04/24 01:34:01, cduvall wrote:
> > On 2013/04/19 14:34:36, kalman wrote:
> > > I'd be interested in seeing whether moving this Require("json") inside an
> "if
> > > (extension)" helps anything. It wouldn't be a permanent solution, but it
> would
> > > give data.
> > 
> > Deleting this Require ended up not failing any tests. Maybe something
changed
> > that made it not necessary any more?
> 
> No tests? I would actually expect it to break one of the Messaging tests since
> that sets up the JSON problem. Maybe some of this code ends up executing
first.
> 
> But, putting this behind a !web_page_context should be safe. For now at least.
> For launching the externally_connectable stuff, we'll need to do something
like
> the separate context thing.

Done.

[commit-bot: I haz the power, 2013-04-25 00:12:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/133001

[commit-bot: I haz the power, 2013-04-25 01:15:23]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-04-25 02:19:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13604005/133001

[commit-bot: I haz the power, 2013-04-25 05:37:31]

Change committed as 196319