Move chrome.appWindow to chrome.app.window.

Move chrome.appWindow to chrome.app.window.

BUG=134573
TEST=In platform apps, you should be able to call all the same methods, etc.
via the new chrome.app.window namespace that you used to be able to with
chrome.appWindow.

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

[asargent_no_longer_on_chrome, 2012-06-26 00:35:14]

miket: please review everything

kalman: please review these files in particular:

_permission_features.json
extension_dispatcher.cc
extension_permission_set.cc

Everything seems to work ok (my try jobs are currently running and might show
problems I've missed), but let me know if you anticipate any problems that we'll
run into with using "chrome.app" here - this seems to be the first instance of a
multi-part namespace like this.

[not at google - send to devlin, 2012-06-26 01:18:08]

Yes, I have to think about what might happen giving different sections of APIs
different permissions. However, I don't have enough time to do that plus worry
about the apps deadlines and so on.

I think this change will make it so that extensions can call app.window. We'd
need to add yet another check that only platform apps can call the API; in
v8_context_set I suppose, and then make sure there's a corresponding
browser-side check.

Alternatively, remove the app.window from extension_permission_set.cc and add
code to manifest.cc to add it. Ideally this would use the feature system but I
don't think we can do that yet (grant permissions based on features; we can only
restrict them).

So... try that first. Adding to manifest.cc.

[Aaron Boodman, 2012-06-26 02:21:00]

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:16: "app.window": {
We've never had permissions named this way... ?

http://codereview.chromium.org/10659021/diff/2001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:12:
chromeHidden.registerCustomHook('app.window', function(bindingsAPI) {
Does this really work!? It seems like these names are probably used as a key in
a javascript object at some point, and I wouldn't expect this to do the right
thing.

[Mihai Parparita -not on Chrome, 2012-06-26 05:19:16]

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:16: "app.window": {
On 2012/06/26 02:21:00, Aaron Boodman wrote:
> We've never had permissions named this way... ?

You mean it's not the right style? Given r134463, we don't expect anyone to
actually include it in their manifest, so it'f for internal use only. It might
actually be nice to enforce that it can't be specified in a manifest.

http://codereview.chromium.org/10659021/diff/2001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/iframes/manifest.json (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/iframes/manifest.json:7: "app.window"
I assume this can be removed entirely (given r134463).

http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
tools/json_schema_compiler/schema_bundle_generator.py:86: # Transform a fully
qualified function name like foo.bar.baz into
Move into a function so that it can be unit tested?

[Mihai Parparita -not on Chrome, 2012-06-26 05:21:24]

BTW, the tryserver failures are because the tests added by
http://crrev.com/144017 need to be updated too.

Mihai

On Mon, Jun 25, 2012 at 10:19 PM, <mihaip@chromium.org> wrote:

>
> http://codereview.chromium.**org/10659021/diff/2001/chrome/**
>
common/extensions/api/_**permission_features.json<http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/api/_permission_features.json>
> File chrome/common/extensions/api/_**permission_features.json (right):
>
> http://codereview.chromium.**org/10659021/diff/2001/chrome/**
>
common/extensions/api/_**permission_features.json#**newcode16<http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/api/_permission_features.json#newcode16>
> chrome/common/extensions/api/_**permission_features.json:16: "app.window":
> {
> On 2012/06/26 02:21:00, Aaron Boodman wrote:
>
>> We've never had permissions named this way... ?
>>
>
> You mean it's not the right style? Given r134463, we don't expect anyone
> to actually include it in their manifest, so it'f for internal use only.
> It might actually be nice to enforce that it can't be specified in a
> manifest.
>
> http://codereview.chromium.**org/10659021/diff/2001/chrome/**
>
test/data/extensions/platform_**apps/iframes/manifest.json<http://codereview.chromium.org/10659021/diff/2001/chrome/test/data/extensions/platform_apps/iframes/manifest.json>
> File chrome/test/data/extensions/**platform_apps/iframes/**manifest.json
> (right):
>
> http://codereview.chromium.**org/10659021/diff/2001/chrome/**
>
test/data/extensions/platform_**apps/iframes/manifest.json#**newcode7<http://codereview.chromium.org/10659021/diff/2001/chrome/test/data/extensions/platform_apps/iframes/manifest.json#newcode7>
> chrome/test/data/extensions/**platform_apps/iframes/**manifest.json:7:
> "app.window"
> I assume this can be removed entirely (given r134463).
>
> http://codereview.chromium.**org/10659021/diff/2001/tools/**
>
json_schema_compiler/schema_**bundle_generator.py<http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/schema_bundle_generator.py>
> File tools/json_schema_compiler/**schema_bundle_generator.py (right):
>
> http://codereview.chromium.**org/10659021/diff/2001/tools/**
>
json_schema_compiler/schema_**bundle_generator.py#newcode86<http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/schema_bundle_generator.py#newcode86>
> tools/json_schema_compiler/**schema_bundle_generator.py:86: # Transform a
> fully qualified function name like foo.bar.baz into
> Move into a function so that it can be unit tested?
>
>
http://codereview.chromium.**org/10659021/<http://codereview.chromium.org/106...
>

[Aaron Boodman, 2012-06-26 05:41:29]

asargent, can you check that this doesn't result in extensions getting access to
this API before landing?

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/ap...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/ap...
chrome/common/extensions/api/_permission_features.json:16: "app.window": {
On 2012/06/26 05:19:16, Mihai Parparita wrote:
> On 2012/06/26 02:21:00, Aaron Boodman wrote:
> > We've never had permissions named this way... ?
> 
> You mean it's not the right style?

No I just meant it stuck out that we haven't done this before. Sorry, it was
more a note to myself that I should have removed pre-publish.

> Given r134463

I think maybe you meant a different rev? crrev.com/134463 seems unrelated.

http://codereview.chromium.org/10659021/diff/2001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:12:
chromeHidden.registerCustomHook('app.window', function(bindingsAPI) {
On 2012/06/26 02:21:00, Aaron Boodman wrote:
> Does this really work!? It seems like these names are probably used as a key
in
> a javascript object at some point, and I wouldn't expect this to do the right
> thing.

I take this back, it seems ok.

[Mihai Parparita -not on Chrome, 2012-06-26 07:03:51]

On Mon, Jun 25, 2012 at 10:41 PM, <aa@chromium.org> wrote:

>  Given r134463
>>
>
> I think maybe you meant a different rev? crrev.com/134463 seems
> unrelated.


Meant http://crrev.com/144056

Mihai

[not at google - send to devlin, 2012-06-26 07:05:12]

On 2012/06/26 07:03:51, Mihai Parparita wrote:
> On Mon, Jun 25, 2012 at 10:41 PM, <mailto:aa@chromium.org> wrote:
> 
> >  Given r134463
> >>
> >
> > I think maybe you meant a different rev? crrev.com/134463 seems
> > unrelated.
> 
> 
> Meant http://crrev.com/144056
> 
> Mihai

And that change didn't make it available to extensions..?

[not at google - send to devlin, 2012-06-26 07:09:35]

On 2012/06/26 07:05:12, kalman wrote:
> On 2012/06/26 07:03:51, Mihai Parparita wrote:
> > On Mon, Jun 25, 2012 at 10:41 PM, <mailto:aa@chromium.org> wrote:
> > 
> > >  Given r134463
> > >>
> > >
> > > I think maybe you meant a different rev? crrev.com/134463 seems
> > > unrelated.
> > 
> > 
> > Meant http://crrev.com/144056
> > 
> > Mihai
> 
> And that change didn't make it available to extensions..?

Correction. I just checked, and now extensions can call appWindow.create.

[jeremya, 2012-06-26 18:01:42]

Oh, I thought the "platform_app" restriction in _permission_features.json
would prevent that :(

Made a bug for myself: http://crbug.com/134683

[miket_OOO, 2012-06-26 18:07:05]

lgtm

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/do...
File chrome/common/extensions/docs/extensions/samples.html (right):

http://codereview.chromium.org/10659021/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/extensions/samples.html:207:
"3fb3045783163c0eaaf0d0ce139f254502192a92": "CALCULATOR (PACKAGED APP) ADD
NUMBERS. NOW INCLUDES SUBTRACTION! EXPERIMENTAL CHROME.APP.WINDOW.CREATE",
I've moved this app over to chrome-app-samples, so I'll remove it from here
unless you want to do it in this CL.

[not at google - send to devlin, 2012-06-26 21:48:04]

> Oh, I thought the "platform_app" restriction in _permission_features.json
> would prevent that :(
> 
> Made a bug for myself: http://crbug.com/134683

Features are higher-level than permissions, they just populate an
ExtensionPermissionSet. Adding something to the whitelist in
extension_permission_set.cc means that it's effectively always in the set.

[Mihai Parparita -not on Chrome, 2012-07-14 00:42:32]

Given that Jeremy fixed appWindow getting exposed to extensions, are there
any other fundamental objections to this CL?

(only thing I see left my comments about tests at
http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
)

Mihai

P.S. Sent out http://crrev.com/10785009 to remove the calculator sample.

On Tue, Jun 26, 2012 at 2:48 PM, <kalman@chromium.org> wrote:

> Oh, I thought the "platform_app" restriction in _permission_features.json
>> would prevent that :(
>>
>
>  Made a bug for myself: http://crbug.com/134683
>>
>
> Features are higher-level than permissions, they just populate an
> ExtensionPermissionSet. Adding something to the whitelist in
> extension_permission_set.cc means that it's effectively always in the set.
>
>
http://codereview.chromium.**org/10659021/<http://codereview.chromium.org/106...
>

[not at google - send to devlin, 2012-07-16 01:54:42]

On 2012/07/14 00:42:32, Mihai Parparita wrote:
> Given that Jeremy fixed appWindow getting exposed to extensions, are there
> any other fundamental objections to this CL?
> 
> (only thing I see left my comments about tests at
>
http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
> )
> 
> Mihai
> 
> P.S. Sent out http://crrev.com/10785009 to remove the calculator sample.
> 
> On Tue, Jun 26, 2012 at 2:48 PM, <mailto:kalman@chromium.org> wrote:
> 
> > Oh, I thought the "platform_app" restriction in _permission_features.json
> >> would prevent that :(
> >>
> >
> >  Made a bug for myself: http://crbug.com/134683
> >>
> >
> > Features are higher-level than permissions, they just populate an
> > ExtensionPermissionSet. Adding something to the whitelist in
> > extension_permission_set.cc means that it's effectively always in the set.
> >
> >
>
http://codereview.chromium.**org/10659021/%3Chttp://codereview.chromium.org/1...>
> >

It needs to be rebased.

[not at google - send to devlin, 2012-07-16 01:59:25]

On 2012/07/16 01:54:42, kalman wrote:
> On 2012/07/14 00:42:32, Mihai Parparita wrote:
> > Given that Jeremy fixed appWindow getting exposed to extensions, are there
> > any other fundamental objections to this CL?
> > 
> > (only thing I see left my comments about tests at
> >
>
http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
> > )
> > 
> > Mihai
> > 
> > P.S. Sent out http://crrev.com/10785009 to remove the calculator sample.
> > 
> > On Tue, Jun 26, 2012 at 2:48 PM, <mailto:kalman@chromium.org> wrote:
> > 
> > > Oh, I thought the "platform_app" restriction in _permission_features.json
> > >> would prevent that :(
> > >>
> > >
> > >  Made a bug for myself: http://crbug.com/134683
> > >>
> > >
> > > Features are higher-level than permissions, they just populate an
> > > ExtensionPermissionSet. Adding something to the whitelist in
> > > extension_permission_set.cc means that it's effectively always in the set.
> > >
> > >
> >
>
http://codereview.chromium.**org/10659021/%253Chttp://codereview.chromium.org...>
> > >
> 
> It needs to be rebased.

(I have no objects though)

[not at google - send to devlin, 2012-07-16 01:59:33]

On 2012/07/16 01:59:25, kalman wrote:
> On 2012/07/16 01:54:42, kalman wrote:
> > On 2012/07/14 00:42:32, Mihai Parparita wrote:
> > > Given that Jeremy fixed appWindow getting exposed to extensions, are there
> > > any other fundamental objections to this CL?
> > > 
> > > (only thing I see left my comments about tests at
> > >
> >
>
http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
> > > )
> > > 
> > > Mihai
> > > 
> > > P.S. Sent out http://crrev.com/10785009 to remove the calculator sample.
> > > 
> > > On Tue, Jun 26, 2012 at 2:48 PM, <mailto:kalman@chromium.org> wrote:
> > > 
> > > > Oh, I thought the "platform_app" restriction in
_permission_features.json
> > > >> would prevent that :(
> > > >>
> > > >
> > > >  Made a bug for myself: http://crbug.com/134683
> > > >>
> > > >
> > > > Features are higher-level than permissions, they just populate an
> > > > ExtensionPermissionSet. Adding something to the whitelist in
> > > > extension_permission_set.cc means that it's effectively always in the
set.
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/10659021/%25253Chttp://codereview.chromium.o...>
> > > >
> > 
> > It needs to be rebased.
> 
> (I have no objects though)

(nor objections)

[not at google - send to devlin, 2012-07-18 04:27:37]

I didn't get another email, but it seems to have been updated to here we go
anyway.

Shouldn't there be an update to at least
chrome/common/extensions/permissions/api_permission.cc as well?

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/browser_tag_isolation/test.js
(right):

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/browser_tag_isolation/test.js:6:
chrome.app.window.create('main.html', {}, function () {});
Unrelated obseration:

It looks like in the IDL the second 2 arguments are optional. Just wondering if
you guys want to leave them out in all the examples, since it seems like there's
a lot of "function(){}" going on.

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/iframes/manifest.json (right):

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/iframes/manifest.json:7: "app.window"
delete?

http://codereview.chromium.org/10659021/diff/18001/tools/json_schema_compiler...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

http://codereview.chromium.org/10659021/diff/18001/tools/json_schema_compiler...
tools/json_schema_compiler/schema_bundle_generator.py:16: def
CapitalizeFirstLetter(value):
nit: start with _

[asargent_no_longer_on_chrome, 2012-07-18 23:41:08]

No email because I wasn't ready for re-review yet (still updating tests and
getting documentation generation working). =)

[not at google - send to devlin, 2012-07-18 23:49:28]

On 2012/07/18 23:41:08, Antony Sargent wrote:
> No email because I wasn't ready for re-review yet (still updating tests and
> getting documentation generation working). =)

Ah, sorry :)

Btw the documentation generator was broken for a while, the --test-shell
argument was removed from DRT. I believe it's fixed now, though.

[asargent_no_longer_on_chrome, 2012-07-20 00:11:46]

Ok, now ready for review again. I suggest looking at diffs between patch sets 4
and 5.

Added more docs and test file changes so the total number of files in the CL is
larger - sorry. 

> Shouldn't there be an update to at least
> chrome/common/extensions/permissions/api_permission.cc
> as well?

Yes, there is that now.

http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
tools/json_schema_compiler/schema_bundle_generator.py:86: # Transform a fully
qualified function name like foo.bar.baz into
On 2012/06/26 05:19:16, Mihai Parparita wrote:
> Move into a function so that it can be unit tested?

Done.

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/iframes/manifest.json (right):

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/iframes/manifest.json:7: "app.window"
On 2012/07/18 04:27:37, kalman wrote:
> delete?

I think we actually still want it as an explicit permission - Mihai, was this
your intention?

http://codereview.chromium.org/10659021/diff/18001/tools/json_schema_compiler...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

http://codereview.chromium.org/10659021/diff/18001/tools/json_schema_compiler...
tools/json_schema_compiler/schema_bundle_generator.py:16: def
CapitalizeFirstLetter(value):
On 2012/07/18 04:27:37, kalman wrote:
> nit: start with _

this function moved to schema_util.py

[Mihai Parparita -not on Chrome, 2012-07-20 00:21:42]

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/iframes/manifest.json (right):

http://codereview.chromium.org/10659021/diff/18001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/iframes/manifest.json:7: "app.window"
On 2012/07/20 00:11:47, Antony Sargent wrote:
> On 2012/07/18 04:27:37, kalman wrote:
> > delete?
> 
> I think we actually still want it as an explicit permission - Mihai, was this
> your intention?

Jeremy made it so that apps don't need to declare it in the manifest
(http://crrev.com/144056, redone as http://crrev.com/144370). There's no harm in
including it, but it's not necessary.

[Mihai Parparita -not on Chrome, 2012-07-20 00:36:51]

http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

http://codereview.chromium.org/10659021/diff/2001/tools/json_schema_compiler/...
tools/json_schema_compiler/schema_bundle_generator.py:86: # Transform a fully
qualified function name like foo.bar.baz into
On 2012/07/20 00:11:46, Antony Sargent wrote:
> On 2012/06/26 05:19:16, Mihai Parparita wrote:
> > Move into a function so that it can be unit tested?
> 
> Done.

Not seeing this change in the current patch.

[asargent_no_longer_on_chrome, 2012-07-20 05:02:09]

> Not seeing this change in the current patch.

Oops, forgot to merge this into the branch I was using for codereview upload.
Should be there now.

[Mihai Parparita -not on Chrome, 2012-07-20 05:06:12]

LGTM