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

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

This is the first step to making platform apps document editors. This change
allows platform apps to declare that they provide the webintents.org/view
intent, and if so files passed on the command line will be passed through
to the platform app's window.intent object, and the app will be given access
to the file.

BUG=None
TEST=Browser test added

[benwells, 2012-03-23 07:21:50]

There is a hard to reproduce problem with this code but would like to get any
early opinions.

I think the problem is because of a race between the stuff on the file thread,
and the renderer getting set up. If the file thread is too slow the intent isn't
setup properly. Next week I'll look at fixing but any feedback on the approach
is welcome.

https://chromiumcodereview.appspot.com/9845003/diff/3002/chrome/browser/ui/ex...
File chrome/browser/ui/extensions/shell_window.h (left):

https://chromiumcodereview.appspot.com/9845003/diff/3002/chrome/browser/ui/ex...
chrome/browser/ui/extensions/shell_window.h:17: class ExtensionHost;
This is cleanup unrelated to this change.

[James Hawkins, 2012-03-26 03:18:32]

Adding myself to R=.

[benwells, 2012-03-26 03:47:33]

I've avoided the race by using ScopedAllowIO to get the MIME type on the UI
thread.

This should be changed so that the MIME type is retrieved at startup on the FILE
thread, and render view creation delayed until this has happened. I'd like to do
that once the new background page stuff is in and / or the PlatformAppHost
refactoring is in.

[James Hawkins, 2012-03-26 19:55:57]

Mostly style nits.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:200:
IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, DocumentEditor) {
nit: Document what the test is testing.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_intent_dispatcher.cc (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:19: #include
"content/public/browser/browser_thread.h"
nit: Alphabetize includes.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:42: // See if we
have a command line argument.
nit: Don't use 'we' (or pronouns in general) in comments; pronouns are
ambiguous.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:63: const string16
open_action = ASCIIToUTF16("http://webintents.org/view");
s/open_action/kOpenAction/ for const variable.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:89:
ExtensionAPIPermissionSet apis; // empty
nit: Two spaces before start of comments.

Though I'm not sure it's necessary to document that a variable is intentionally
empty.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:97:
host_->extension()->id(),
Optional nit: Can save lines by moving parameters to the previous lines.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_intent_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:15: class
PlatformAppIntentDispatcher
nit: Document the class.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:18: static void
DispatchIntentData(ExtensionHost* host);
nit: Document all methods and parameters.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:24: ExtensionHost*
host_;
nit: Document member variable, especially ownership (weak ptr?).

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:29: } // namespace
extensions
nit: Two spaces before start of comment.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/shell_window.h:48: OVERRIDE;
nit: Blank line before 'protected:'.

http://codereview.chromium.org/9845003/diff/8001/content/public/browser/web_i...
File content/public/browser/web_intents_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/8001/content/public/browser/web_i...
content/public/browser/web_intents_dispatcher.h:67: RenderViewHost*
render_view_host);
nit: Document the parameters.

[benwells, 2012-03-27 04:24:12]

Cleanup and nit addressing.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:200:
IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, DocumentEditor) {
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Document what the test is testing.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_intent_dispatcher.cc (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:19: #include
"content/public/browser/browser_thread.h"
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Alphabetize includes.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:42: // See if we
have a command line argument.
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Don't use 'we' (or pronouns in general) in comments; pronouns are
> ambiguous.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:63: const string16
open_action = ASCIIToUTF16("http://webintents.org/view");
On 2012/03/26 19:55:57, James Hawkins wrote:
> s/open_action/kOpenAction/ for const variable.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:89:
ExtensionAPIPermissionSet apis; // empty
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Two spaces before start of comments.
> 
> Though I'm not sure it's necessary to document that a variable is
intentionally
> empty.

Done (comment removed).

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:97:
host_->extension()->id(),
On 2012/03/26 19:55:57, James Hawkins wrote:
> Optional nit: Can save lines by moving parameters to the previous lines.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_intent_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:15: class
PlatformAppIntentDispatcher
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Document the class.

As this is all happening on the UI thread (for now) I've removed the class.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:18: static void
DispatchIntentData(ExtensionHost* host);
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Document all methods and parameters.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:24: ExtensionHost*
host_;
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Document member variable, especially ownership (weak ptr?).

The class is now removed.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_intent_dispatcher.h:29: } // namespace
extensions
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Two spaces before start of comment.

Done.

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/9845003/diff/8001/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/shell_window.h:48: OVERRIDE;
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Blank line before 'protected:'.

Done.

http://codereview.chromium.org/9845003/diff/8001/content/public/browser/web_i...
File content/public/browser/web_intents_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/8001/content/public/browser/web_i...
content/public/browser/web_intents_dispatcher.h:67: RenderViewHost*
render_view_host);
On 2012/03/26 19:55:57, James Hawkins wrote:
> nit: Document the parameters.

Done.

[James Hawkins, 2012-03-28 00:59:52]

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.cc (right):

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:53: const string16
kOpenAction = ASCIIToUTF16("http://webintents.org/view");
Can we find a better variable name than kOpenAction?  I forgot this string ==
'view' later on in the code.

kViewAction may be better.

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:70: // Give the app
permission to use the file:// scheme.
Just to make sure I'm reading this correctly, the app only has access to the
particular file:// URL passed in |data|, right?

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.h:13: // Transform and
dispatch command line arguments to WebIntent data. A
nit: Function/method comments should be in the declarative form (as opposed to
imperative), so s/Transform/Transforms/ and s/dispatch/dispatches/.

[benwells, 2012-03-28 04:50:26]

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.cc (right):

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:53: const string16
kOpenAction = ASCIIToUTF16("http://webintents.org/view");
On 2012/03/28 00:59:52, James Hawkins wrote:
> Can we find a better variable name than kOpenAction?  I forgot this string ==
> 'view' later on in the code.
> 
> kViewAction may be better.

Done.

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:70: // Give the app
permission to use the file:// scheme.
On 2012/03/28 00:59:52, James Hawkins wrote:
> Just to make sure I'm reading this correctly, the app only has access to the
> particular file:// URL passed in |data|, right?

Yes that's right. I've tried to make it more obvious from the comments, let me
know if its still unclear.

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.h:13: // Transform and
dispatch command line arguments to WebIntent data. A
On 2012/03/28 00:59:52, James Hawkins wrote:
> nit: Function/method comments should be in the declarative form (as opposed to
> imperative), so s/Transform/Transforms/ and s/dispatch/dispatches/.

Done.

[James Hawkins, 2012-03-28 05:17:40]

LGTM with nit.

http://codereview.chromium.org/9845003/diff/23001/content/browser/intents/web...
File content/browser/intents/web_intents_direct_dispatch.cc (right):

http://codereview.chromium.org/9845003/diff/23001/content/browser/intents/web...
content/browser/intents/web_intents_direct_dispatch.cc:13:
render_view_host->Send(new IntentsMsg_SetWebIntentData(
nit: Add include for RenderViewHost.

[benwells, 2012-03-28 07:47:59]

+avi for content/public changes

http://codereview.chromium.org/9845003/diff/23001/content/browser/intents/web...
File content/browser/intents/web_intents_direct_dispatch.cc (right):

http://codereview.chromium.org/9845003/diff/23001/content/browser/intents/web...
content/browser/intents/web_intents_direct_dispatch.cc:13:
render_view_host->Send(new IntentsMsg_SetWebIntentData(
On 2012/03/28 05:17:41, James Hawkins wrote:
> nit: Add include for RenderViewHost.

Done, with some other include changes to make it (hopefully) correct style and
IWYU compliant.

[Greg Billock, 2012-03-28 14:21:02]

Our data type for MIME may need to be broadened to always support URLs as data.
This would definitely be convenient for RCH dispatch, btw... For large text
types, that may be what we need to end up doing anyway.

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.cc (right):

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:53: const string16
kOpenAction = ASCIIToUTF16("http://webintents.org/view");
Can you put this constant in webkit/glue/web_intent_data? There'll be at least
two uses of it already shortly.

On 2012/03/28 04:50:27, benwells wrote:
> On 2012/03/28 00:59:52, James Hawkins wrote:
> > Can we find a better variable name than kOpenAction?  I forgot this string
==
> > 'view' later on in the code.
> > 
> > kViewAction may be better.
> 
> Done.

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:70: // Give the app
permission to use the file:// scheme.
Don't you just want GrantReadFile?

On 2012/03/28 04:50:27, benwells wrote:
> On 2012/03/28 00:59:52, James Hawkins wrote:
> > Just to make sure I'm reading this correctly, the app only has access to the
> > particular file:// URL passed in |data|, right?
> 
> Yes that's right. I've tried to make it more obvious from the comments, let me
> know if its still unclear.

http://codereview.chromium.org/9845003/diff/25002/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/25002/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.h:23: //   - data will
have the filename as a file:/// URL.
The current thinking is that if the contents are binary, they'll be set as a URL
(Blob), and if they are text, they'll come in as text. Do we need to account for
that here? That is, if your file type is text, the service is going to think the
content is "file://xxx".

http://codereview.chromium.org/9845003/diff/25002/content/browser/intents/web...
File content/browser/intents/web_intents_direct_dispatch.cc (right):

http://codereview.chromium.org/9845003/diff/25002/content/browser/intents/web...
content/browser/intents/web_intents_direct_dispatch.cc:11: void
DirectDispatchIntent(webkit_glue::WebIntentData intent_data,
We've been calling "dispatch" sending the intent up from the client. Do you want
to call this DirectSetIntent?

http://codereview.chromium.org/9845003/diff/25002/content/public/browser/web_...
File content/public/browser/web_intents_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/25002/content/public/browser/web_...
content/public/browser/web_intents_dispatcher.h:63: // This function is used
when the intent provider render view is already known,
Why not use the internal dispatcher, so the return pathways are hooked up? You
can simply not set the callback, but that'll give you a hook if you need it.

[benwells, 2012-03-29 03:29:18]

Some comment responses. I'll be waiting until
http://codereview.chromium.org/9651020/ lands (and gives me the extra_data
intent field) before putting up another patch.

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.cc (right):

http://codereview.chromium.org/9845003/diff/14004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.cc:70: // Give the app
permission to use the file:// scheme.
On 2012/03/28 14:21:02, Greg Billock wrote:
> Don't you just want GrantReadFile?
> 
> On 2012/03/28 04:50:27, benwells wrote:
> > On 2012/03/28 00:59:52, James Hawkins wrote:
> > > Just to make sure I'm reading this correctly, the app only has access to
the
> > > particular file:// URL passed in |data|, right?
> > 
> > Yes that's right. I've tried to make it more obvious from the comments, let
me
> > know if its still unclear.
> 

GrantReadFile doesn't do what I want, as WebKit (or wherever the cross origin
policy stuff lives) will filter out any XHR reads before it gets to the browser
process.

It might be correct to also use GrantReadFile, however I found it not necessary
for XHR requests (but back when I was playing with Blob stuff I think I remember
it was needed).

http://codereview.chromium.org/9845003/diff/25002/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_intent_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/25002/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_intent_dispatcher.h:23: //   - data will
have the filename as a file:/// URL.
On 2012/03/28 14:21:02, Greg Billock wrote:
> The current thinking is that if the contents are binary, they'll be set as a
URL
> (Blob), and if they are text, they'll come in as text. Do we need to account
for
> that here? That is, if your file type is text, the service is going to think
the
> content is "file://xxx".

Greg and I chatted about this and he filled me in on the work being done on the
payload data formats. As a result I'm going to hold off on this patch until
another change lands and then update to put the URL in the extras field.

http://codereview.chromium.org/9845003/diff/25002/content/public/browser/web_...
File content/public/browser/web_intents_dispatcher.h (right):

http://codereview.chromium.org/9845003/diff/25002/content/public/browser/web_...
content/public/browser/web_intents_dispatcher.h:63: // This function is used
when the intent provider render view is already known,
On 2012/03/28 14:21:02, Greg Billock wrote:
> Why not use the internal dispatcher, so the return pathways are hooked up? You
> can simply not set the callback, but that'll give you a hook if you need it.

OK, that makes sense. So instead of this function there would be a function like
WebIntentsDispatcher CreateInternalWebIntentsDispatcher(WebIntentData).

After creating one of these in the browser process I'd then call
DispatchIntent(web_contents).

[benwells, 2012-05-09 06:00:10]

Closing this issue as the code has changed dramatically. Will post a new cl.