Pass command line arguments onto platform apps which provide the right intent.

Pass command line arguments onto platform apps which provide the right intent.

Any command line arguments which are file names are passed through in
launchData.intent

BUG=None
TEST=New tests added


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

[benwells, 2012-05-09 12:27:22]

+gbillock especially for intent data format in launchData- see:
    chrome/browser/extensions/api/app/app_api.h

[benwells, 2012-05-09 12:29:07]

More tests to come, before commit

[benwells, 2012-05-11 01:52:53]

On 2012/05/09 12:29:07, benwells wrote:
> More tests to come, before commit

After adding more tests, its clear this patch has security problems. It gives
more access than it should to the platform apps.

After I've worked out how to properly relax security (probably via an isolated
file system) I'll update the patch.

[benwells, 2012-05-14 04:26:49]

Ready for review now: tests added, security changes removed, compiling on
non-linux platforms fixed.

More changes to come to give the app access to the file.

[James Hawkins, 2012-05-14 05:11:05]

Wha?  Where is the design doc for this change?  I don't remember ever seeing
this in a discussion.

[benwells, 2012-05-14 05:16:46]

On 2012/05/14 05:11:05, James Hawkins wrote:
> Wha?  Where is the design doc for this change?  I don't remember ever seeing
> this in a discussion.

This is a continuation of a previous patch
http://codereview.chromium.org/9845003/ but the code changed so much I started a
new CL. You reviewed that patch some time ago. I'll send you a link to the
design doc (it is out of date now).

[Greg Billock, 2012-05-14 16:16:27]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
Should also have a "data" field.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:53: static void
DispatchOnLaunchedEventWithUrl(Profile* profile,
How about "DispatchOnLaunchedEventWithIntent" and pass in the intent payload?

[benwells, 2012-05-14 23:44:32]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
On 2012/05/14 16:16:27, Greg Billock wrote:
> Should also have a "data" field.

When sending onLaunched there is no data field. At one point in time this was
valid for WebIntents, is it still valid?

I have seen that after a download the data field is populated with a blob. I
don't think that is worth the effort for this case as, once the next patch in
this series lands, a FileEntry can be easily retrieved, which will do reading
and writing (writing in the future).

[benwells, 2012-05-14 23:50:31]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:209: argv.pop_back();
The removal of existing command line args should be extracted into a common
function.

[Greg Billock, 2012-05-14 23:55:38]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
It's a valid intent, but I was thinking the API would be broader and more
potentially useful if it could carry the "data" payload as well -- that'd let
clients send text content, for instance.

On 2012/05/14 23:44:32, benwells wrote:
> On 2012/05/14 16:16:27, Greg Billock wrote:
> > Should also have a "data" field.
> 
> When sending onLaunched there is no data field. At one point in time this was
> valid for WebIntents, is it still valid?
> 
> I have seen that after a download the data field is populated with a blob. I
> don't think that is worth the effort for this case as, once the next patch in
> this series lands, a FileEntry can be easily retrieved, which will do reading
> and writing (writing in the future).

[benwells, 2012-05-15 00:11:45]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
On 2012/05/14 23:55:38, Greg Billock wrote:
> It's a valid intent, but I was thinking the API would be broader and more
> potentially useful if it could carry the "data" payload as well -- that'd let
> clients send text content, for instance.
> 
> On 2012/05/14 23:44:32, benwells wrote:
> > On 2012/05/14 16:16:27, Greg Billock wrote:
> > > Should also have a "data" field.
> > 
> > When sending onLaunched there is no data field. At one point in time this
was
> > valid for WebIntents, is it still valid?
> > 
> > I have seen that after a download the data field is populated with a blob. I
> > don't think that is worth the effort for this case as, once the next patch
in
> > this series lands, a FileEntry can be easily retrieved, which will do
reading
> > and writing (writing in the future).
> 

OK ... this could be added in future but for now I'd like to leave it as is.
This is only accessed when starting platform apps with file names on the command
line.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:53: static void
DispatchOnLaunchedEventWithUrl(Profile* profile,
On 2012/05/14 16:16:27, Greg Billock wrote:
> How about "DispatchOnLaunchedEventWithIntent" and pass in the intent payload?

Yeah that would be nice. I started going down that track, but pulled back as
data is null. To do it we'd need to add a new DataType value NULL, is that
right?

[Mihai Parparita -not on Chrome, 2012-05-15 00:39:27]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
kOnLaunchedEvent, json_args, NULL, GURL());
Seems like you would also need to update experimental.app.json to include the
launch data parameter (and define the type, for validation).

Additionally, if you include the type in the JSON file, the JSON schema compiler
will generate a struct for you that has a ToValue() method, so that you don't
need to hardcode the key names.

For an example, see the alarms API: 

The Alarm type is defined here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...

The  generated code looks like:
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...

And is used here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
On 2012/05/15 00:11:46, benwells wrote:
> OK ... this could be added in future but for now I'd like to leave it as is.
> This is only accessed when starting platform apps with file names on the
command
> line.

In that case, can "url" be named more specifically (launch_file_url or the
like)? I found the name too generic/confusing right now.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:204: // The command line
already has arguments on it ("about:blank"). We want to
What is adding the about:blank?

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_launcher.cc:72: const string16
kViewAction_UTF16 = ASCIIToUTF16("http://webintents.org/view");
There's no constant for this intent?

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_launcher.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_launcher.h:18: void
LaunchPlatformApp(const CommandLine* command_line,
Nit: seems like command_line should be the last parameter, since it's optional.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:294: static content::WebContents* OpenApplication(
This can be in a follow-up cleanup CL, but OpenApplication now seems to have too
many parameters, only some combinations of which are valid (e.g. platform apps
look at command_line but ignore container, override_url and disposition).

Time for a LaunchData struct that can be created for platform app vs.
hosted/packaged app launches?

http://codereview.chromium.org/10332071/diff/8004/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1777: } else {
Might as well make this into an explicit "else if (is_packaged_app())", since
this branch isn't applicable to platform apps (href is empty, so calling
GetResourceURL on it doesn't really make sense).

[Greg Billock, 2012-05-15 00:54:58]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
kOnLaunchedEvent, json_args, NULL, GURL());
If we're using an idl value, we could put "action", "type", "data", and "extras"
in it, as for the Alarm type, right? We couldn't pass any serializable JS
objects, but it'd give quite a bit of flexibility for passing text. Or is this
facility something different?

On 2012/05/15 00:39:28, Mihai Parparita wrote:
> Seems like you would also need to update experimental.app.json to include the
> launch data parameter (and define the type, for validation).
> 
> Additionally, if you include the type in the JSON file, the JSON schema
compiler
> will generate a struct for you that has a ToValue() method, so that you don't
> need to hardcode the key names.
> 
> For an example, see the alarms API: 
> 
> The Alarm type is defined here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> 
> The  generated code looks like:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...
> 
> And is used here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:53: static void
DispatchOnLaunchedEventWithUrl(Profile* profile,
On 2012/05/15 00:11:46, benwells wrote:
> On 2012/05/14 16:16:27, Greg Billock wrote:
> > How about "DispatchOnLaunchedEventWithIntent" and pass in the intent
payload?
> 
> Yeah that would be nice. I started going down that track, but pulled back as
> data is null. To do it we'd need to add a new DataType value NULL, is that
> right?

Couldn't it just be left blank if there's no "data" parameter? I guess I'm not
totally clear on the scope (perhaps I didn't read enough of the CL.) If this
ties into an existing facility to launch the app with a url, it looks fine, but
I'm not sure why you'd have the action/type then, for instance. Wouldn't that be
handled upstream in the matching logic, and not need to be passed in here?

[benwells, 2012-05-16 01:35:29]

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
kOnLaunchedEvent, json_args, NULL, GURL());
On 2012/05/15 00:54:59, Greg Billock wrote:
> If we're using an idl value, we could put "action", "type", "data", and
"extras"
> in it, as for the Alarm type, right? We couldn't pass any serializable JS
> objects, but it'd give quite a bit of flexibility for passing text. Or is this
> facility something different?
> 
> On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > Seems like you would also need to update experimental.app.json to include
the
> > launch data parameter (and define the type, for validation).
> > 
> > Additionally, if you include the type in the JSON file, the JSON schema
> compiler
> > will generate a struct for you that has a ToValue() method, so that you
don't
> > need to hardcode the key names.
> > 
> > For an example, see the alarms API: 
> > 
> > The Alarm type is defined here:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> > 
> > The  generated code looks like:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...
> > 
> > And is used here:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> 

My understanding is that the IDL or JSON provides some nice autogenerated code
for you, which makes creating these kind of parameters easier. It doesn't change
what can go in them.

I am working on adding a FileEntry as a data parameter to the intent, which will
happen in js with some extension hookery.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_launcher.cc:72: const string16
kViewAction_UTF16 = ASCIIToUTF16("http://webintents.org/view");
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> There's no constant for this intent?

No. Its used somewhere else (in the downloads code somewhere) already. Maybe we
could move it somewhere central - Greg, any thoughts on where it would go?

[Mihai Parparita -not on Chrome, 2012-05-17 00:40:14]

On Tue, May 15, 2012 at 6:35 PM, <benwells@chromium.org> wrote:

> My understanding is that the IDL or JSON provides some nice
> autogenerated code for you, which makes creating these kind of
> parameters easier. It doesn't change what can go in them.
>

That's correct. I was just pointing out that you don't need to have the
error-prone DictionaryValue construction, instead there's a struct you can
populate.

Mihai

[Greg Billock, 2012-05-17 01:41:27]

On 2012/05/16 01:35:29, benwells wrote:
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/app/app_api.cc (right):
> 
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
> chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
> kOnLaunchedEvent, json_args, NULL, GURL());
> On 2012/05/15 00:54:59, Greg Billock wrote:
> > If we're using an idl value, we could put "action", "type", "data", and
> "extras"
> > in it, as for the Alarm type, right? We couldn't pass any serializable JS
> > objects, but it'd give quite a bit of flexibility for passing text. Or is
this
> > facility something different?
> > 
> > On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > > Seems like you would also need to update experimental.app.json to include
> the
> > > launch data parameter (and define the type, for validation).
> > > 
> > > Additionally, if you include the type in the JSON file, the JSON schema
> > compiler
> > > will generate a struct for you that has a ToValue() method, so that you
> don't
> > > need to hardcode the key names.
> > > 
> > > For an example, see the alarms API: 
> > > 
> > > The Alarm type is defined here:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> > > 
> > > The  generated code looks like:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...
> > > 
> > > And is used here:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> > 
> 
> My understanding is that the IDL or JSON provides some nice autogenerated code
> for you, which makes creating these kind of parameters easier. It doesn't
change
> what can go in them.
> 
> I am working on adding a FileEntry as a data parameter to the intent, which
will
> happen in js with some extension hookery.
> 
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
> File chrome/browser/extensions/platform_app_launcher.cc (right):
> 
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
> chrome/browser/extensions/platform_app_launcher.cc:72: const string16
> kViewAction_UTF16 = ASCIIToUTF16("http://webintents.org/view");
> On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > There's no constant for this intent?
> 
> No. Its used somewhere else (in the downloads code somewhere) already. Maybe
we
> could move it somewhere central - Greg, any thoughts on where it would go?

Having it centralized would work OK. It's in a couple places in the code now.
We're going to have intent-specific strings for well-known intents, so there
will be a list of well-known ones we'll reference.

[benwells, 2012-05-17 03:59:10]

On 2012/05/17 01:41:27, Greg Billock wrote:
> On 2012/05/16 01:35:29, benwells wrote:
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
> > File chrome/browser/extensions/api/app/app_api.cc (right):
> > 
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
> > chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
> > kOnLaunchedEvent, json_args, NULL, GURL());
> > On 2012/05/15 00:54:59, Greg Billock wrote:
> > > If we're using an idl value, we could put "action", "type", "data", and
> > "extras"
> > > in it, as for the Alarm type, right? We couldn't pass any serializable JS
> > > objects, but it'd give quite a bit of flexibility for passing text. Or is
> this
> > > facility something different?
> > > 
> > > On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > > > Seems like you would also need to update experimental.app.json to
include
> > the
> > > > launch data parameter (and define the type, for validation).
> > > > 
> > > > Additionally, if you include the type in the JSON file, the JSON schema
> > > compiler
> > > > will generate a struct for you that has a ToValue() method, so that you
> > don't
> > > > need to hardcode the key names.
> > > > 
> > > > For an example, see the alarms API: 
> > > > 
> > > > The Alarm type is defined here:
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> > > > 
> > > > The  generated code looks like:
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...
> > > > 
> > > > And is used here:
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> > > 
> > 
> > My understanding is that the IDL or JSON provides some nice autogenerated
code
> > for you, which makes creating these kind of parameters easier. It doesn't
> change
> > what can go in them.
> > 
> > I am working on adding a FileEntry as a data parameter to the intent, which
> will
> > happen in js with some extension hookery.
> > 
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
> > File chrome/browser/extensions/platform_app_launcher.cc (right):
> > 
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
> > chrome/browser/extensions/platform_app_launcher.cc:72: const string16
> > kViewAction_UTF16 = ASCIIToUTF16("http://webintents.org/view");
> > On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > > There's no constant for this intent?
> > 
> > No. Its used somewhere else (in the downloads code somewhere) already. Maybe
> we
> > could move it somewhere central - Greg, any thoughts on where it would go?
> 
> Having it centralized would work OK. It's in a couple places in the code now.
> We're going to have intent-specific strings for well-known intents, so there
> will be a list of well-known ones we'll reference.

function onLaunched(launchData) {
    if (launchData.intent) {
        window = chrome.app.createWindow();
        window.intent = launchData.intent;
    } else {
    // something sensible
    }
}

Any preference on where it goes?
content/public/browser/web_intents_dispatcher.h? Maybe a new file
content/public/common/web_intents.h?

[benwells, 2012-05-17 04:11:22]

On 2012/05/17 03:59:10, benwells wrote:
> On 2012/05/17 01:41:27, Greg Billock wrote:
> > On 2012/05/16 01:35:29, benwells wrote:
> > >
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
> > > File chrome/browser/extensions/api/app/app_api.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
> > > chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
> > > kOnLaunchedEvent, json_args, NULL, GURL());
> > > On 2012/05/15 00:54:59, Greg Billock wrote:
> > > > If we're using an idl value, we could put "action", "type", "data", and
> > > "extras"
> > > > in it, as for the Alarm type, right? We couldn't pass any serializable
JS
> > > > objects, but it'd give quite a bit of flexibility for passing text. Or
is
> > this
> > > > facility something different?
> > > > 
> > > > On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > > > > Seems like you would also need to update experimental.app.json to
> include
> > > the
> > > > > launch data parameter (and define the type, for validation).
> > > > > 
> > > > > Additionally, if you include the type in the JSON file, the JSON
schema
> > > > compiler
> > > > > will generate a struct for you that has a ToValue() method, so that
you
> > > don't
> > > > > need to hardcode the key names.
> > > > > 
> > > > > For an example, see the alarms API: 
> > > > > 
> > > > > The Alarm type is defined here:
> > > > >
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> > > > > 
> > > > > The  generated code looks like:
> > > > >
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...
> > > > > 
> > > > > And is used here:
> > > > >
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> > > > 
> > > 
> > > My understanding is that the IDL or JSON provides some nice autogenerated
> code
> > > for you, which makes creating these kind of parameters easier. It doesn't
> > change
> > > what can go in them.
> > > 
> > > I am working on adding a FileEntry as a data parameter to the intent,
which
> > will
> > > happen in js with some extension hookery.
> > > 
> > >
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
> > > File chrome/browser/extensions/platform_app_launcher.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
> > > chrome/browser/extensions/platform_app_launcher.cc:72: const string16
> > > kViewAction_UTF16 = ASCIIToUTF16("http://webintents.org/view");
> > > On 2012/05/15 00:39:28, Mihai Parparita wrote:
> > > > There's no constant for this intent?
> > > 
> > > No. Its used somewhere else (in the downloads code somewhere) already.
Maybe
> > we
> > > could move it somewhere central - Greg, any thoughts on where it would go?
> > 
> > Having it centralized would work OK. It's in a couple places in the code
now.
> > We're going to have intent-specific strings for well-known intents, so there
> > will be a list of well-known ones we'll reference.
> 
> function onLaunched(launchData) {
>     if (launchData.intent) {
>         window = chrome.app.createWindow();
>         window.intent = launchData.intent;
>     } else {
>     // something sensible
>     }
> }

Ignore the above, it's funny what happens if you accidentally hit paste....

> 
> Any preference on where it goes?
> content/public/browser/web_intents_dispatcher.h? Maybe a new file
> content/public/common/web_intents.h?

[benwells, 2012-05-18 03:35:58]

Ready for review again. Sorry its getting to be a monster, I could split it up
if it helps the review.

reviewers:
- mihaip for ... all of it
- gbillock for WebIntents format (especially the comment in app_api.h)
- kinuko for use of isolated file system (especially platform_app_launcher.cc,
experimental.app_custom_bindings.[cc|js}])

Once you guys are happy i'll bring in the OWNERS.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.cc:142: extension->id(),
kOnLaunchedEvent, json_args, NULL, GURL());
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> Seems like you would also need to update experimental.app.json to include the
> launch data parameter (and define the type, for validation).
> 
> Additionally, if you include the type in the JSON file, the JSON schema
compiler
> will generate a struct for you that has a ToValue() method, so that you don't
> need to hardcode the key names.
> 
> For an example, see the alarms API: 
> 
> The Alarm type is defined here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> 
> The  generated code looks like:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/out/Release/obj/gen/chrome...
> 
> And is used here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

I've added the launchData field into the json, but I'm not using the handy
structure - the data format here is different to what gets delivered to the app,
being transformed in a custom event dispatcher.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> On 2012/05/15 00:11:46, benwells wrote:
> > OK ... this could be added in future but for now I'd like to leave it as is.
> > This is only accessed when starting platform apps with file names on the
> command
> > line.
> 
> In that case, can "url" be named more specifically (launch_file_url or the
> like)? I found the name too generic/confusing right now.
> 

url is now gone, with a FileEntry instead in data.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:204: // The command line
already has arguments on it ("about:blank"). We want to
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> What is adding the about:blank?

Its added here
http://code.google.com/p/chromium/source/search?q=InProcessBrowserTest%3A%3AP...

I added a comment mentioning this.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:209: argv.pop_back();
On 2012/05/14 23:50:33, benwells wrote:
> The removal of existing command line args should be extracted into a common
> function.

Done.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_launcher.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_launcher.h:18: void
LaunchPlatformApp(const CommandLine* command_line,
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> Nit: seems like command_line should be the last parameter, since it's
optional.

Done.

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:294: static content::WebContents* OpenApplication(
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> This can be in a follow-up cleanup CL, but OpenApplication now seems to have
too
> many parameters, only some combinations of which are valid (e.g. platform apps
> look at command_line but ignore container, override_url and disposition).
> 
> Time for a LaunchData struct that can be created for platform app vs.
> hosted/packaged app launches?

Agreed. Have added TODO.

http://codereview.chromium.org/10332071/diff/8004/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10332071/diff/8004/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1777: } else {
On 2012/05/15 00:39:28, Mihai Parparita wrote:
> Might as well make this into an explicit "else if (is_packaged_app())", since
> this branch isn't applicable to platform apps (href is empty, so calling
> GetResourceURL on it doesn't really make sense).

Done.

[Mihai Parparita -not on Chrome, 2012-05-18 04:26:03]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:106: for (size_t i=0;
i<args.size(); i++)
Nit: add space before/after = and <

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:34: const char kViewIntent[]
= "http://webintents.org/view";
Rather than having this as a a constant here and
DispatchOnLaunchedEventWithFileEntry taking a string action, it seems less
error-prone to have the function take an enum, and map it to the intent string
inside the function.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:103: found_service = true;
Add a break here.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:126: // The host isn't loaded
yet, so we post a task to the queue for the host.
Can this be racy (the host was about to be shut down and/or is in the process of
starting up)? Is it safer to always use AddPendingTask?

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:148: std::set<FilePath>
filesets;
Nit: this can be moved a few lines lower, since it's not needed to grant access.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
File chrome/renderer/extensions/experimental.app_custom_bindings.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
chrome/renderer/extensions/experimental.app_custom_bindings.cc:27: GURL
context_url = UserScriptSlave::GetDataSourceURLForFrame(webframe);
Might be safe to add a CHECK that this is an extension URL.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
chrome/renderer/extensions/experimental.app_custom_bindings.cc:29:
name.append("isolated/");
Seems like you can use GetFileSystemRootURI(context_url,
kFileSystemTypeIsolated) would help you construct these URLs

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/resources/...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:13:
function(launchData, file_system_id, base_name) {
Nit: JS naming style is to use camelCase for variable names.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/resources/...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:21:
chrome.Event.prototype.dispatch.apply(event, [launchData]);
Nit: if you use .call(), you can pass the arguments directly (so that you don't
need the empty array when you don't have any additional arguments).

http://codereview.chromium.org/10332071/diff/10073/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/launch_file/test.js (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/launch_file/test.js:12:
chrome.test.assertTrue(
You can use chrome.test.assertEq, so that you don't need to provide your own
failure message.

[benwells, 2012-05-18 04:56:54]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:34: const char kViewIntent[]
= "http://webintents.org/view";
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Rather than having this as a a constant here and
> DispatchOnLaunchedEventWithFileEntry taking a string action, it seems less
> error-prone to have the function take an enum, and map it to the intent string
> inside the function.

How is it error-prone? Do you mean if the code uses different strings, or
doesn't use the constant?

I talked to Greg about putting the constant somewhere and he wants to wait a
while. Maybe in the meantime we could put it in app_api.h, or pass through the
WebIntentServiceData to the Launch function (it has the action in it).

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:126: // The host isn't loaded
yet, so we post a task to the queue for the host.
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Can this be racy (the host was about to be shut down and/or is in the process
of
> starting up)? Is it safer to always use AddPendingTask?

Good point. I initially used AddPendingTask but it doesn't send the task if the
host is already loaded. This pattern of checking for the host and adding a
pending task is used elsewhere (e.g.
ExtensionSettingsHandler::HandleInspectMessage).

I haven't investigated to see if this could be racy. Matt - is this usage
correct? Is there a reason why AddPendingTask can't dispatch immediately if the
host is available to simplify usage?

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
File chrome/renderer/extensions/experimental.app_custom_bindings.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
chrome/renderer/extensions/experimental.app_custom_bindings.cc:29:
name.append("isolated/");
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Seems like you can use GetFileSystemRootURI(context_url,
> kFileSystemTypeIsolated) would help you construct these URLs

That isn't implemented for isolated file systems, I think intentionally as the
URL syntax is not exposed. Kinuko - is that correct?

[benwells, 2012-05-18 05:05:40]

+cc mpcomplete

Matt - please see question above about AddPendingTask. Summary: am I using it
correctly?

[kinuko, 2012-05-18 10:22:16]

Nice to see Isolated feature is nicely utilized in this neat feature!

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:119:
ExtensionSystem::Get(profile_)->process_manager();
nit: indent

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
File chrome/renderer/extensions/experimental.app_custom_bindings.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
chrome/renderer/extensions/experimental.app_custom_bindings.cc:29:
name.append("isolated/");
On 2012/05/18 04:56:55, benwells wrote:
> On 2012/05/18 04:26:04, Mihai Parparita wrote:
> > Seems like you can use GetFileSystemRootURI(context_url,
> > kFileSystemTypeIsolated) would help you construct these URLs
> 
> That isn't implemented for isolated file systems, I think intentionally as the
> URL syntax is not exposed. Kinuko - is that correct?

It was correct, but I just added the minimal support in the fileapi code as
seeing a constant string like "isolated" here doesn't feel right.

Can you use fileapi::GetFileSystemRootURI() and fileapi::GetFileSystemName() for
the common part, and just append |file_system_id| at the end of the returned
value?

[Greg Billock, 2012-05-18 14:14:39]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.cc:134: intent->SetString("type",
mime_type);
Our documentation for MIME type passing is here:
http://www.w3.org/wiki/WebIntents/MIME_Types.

Due to FileEntry not being serializable, we've used blobs for this. I think for
View a Blob works fine in general, but given the context, a FileEntry is a
better choice. I'd suggest passing a different type name to make sure the app
can receive both types of intents.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
If the data is always a FileEntry, the type should be more-or-less fixed as
well.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.h:49: //     "postResults" : a null
function
The spec uses two return functions: "postResult" and "postFailure".

[Matt Perry, 2012-05-18 19:02:39]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:126: // The host isn't loaded
yet, so we post a task to the queue for the host.
On 2012/05/18 04:56:55, benwells wrote:
> On 2012/05/18 04:26:04, Mihai Parparita wrote:
> > Can this be racy (the host was about to be shut down and/or is in the
process
> of
> > starting up)? Is it safer to always use AddPendingTask?
> 
> Good point. I initially used AddPendingTask but it doesn't send the task if
the
> host is already loaded. This pattern of checking for the host and adding a
> pending task is used elsewhere (e.g.
> ExtensionSettingsHandler::HandleInspectMessage).
> 
> I haven't investigated to see if this could be racy. Matt - is this usage
> correct? Is there a reason why AddPendingTask can't dispatch immediately if
the
> host is available to simplify usage?

Thanks for checking. Rather than checking if host is NULL, use
LazyBackgroundTaskQueue::ShouldEnqueueTask(). If that returns NULL, then you can
get the background host and call GrantAccessToFileAndLaunch.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:129:
ExtensionSystem::Get(profile_)->lazy_background_task_queue();
indent += 2

[Matt Perry, 2012-05-18 19:04:37]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:126: // The host isn't loaded
yet, so we post a task to the queue for the host.
On 2012/05/18 19:02:41, Matt Perry wrote:
> On 2012/05/18 04:56:55, benwells wrote:
> > On 2012/05/18 04:26:04, Mihai Parparita wrote:
> > > Can this be racy (the host was about to be shut down and/or is in the
> process
> > of
> > > starting up)? Is it safer to always use AddPendingTask?
> > 
> > Good point. I initially used AddPendingTask but it doesn't send the task if
> the
> > host is already loaded. This pattern of checking for the host and adding a
> > pending task is used elsewhere (e.g.
> > ExtensionSettingsHandler::HandleInspectMessage).
> > 
> > I haven't investigated to see if this could be racy. Matt - is this usage
> > correct? Is there a reason why AddPendingTask can't dispatch immediately if
> the
> > host is available to simplify usage?
> 
> Thanks for checking. Rather than checking if host is NULL, use
> LazyBackgroundTaskQueue::ShouldEnqueueTask(). If that returns NULL, then you
can
> get the background host and call GrantAccessToFileAndLaunch.

Sorry, I meant to say "if ShouldEnqueueTask returns false".

For more context, this method does a bit more checking, i.e. that the host is
loaded and not about to shut down.

[benwells, 2012-05-21 04:26:32]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.cc:134: intent->SetString("type",
mime_type);
On 2012/05/18 14:14:39, Greg Billock wrote:
> Our documentation for MIME type passing is here:
> http://www.w3.org/wiki/WebIntents/MIME_Types.
> 
> Due to FileEntry not being serializable, we've used blobs for this. I think
for
> View a Blob works fine in general, but given the context, a FileEntry is a
> better choice. I'd suggest passing a different type name to make sure the app
> can receive both types of intents.

Greg: I want to make sure I understand properly: do you mean don't put the MIME
type, but instead something fixed like "fileentry/"?

Also, after reading the linked documentation (thanks!) I think I'll add a
filename field as well as it will be quite useful.

[Greg Billock, 2012-05-21 14:25:34]

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.cc:134: intent->SetString("type",
mime_type);
Right. We don't really have a limitation on the type string, but so far we're
namespacing as MIME-types (which should really be mime types), or urls (for
microdata compat). For a FileEntry, perhaps put something using the chrome
extension url prefix, and then just "fileentry"?

On 2012/05/21 04:26:32, benwells wrote:
> On 2012/05/18 14:14:39, Greg Billock wrote:
> > Our documentation for MIME type passing is here:
> > http://www.w3.org/wiki/WebIntents/MIME_Types.
> > 
> > Due to FileEntry not being serializable, we've used blobs for this. I think
> for
> > View a Blob works fine in general, but given the context, a FileEntry is a
> > better choice. I'd suggest passing a different type name to make sure the
app
> > can receive both types of intents.
> 
> Greg: I want to make sure I understand properly: do you mean don't put the
MIME
> type, but instead something fixed like "fileentry/"?
> 
> Also, after reading the linked documentation (thanks!) I think I'll add a
> filename field as well as it will be quite useful.

[benwells, 2012-05-22 13:15:02]

+sky as chrome/browser/ui owner
+avi as content/ owner

sky: this will clash with the other change, i'll merge it once that one lands
(its stuck in the queue)

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.cc:134: intent->SetString("type",
mime_type);
On 2012/05/21 14:25:34, Greg Billock wrote:
> Right. We don't really have a limitation on the type string, but so far we're
> namespacing as MIME-types (which should really be mime types), or urls (for
> microdata compat). For a FileEntry, perhaps put something using the chrome
> extension url prefix, and then just "fileentry"?
> 
> On 2012/05/21 04:26:32, benwells wrote:
> > On 2012/05/18 14:14:39, Greg Billock wrote:
> > > Our documentation for MIME type passing is here:
> > > http://www.w3.org/wiki/WebIntents/MIME_Types.
> > > 
> > > Due to FileEntry not being serializable, we've used blobs for this. I
think
> > for
> > > View a Blob works fine in general, but given the context, a FileEntry is a
> > > better choice. I'd suggest passing a different type name to make sure the
> app
> > > can receive both types of intents.
> > 
> > Greg: I want to make sure I understand properly: do you mean don't put the
> MIME
> > type, but instead something fixed like "fileentry/"?
> > 
> > Also, after reading the linked documentation (thanks!) I think I'll add a
> > filename field as well as it will be quite useful.
> 

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/api/app/app_api.h (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.h:47: //     "type" : |mime_type|,
On 2012/05/18 14:14:39, Greg Billock wrote:
> If the data is always a FileEntry, the type should be more-or-less fixed as
> well.

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.h:49: //     "postResults" : a null
function
On 2012/05/18 14:14:39, Greg Billock wrote:
> The spec uses two return functions: "postResult" and "postFailure".

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:106: for (size_t i=0;
i<args.size(); i++)
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Nit: add space before/after = and <

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:34: const char kViewIntent[]
= "http://webintents.org/view";
On 2012/05/18 04:56:55, benwells wrote:
> On 2012/05/18 04:26:04, Mihai Parparita wrote:
> > Rather than having this as a a constant here and
> > DispatchOnLaunchedEventWithFileEntry taking a string action, it seems less
> > error-prone to have the function take an enum, and map it to the intent
string
> > inside the function.
> 
> How is it error-prone? Do you mean if the code uses different strings, or
> doesn't use the constant?
> 
> I talked to Greg about putting the constant somewhere and he wants to wait a
> while. Maybe in the meantime we could put it in app_api.h, or pass through the
> WebIntentServiceData to the Launch function (it has the action in it).

After talking to Mihai will leave as is.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:103: found_service = true;
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Add a break here.

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:119:
ExtensionSystem::Get(profile_)->process_manager();
On 2012/05/18 10:22:17, kinuko wrote:
> nit: indent

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:126: // The host isn't loaded
yet, so we post a task to the queue for the host.
On 2012/05/18 19:02:41, Matt Perry wrote:
> On 2012/05/18 04:56:55, benwells wrote:
> > On 2012/05/18 04:26:04, Mihai Parparita wrote:
> > > Can this be racy (the host was about to be shut down and/or is in the
> process
> > of
> > > starting up)? Is it safer to always use AddPendingTask?
> > 
> > Good point. I initially used AddPendingTask but it doesn't send the task if
> the
> > host is already loaded. This pattern of checking for the host and adding a
> > pending task is used elsewhere (e.g.
> > ExtensionSettingsHandler::HandleInspectMessage).
> > 
> > I haven't investigated to see if this could be racy. Matt - is this usage
> > correct? Is there a reason why AddPendingTask can't dispatch immediately if
> the
> > host is available to simplify usage?
> 
> Thanks for checking. Rather than checking if host is NULL, use
> LazyBackgroundTaskQueue::ShouldEnqueueTask(). If that returns NULL, then you
can
> get the background host and call GrantAccessToFileAndLaunch.

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:129:
ExtensionSystem::Get(profile_)->lazy_background_task_queue();
On 2012/05/18 19:02:41, Matt Perry wrote:
> indent += 2

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:148: std::set<FilePath>
filesets;
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Nit: this can be moved a few lines lower, since it's not needed to grant
access.

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
File chrome/renderer/extensions/experimental.app_custom_bindings.cc (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
chrome/renderer/extensions/experimental.app_custom_bindings.cc:27: GURL
context_url = UserScriptSlave::GetDataSourceURLForFrame(webframe);
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Might be safe to add a CHECK that this is an extension URL.

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/extensions...
chrome/renderer/extensions/experimental.app_custom_bindings.cc:29:
name.append("isolated/");
On 2012/05/18 10:22:17, kinuko wrote:
> On 2012/05/18 04:56:55, benwells wrote:
> > On 2012/05/18 04:26:04, Mihai Parparita wrote:
> > > Seems like you can use GetFileSystemRootURI(context_url,
> > > kFileSystemTypeIsolated) would help you construct these URLs
> > 
> > That isn't implemented for isolated file systems, I think intentionally as
the
> > URL syntax is not exposed. Kinuko - is that correct?
> 
> It was correct, but I just added the minimal support in the fileapi code as
> seeing a constant string like "isolated" here doesn't feel right.
> 
> Can you use fileapi::GetFileSystemRootURI() and fileapi::GetFileSystemName()
for
> the common part, and just append |file_system_id| at the end of the returned
> value?

Nice! Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/resources/...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:13:
function(launchData, file_system_id, base_name) {
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Nit: JS naming style is to use camelCase for variable names.

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/renderer/resources/...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:21:
chrome.Event.prototype.dispatch.apply(event, [launchData]);
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> Nit: if you use .call(), you can pass the arguments directly (so that you
don't
> need the empty array when you don't have any additional arguments).

Done.

http://codereview.chromium.org/10332071/diff/10073/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/launch_file/test.js (right):

http://codereview.chromium.org/10332071/diff/10073/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/launch_file/test.js:12:
chrome.test.assertTrue(
On 2012/05/18 04:26:04, Mihai Parparita wrote:
> You can use chrome.test.assertEq, so that you don't need to provide your own
> failure message.

Done.

http://codereview.chromium.org/10332071/diff/38/chrome/browser/extensions/api...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/10332071/diff/38/chrome/browser/extensions/api...
chrome/browser/extensions/api/app/app_api.cc:138: #if defined(OS_WIN)
This is yuck. Is there a better way to do this? FilePath::StringType is a wide
string on windows.

[benwells, 2012-05-22 13:42:04]

+sky as chrome/browser/ui owner
+avi as content/ owner

sky: this will clash with the other change, i'll merge it once that one lands
(its stuck in the queue)

[sky, 2012-05-22 16:03:52]

LGTM

Could we move all the OpenApplication methods from Browser closer to extensions?

[Matt Perry, 2012-05-22 19:03:30]

http://codereview.chromium.org/10332071/diff/43/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/43/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.cc:138: ExtensionHost* host =
pm->GetBackgroundHostForExtension(extension_->id());
FYI, this will return NULL if the extension has no background page.
(ShouldEnqueueTask also returns false in that case, allowing you to get here.)

[Mihai Parparita -not on Chrome, 2012-05-23 06:17:55]

LGTM

http://codereview.chromium.org/10332071/diff/43/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/43/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.cc:138: ExtensionHost* host =
pm->GetBackgroundHostForExtension(extension_->id());
On 2012/05/22 19:03:30, Matt Perry wrote:
> FYI, this will return NULL if the extension has no background page.

You mean, if one isn't declared in the manifest? We require one for platform
apps
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e...).

[Matt Perry, 2012-05-23 18:53:25]

http://codereview.chromium.org/10332071/diff/43/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10332071/diff/43/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.cc:138: ExtensionHost* host =
pm->GetBackgroundHostForExtension(extension_->id());
On 2012/05/23 06:17:56, Mihai Parparita wrote:
> On 2012/05/22 19:03:30, Matt Perry wrote:
> > FYI, this will return NULL if the extension has no background page.
> 
> You mean, if one isn't declared in the manifest? We require one for platform
> apps
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e...).

Correct. This should be safe, then.

[Matt Perry, 2012-05-23 21:34:35]

lgtm

[benwells, 2012-05-24 00:48:05]

ping for gbillock@, kinuko@ and avi@ - could have have a look at this patch?

avi: i only need you to look at exposing the extra security bits from content.

[kinuko, 2012-05-24 11:55:22]

fileapi related changes LGTM

[Avi (use Gerrit), 2012-05-24 14:37:30]

http://codereview.chromium.org/10332071/diff/37001/content/browser/child_proc...
File content/browser/child_process_security_policy_impl.h (right):

http://codereview.chromium.org/10332071/diff/37001/content/browser/child_proc...
content/browser/child_process_security_policy_impl.h:82: const std::string&
filesystem_id) OVERRIDE;
If this is a part of the ChildProcessSecurityPolicy interface, it should be
declared above with the other functions.

http://codereview.chromium.org/10332071/diff/37001/content/browser/child_proc...
content/browser/child_process_security_policy_impl.h:101: virtual bool
CanReadFile(int child_id, const FilePath& file) OVERRIDE;
ditto

[benwells, 2012-05-24 23:36:54]

https://chromiumcodereview.appspot.com/10332071/diff/37001/content/browser/ch...
File content/browser/child_process_security_policy_impl.h (right):

https://chromiumcodereview.appspot.com/10332071/diff/37001/content/browser/ch...
content/browser/child_process_security_policy_impl.h:82: const std::string&
filesystem_id) OVERRIDE;
On 2012/05/24 14:37:31, Avi wrote:
> If this is a part of the ChildProcessSecurityPolicy interface, it should be
> declared above with the other functions.

Done.

https://chromiumcodereview.appspot.com/10332071/diff/37001/content/browser/ch...
content/browser/child_process_security_policy_impl.h:101: virtual bool
CanReadFile(int child_id, const FilePath& file) OVERRIDE;
On 2012/05/24 14:37:31, Avi wrote:
> ditto

Done.

[Avi (use Gerrit), 2012-05-25 02:13:38]

content lgtm

[commit-bot: I haz the power, 2012-05-25 02:21:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10332071/37002

[commit-bot: I haz the power, 2012-05-25 02:21:30]

Failed to apply patch for chrome/browser/extensions/api/omnibox/omnibox_api.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/api/omnibox/omnibox_api.cc
Hunk #1 FAILED at 321.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/api/omnibox/omnibox_api.cc.rej

[commit-bot: I haz the power, 2012-05-25 06:35:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10332071/42001

[commit-bot: I haz the power, 2012-05-25 07:44:19]

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

[commit-bot: I haz the power, 2012-05-25 08:56:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10332071/28003

[commit-bot: I haz the power, 2012-05-26 05:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10332071/37013

[commit-bot: I haz the power, 2012-05-26 07:09:54]

Change committed as 139195