Allow platform apps to respond to Web Intents

This adds WebIntentCallbacks, which is a ProfileKeyedService. It manages WebIntentsDispatcher instances which represent the callback type for Web Intents currently being handled by packaged apps. These dispatchers are cleared on reply, or when the source WebContents disappears.

This patch also adds a hidden API call which allows packaged apps to actually send their response, this is in app_runtime_api.cc.

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

[thorogood, 2012-08-08 02:45:25]

Koz, can you PTAL? I've broken this out into a seperate class, although it still
will leak memory if intents aren't responded to.

[koz (OOO until 15th September), 2012-08-08 04:47:30]

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/app/app_api.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/app/app_api.cc:133: if
(details->HasKey(kIntentIdKey))
Is this not always present?

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/app/app_api.cc:154: Value* result;
Initialize to NULL.

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:23: // The WebIntentsCallback
class tracks the pending callbacks for web intents
s/WebIntentsCallback/WebIntentCallbacks/

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:36: std::string
RegisterCallback(content::WebIntentsDispatcher* dispatcher);
Comments on this and RetrieveCallback().

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/ui/i...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:290: extension,
intents_dispatcher_);
Hm. So looking at the code below a bit more it seems that there are a few things
we need to consider

- when the source tab gets closed, we need to clean up
- if the app gets suspended or uninstalled, we need to clean up

When the source tab is closed I wonder if we can just kill the mapping and
ignore any responses from the app.

When the app gets suspended, it will lose its JS reference, and thus any ability
to respond to this web intent, so in that case we can also just kill the
mapping, and we'd need to notify the source tab that this has happened.

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/common/exten...
File chrome/common/extensions/api/experimental_app.json (right):

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/common/exten...
chrome/common/extensions/api/experimental_app.json:85: "description": "stuff",
Remove or make meaningful.

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/renderer/res...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/2001/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:24:
fn({ 'intentId': id, 'success': true, 'data': data });
no spaces after curlies, here and below.

[thorogood, 2012-08-08 07:15:49]

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

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.cc:133: if
(details->HasKey(kIntentIdKey))
On 2012/08/08 04:47:30, koz wrote:
> Is this not always present?

I've removed the HasKey checks, since yes, EXTENSION_FUNCTION_VALIDATE should
fail if the keys don't exist.

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/app/app_api.cc:154: Value* result;
On 2012/08/08 04:47:30, koz wrote:
> Initialize to NULL.

No longer relevant, as now I'm using a std::string containing pre-serialized
data, so that the data flows through properly (this is a value generated from
"WebSerializedScriptValue::serialize", which is what SendReplyMessage expects)/

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/extensions/w...
File chrome/browser/extensions/web_intent_callbacks.h (right):

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.h:23: // The WebIntentsCallback
class tracks the pending callbacks for web intents
On 2012/08/08 04:47:30, koz wrote:
> s/WebIntentsCallback/WebIntentCallbacks/

Thanks. I'm not sure about this as the actual classname also, but for now at
least it's consistent =)

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.h:36: std::string
RegisterCallback(content::WebIntentsDispatcher* dispatcher);
On 2012/08/08 04:47:30, koz wrote:
> Comments on this and RetrieveCallback().

Done.

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10828172/diff/2001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:290: extension,
intents_dispatcher_);
On 2012/08/08 04:47:30, koz wrote:
> Hm. So looking at the code below a bit more it seems that there are a few
things
> we need to consider
> 
> - when the source tab gets closed, we need to clean up
> - if the app gets suspended or uninstalled, we need to clean up
> 
> When the source tab is closed I wonder if we can just kill the mapping and
> ignore any responses from the app.
> 
> When the app gets suspended, it will lose its JS reference, and thus any
ability
> to respond to this web intent, so in that case we can also just kill the
> mapping, and we'd need to notify the source tab that this has happened.

We'd need to call postFailure with null, which is what happens in other cases
(there's no "magic message" used for different circumstances).

so, I agree to all this: I'm going to dig around (or ask you) how to do these
things, but not in this immediate reply batch.

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

http://codereview.chromium.org/10828172/diff/2001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_app.json:85: "description": "stuff",
On 2012/08/08 04:47:30, koz wrote:
> Remove or make meaningful.

meaningful :)

http://codereview.chromium.org/10828172/diff/2001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

http://codereview.chromium.org/10828172/diff/2001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:24:
fn({ 'intentId': id, 'success': true, 'data': data });
On 2012/08/08 04:47:30, koz wrote:
> no spaces after curlies, here and below.

I've put the dictionary definition on multiple lines, so should be irrelevant
now.

[Mihai Parparita -not on Chrome, 2012-08-09 04:38:33]

(some drive-by comments)

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
File chrome/browser/extensions/web_intent_callbacks.cc (right):

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.cc:21: std::string key =
StringPrintf("key_%d", last_id_++);
Can keys somehow be scoped by the extension ID of the app that they're intended
for? Otherwise an app could call chrome.experimental.app.postIntentResponse
directly with another app's intent ID to spoof it's response.

Alternatively, you could make it so that postIntentResponse can't be called by
app code by moving it to an internal namespace.

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.cc:23: // TODO(thorogood):
cleanup task, perhaps we can watch renderer threads.
Can you do this cleanup when the launchData object gets GC-ed? There's a
BindToGC native function that you can use in the bindings, see
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/resources/...
for an example.

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
File chrome/browser/extensions/web_intent_callbacks.h (right):

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.h:25: class WebIntentCallbacks :
public ProfileKeyedService {
This should either be in the extensions namespace (preferable), or have
"Extension" somewhere in the class name, since otherwise it sounds very generic,
even though the use is extensions/apps-specific.

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.h:27: typedef
std::map<std::string, content::WebIntentsDispatcher*> CallbackMap;
This doesn't seem to need to be public.

[thorogood, 2012-08-09 09:56:50]

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
File chrome/browser/extensions/web_intent_callbacks.cc (right):

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.cc:21: std::string key =
StringPrintf("key_%d", last_id_++);
On 2012/08/09 04:38:33, Mihai Parparita wrote:
> Can keys somehow be scoped by the extension ID of the app that they're
intended
> for? Otherwise an app could call chrome.experimental.app.postIntentResponse
> directly with another app's intent ID to spoof it's response.
> 
> Alternatively, you could make it so that postIntentResponse can't be called by
> app code by moving it to an internal namespace.

I've taken the first approach, and scoped the key by the extension ID.

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.cc:23: // TODO(thorogood):
cleanup task, perhaps we can watch renderer threads.
On 2012/08/09 04:38:33, Mihai Parparita wrote:
> Can you do this cleanup when the launchData object gets GC-ed? There's a
> BindToGC native function that you can use in the bindings, see
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/resources/...
> for an example.

So I haven't solved this yet, but we talked about it around the office here:
GCing seems to happen unpredictably at some point, and potentially not at all if
the extension were to shut down immediately or due to a crash. So that wouldn't
clear the reference.

Instead: I'm probably going to add a helper class within WebIntentCallbacks that
implements WebContentObserver, and instantiate an instance of that whenever I
invoke RegisterCallback: attaching a unique observer to the WebContents of the
extension page being delivered the intent, for every given pending intent (since
this is a low number). Then when the extension handling the intent goes away, so
does this callback.

On the other flipside, we already mostly deal with the source renderer going
away: WebIntentsDispatcherImpl uses WebContentsObserver to delete its inner
reference to its response mechanism when the source disappears.

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
File chrome/browser/extensions/web_intent_callbacks.h (right):

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.h:25: class WebIntentCallbacks :
public ProfileKeyedService {
On 2012/08/09 04:38:33, Mihai Parparita wrote:
> This should either be in the extensions namespace (preferable), or have
> "Extension" somewhere in the class name, since otherwise it sounds very
generic,
> even though the use is extensions/apps-specific.

I mostly inspired this class from ShellWindowRegistry (in the same folder as
this), which has no namespace. I can add the extension namespace though -- done.

http://codereview.chromium.org/10828172/diff/4005/chrome/browser/extensions/w...
chrome/browser/extensions/web_intent_callbacks.h:27: typedef
std::map<std::string, content::WebIntentsDispatcher*> CallbackMap;
On 2012/08/09 04:38:33, Mihai Parparita wrote:
> This should either be in the extensions namespace (preferable), or have
> "Extension" somewhere in the class name, since otherwise it sounds very
generic,
> even though the use is extensions/apps-specific.

I mostly inspired this class from ShellWindowRegistry (in the same folder as
this), which has no namespace. I can add the extension namespace though, done.

[thorogood, 2012-08-10 08:23:26]

PTAL: I'm seeing some bugs, but I've taken a slightly new approach. We should
clear pending intent callbacks when the caller goes away.

[Mihai Parparita -not on Chrome, 2012-08-11 00:25:44]

BTW, you'll probably want to add a test too.

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
chrome/browser/extensions/web_intent_callbacks.h:51: void
InternalClearCallback(std::string key);
This appears to be unused (and undefined).

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/renderer/re...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/renderer/re...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:17: var
id = args[0];
Nit: call this intentId.

[koz (OOO until 15th September), 2012-08-13 00:29:40]

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/api/app/app_api.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/app/app_api.cc:128: DictionaryValue* details;
nit: initialize to NULL.

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/app/app_api.cc:130:
EXTENSION_FUNCTION_VALIDATE(details != NULL);
I think this case is handled by the previous line, so you can omit this.

[thorogood, 2012-08-13 05:00:12]

I definitely need to add a test: but for now, I've hooked up the code such that
the basic functionality is all there (including cleanup on source WebContent
destruction).

PTAL! :)

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/api/app/app_api.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/app/app_api.cc:128: DictionaryValue* details;
On 2012/08/13 00:29:40, koz wrote:
> nit: initialize to NULL.

Done.

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/app/app_api.cc:130:
EXTENSION_FUNCTION_VALIDATE(details != NULL);
On 2012/08/13 00:29:40, koz wrote:
> I think this case is handled by the previous line, so you can omit this.

Done.

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/browser/ext...
chrome/browser/extensions/web_intent_callbacks.h:51: void
InternalClearCallback(std::string key);
On 2012/08/11 00:25:45, Mihai Parparita wrote:
> This appears to be unused (and undefined).

Removed, I used to use this internally.

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/renderer/re...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/11001/chrome/renderer/re...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:17: var
id = args[0];
On 2012/08/11 00:25:45, Mihai Parparita wrote:
> Nit: call this intentId.

Done.

[benwells, 2012-08-13 08:27:29]

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/shell_window_registry.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/shell_window_registry.h:51: // a convenience wrapper
around ShellWindowRegistry::Factory::GetForProfile.
Nice catch!

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:13: #include
"webkit/glue/web_intent_reply_data.h"
Are all these includes needed?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:32: WebIntentCallbacks*
callbacks,
Indentation

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:40: virtual void
WebContentsDestroyed(content::WebContents* web_contents) {
Missing OVERRIDE

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:50:
WebIntentCallbacks::WebIntentCallbacks() {}
Can you initialize last_id_ to zero here?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:70: // TODO(thorogood): Note
that if the caller is itself a platform app, then
What is the TODO?

Is this behavior comparable to platform apps dispatching intents to normal
pages?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:78: LOG(WARNING) << "No source
WebContents for WebIntent; "
Can this happen? If it should not be able to happen maybe we should have
DCHECK(source).

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:15: #include
"ui/gfx/native_widget_types.h"
Are all these includes needed?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:30: class WebIntentCallbacks :
public ProfileKeyedService {
I know we're now in the extensions namespace, but it might be good to make it
clear this is just for new-style-packaged apps, not extensions in general.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:53: typedef
std::map<std::string, content::WebIntentsDispatcher*> CallbackMap;
Is there any chance of us holding onto the dispatcher after it's deleted? Can we
hold a reference or something so we're not relying on assumptions about how
other code (which might change) works?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/common/exten...
File chrome/common/extensions/api/experimental_app.json (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/common/exten...
chrome/common/extensions/api/experimental_app.json:106: // TODO(koz): Add
MessageChannel parameters.
Is this meant to be TODO(koz)?
Until this is clearer maybe just remove the TODO (and maybe log a bug instead,
if there isn't one already).

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:18: var
launchData = args[1];
This will break the case where we launch with files, see
DispatchOnLaunchedEventWithFileEntry in app_api.cc. It won't have an intentId,
and it should have empty callbacks as there is no other end to call back into.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:26:
'data': SerializeToString(data)
What happens if the data cannot be serialized? E.g. a FileEntry?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:39:
launchData.intent.data = fileEntry;
This is the case where there should be empty callbacks still.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:66:
dispatch([launchData]);
It looks like there *was* a bug here where the null callbacks weren't being
added for Unserialized string data. You've fixed it, yay!

[thorogood, 2012-08-14 02:59:07]

Thanks for the thorough review!

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:13: #include
"webkit/glue/web_intent_reply_data.h"
On 2012/08/13 08:27:29, benwells wrote:
> Are all these includes needed?

I've removed a couple - thanks.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:32: WebIntentCallbacks*
callbacks,
On 2012/08/13 08:27:29, benwells wrote:
> Indentation

Done.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:40: virtual void
WebContentsDestroyed(content::WebContents* web_contents) {
On 2012/08/13 08:27:29, benwells wrote:
> Missing OVERRIDE

Done - although I've copied this inner class mostly from
web_intent_picker_controller.cc, which has a class called SourceWindowObserver.
It does not have OVERRIDE.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:50:
WebIntentCallbacks::WebIntentCallbacks() {}
On 2012/08/13 08:27:29, benwells wrote:
> Can you initialize last_id_ to zero here?

Done.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:70: // TODO(thorogood): Note
that if the caller is itself a platform app, then
On 2012/08/13 08:27:29, benwells wrote:
> What is the TODO?
> 
> Is this behavior comparable to platform apps dispatching intents to normal
> pages?

So, I've rewritten the comment to make it clearer. Yes, it's comparable,
although the case you describe currently segfaults (I filed a bug yesterday:
b/141812). I believe it would have the same problem: the scope of the web intent
is bound to the lifetime of the source WebContents, not the source packaged app.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:78: LOG(WARNING) << "No source
WebContents for WebIntent; "
On 2012/08/13 08:27:29, benwells wrote:
> Can this happen? If it should not be able to happen maybe we should have
> DCHECK(source).

I don't believe so. I originally believed it may occur if we invoked a Web
Intent via the command line on a file, but as you've pointed out that goes
through a different method in app_api.cc. (What about if we invoke it through
the command line without a file?)

I've made your suggestion for now though.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:15: #include
"ui/gfx/native_widget_types.h"
On 2012/08/13 08:27:29, benwells wrote:
> Are all these includes needed?

Done.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:30: class WebIntentCallbacks :
public ProfileKeyedService {
On 2012/08/13 08:27:29, benwells wrote:
> I know we're now in the extensions namespace, but it might be good to make it
> clear this is just for new-style-packaged apps, not extensions in general.

Done.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:53: typedef
std::map<std::string, content::WebIntentsDispatcher*> CallbackMap;
On 2012/08/13 08:27:29, benwells wrote:
> Is there any chance of us holding onto the dispatcher after it's deleted? Can
we
> hold a reference or something so we're not relying on assumptions about how
> other code (which might change) works?

So: I've done a bit of checking. What I've implicitly done here is, on
RegisterCallback, ownership of the WebIntentsDispatcher is actually by this
class. I've documented this explicitly, and ensured that in our cleanup  case,
we delete the dispatcher.

On RetrieveCallback, we transfer ownership to the caller, who then invokes
SendReplyMessage, which causes the object to be deleted. I've also documented
the RetrieveCallback method.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/common/exten...
File chrome/common/extensions/api/experimental_app.json (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/common/exten...
chrome/common/extensions/api/experimental_app.json:106: // TODO(koz): Add
MessageChannel parameters.
On 2012/08/13 08:27:29, benwells wrote:
> Is this meant to be TODO(koz)?
> Until this is clearer maybe just remove the TODO (and maybe log a bug instead,
> if there isn't one already).

I've removed the TODO, but I think I need to investigate this further before I
can file a bug. I'm not sure of the general state of Intents holding onto these
'channels', and whether it works at all or is just some tentative part of the
spec.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:18: var
launchData = args[1];
On 2012/08/13 08:27:29, benwells wrote:
> This will break the case where we launch with files, see
> DispatchOnLaunchedEventWithFileEntry in app_api.cc. It won't have an intentId,
> and it should have empty callbacks as there is no other end to call back into.

Thanks for the tip. I've dealt with this by using a zero intentId in that
method, and checking for it just below.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:26:
'data': SerializeToString(data)
On 2012/08/13 08:27:29, benwells wrote:
> What happens if the data cannot be serialized? E.g. a FileEntry?

I suspect this won't work (but I'm not sure if replying to regular Web Intents
works either). Added a TODO, and I'm going to try to test this now.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:39:
launchData.intent.data = fileEntry;
On 2012/08/13 08:27:29, benwells wrote:
> This is the case where there should be empty callbacks still.

Noted, I've implemented this differently though (checking for zero intentId
above).

[thorogood, 2012-08-14 05:59:42]

+gbillock, who wrote a bunch of Web Intents stuff

[benwells, 2012-08-14 06:57:07]

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:30: class WebIntentCallbacks :
public ProfileKeyedService {
On 2012/08/14 02:59:08, thorogood wrote:
> On 2012/08/13 08:27:29, benwells wrote:
> > I know we're now in the extensions namespace, but it might be good to make
it
> > clear this is just for new-style-packaged apps, not extensions in general.
> 
> Done.

Ah .. I meant by changing the class name, but I'm not too fussed.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:18: var
launchData = args[1];
On 2012/08/14 02:59:08, thorogood wrote:
> On 2012/08/13 08:27:29, benwells wrote:
> > This will break the case where we launch with files, see
> > DispatchOnLaunchedEventWithFileEntry in app_api.cc. It won't have an
intentId,
> > and it should have empty callbacks as there is no other end to call back
into.
> 
> Thanks for the tip. I've dealt with this by using a zero intentId in that
> method, and checking for it just below.

A nicer way to do this would be to have the intentId be the last parameter (i.e.
[2]), leave your check here as is and when calling with the fileEntry case just
don't add an intentId. That way we don't need a special value of intentId to
mean something.

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:26:
'data': SerializeToString(data)
On 2012/08/14 02:59:08, thorogood wrote:
> On 2012/08/13 08:27:29, benwells wrote:
> > What happens if the data cannot be serialized? E.g. a FileEntry?
> 
> I suspect this won't work (but I'm not sure if replying to regular Web Intents
> works either). Added a TODO, and I'm going to try to test this now.

You should make sure there are no explosions.

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:94: return NULL;
Blank line after early return.

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:68:
content::WebIntentsDispatcher* InternalRetrieveCallback(std::string key);
There should be a comment here to say you're also removing the callback from the
map (or update the name to say it).

[thorogood, 2012-08-15 06:15:49]

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:30: class WebIntentCallbacks :
public ProfileKeyedService {
On 2012/08/14 06:57:07, benwells wrote:
> On 2012/08/14 02:59:08, thorogood wrote:
> > On 2012/08/13 08:27:29, benwells wrote:
> > > I know we're now in the extensions namespace, but it might be good to make
> it
> > > clear this is just for new-style-packaged apps, not extensions in general.
> > 
> > Done.
> 
> Ah .. I meant by changing the class name, but I'm not too fussed.

Ok. What about PlatformAppIntentRegistry?

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
File chrome/renderer/resources/extensions/experimental.app_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10828172/diff/5003/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.app_custom_bindings.js:18: var
launchData = args[1];
On 2012/08/14 06:57:07, benwells wrote:
> On 2012/08/14 02:59:08, thorogood wrote:
> > On 2012/08/13 08:27:29, benwells wrote:
> > > This will break the case where we launch with files, see
> > > DispatchOnLaunchedEventWithFileEntry in app_api.cc. It won't have an
> intentId,
> > > and it should have empty callbacks as there is no other end to call back
> into.
> > 
> > Thanks for the tip. I've dealt with this by using a zero intentId in that
> > method, and checking for it just below.
> 
> A nicer way to do this would be to have the intentId be the last parameter
(i.e.
> [2]), leave your check here as is and when calling with the fileEntry case
just
> don't add an intentId. That way we don't need a special value of intentId to
> mean something.

Done, although I've had to modify DispatchOnLaunchedEventWithWebIntent to
_always_ insert a null value for launchData on the unserialized string argument
case: since otherwise intentId would be put in its place instead.

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.cc (right):

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.cc:94: return NULL;
On 2012/08/14 06:57:07, benwells wrote:
> Blank line after early return.

Done.

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
File chrome/browser/extensions/web_intent_callbacks.h (right):

https://chromiumcodereview.appspot.com/10828172/diff/4009/chrome/browser/exte...
chrome/browser/extensions/web_intent_callbacks.h:68:
content::WebIntentsDispatcher* InternalRetrieveCallback(std::string key);
On 2012/08/14 06:57:07, benwells wrote:
> There should be a comment here to say you're also removing the callback from
the
> map (or update the name to say it).

Done.

[thorogood, 2012-08-28 04:20:36]

Hi everyone,

I've added a basic test for this feature. It's hardly exhaustive, but it's neat
and shows that this does work.

It short-circuits to within our C++ code to launch and deal with the response; a
more complete integration test might load a proper source webpage and invoke the
intent from it.

PTAL - Thanks!

[benwells, 2012-08-29 12:19:21]

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.cc:9: #include
"base/utf_string_conversions.h"
Is this needed?

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:67:
content::WebIntentsDispatcher* intents_dispatcher() {
nit: indenting

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:76: waiting_ = true;
I am not sure if the threading model in tests is the same as in normal runtime;
if it is there is a race here.

E.g. Thread A calls WaitUntilReply, and executes past the if (replied_) check
which is false. Then we switch to thread B which calls OnReply. It sets replied
and sees that we're not waiting so doesn't call quit. Then thread A starts again
and waits forever.

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:115: // Tests that
platform apps can reply to "launch" events that contain a Web
It's great that this is tested! It would be even better if we tested:
- postFailure
- postSuccess
- sending some data in the reply.

I'd be OK with that in a follow up patch.

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:239:
PlatformAppBlobIntentLauncher(Profile* profile,
Nit: Profile* profile, should be on the next line.

http://codereview.chromium.org/10828172/diff/15001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

http://codereview.chromium.org/10828172/diff/15001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:24: //
TODO(thorogood): What if data is not of a serializable type?
We should answer this question before landing this change.

[thorogood, 2012-08-30 02:37:42]

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/api/app/app_api.cc:9: #include
"base/utf_string_conversions.h"
On 2012/08/29 12:19:21, benwells wrote:
> Is this needed?

Nope, left over. Removed - thanks!

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:67:
content::WebIntentsDispatcher* intents_dispatcher() {
On 2012/08/29 12:19:21, benwells wrote:
> nit: indenting

Done.

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:76: waiting_ = true;
On 2012/08/29 12:19:21, benwells wrote:
> I am not sure if the threading model in tests is the same as in normal
runtime;
> if it is there is a race here.
> 
> E.g. Thread A calls WaitUntilReply, and executes past the if (replied_) check
> which is false. Then we switch to thread B which calls OnReply. It sets
replied
> and sees that we're not waiting so doesn't call quit. Then thread A starts
again
> and waits forever.

Naively, I tend to actually agree with you -- I've however duplicated this idiom
from some other test, and in fact it seems like a lot of tests do basically the
same thing.

jeremya@ has had a look at the code with me, and he believes that this isn't an
issue. The callback that I'm waiting for is a SyncExtensionFunction, which only
runs on the UI thread, which means that it can only run in this test during a
call to content::RunMessageLoop().

The if (replied_) bit is thus basically just to make calls here idempotent: it
doesn't imply that we're trying to deal with thread race conditions.

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:115: // Tests that
platform apps can reply to "launch" events that contain a Web
On 2012/08/29 12:19:21, benwells wrote:
> It's great that this is tested! It would be even better if we tested:
> - postFailure
> - postSuccess
> - sending some data in the reply.
> 
> I'd be OK with that in a follow up patch.

Thanks! Yep, I'll happily follow this up.

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:239:
PlatformAppBlobIntentLauncher(Profile* profile,
On 2012/08/29 12:19:21, benwells wrote:
> Nit: Profile* profile, should be on the next line.

Done.

[benwells, 2012-08-30 04:29:07]

lgtm

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

http://codereview.chromium.org/10828172/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:76: waiting_ = true;
On 2012/08/30 02:37:42, thorogood wrote:
> On 2012/08/29 12:19:21, benwells wrote:
> > I am not sure if the threading model in tests is the same as in normal
> runtime;
> > if it is there is a race here.
> > 
> > E.g. Thread A calls WaitUntilReply, and executes past the if (replied_)
check
> > which is false. Then we switch to thread B which calls OnReply. It sets
> replied
> > and sees that we're not waiting so doesn't call quit. Then thread A starts
> again
> > and waits forever.
> 
> Naively, I tend to actually agree with you -- I've however duplicated this
idiom
> from some other test, and in fact it seems like a lot of tests do basically
the
> same thing.
> 
> jeremya@ has had a look at the code with me, and he believes that this isn't
an
> issue. The callback that I'm waiting for is a SyncExtensionFunction, which
only
> runs on the UI thread, which means that it can only run in this test during a
> call to content::RunMessageLoop().
> 
> The if (replied_) bit is thus basically just to make calls here idempotent: it
> doesn't imply that we're trying to deal with thread race conditions.

Ah ok, if both functions always run in the same thread it isn't a problem.

[thorogood, 2012-08-30 04:48:27]

Can I get some more concrete reviews for my change? :)

mihaip: chrome/{browser,common,renderer}/extensions
sky: chrome (gypi change)
gbillock: chrome/browser/ui

Thanks!

[sky, 2012-08-30 15:40:12]

LGTM

[Mihai Parparita -not on Chrome, 2012-08-30 20:43:43]

LGTM

[thorogood, 2012-09-01 02:33:02]

I'm going to CQ this as the change to this file:
chrome/browser/ui/intents/web_intent_picker_controller.cc

is absolutely tiny and is directly just to hook up the feature.

[commit-bot: I haz the power, 2012-09-01 02:33:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10828172/34001

[commit-bot: I haz the power, 2012-09-01 02:33:43]

Failed to apply patch for chrome/browser/extensions/platform_app_browsertest.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/platform_app_browsertest.cc
Hunk #1 FAILED at 2.
Hunk #2 succeeded at 18 (offset 2 lines).
Hunk #3 succeeded at 52 with fuzz 2 (offset 5 lines).
Hunk #4 succeeded at 114 (offset 5 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/extensions/platform_app_browsertest.cc.rej

[commit-bot: I haz the power, 2012-09-03 00:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10828172/25004

[commit-bot: I haz the power, 2012-09-03 00:56:19]

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

[thorogood, 2012-09-03 03:37:40]

http://codereview.chromium.org/10828172/diff/15001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

http://codereview.chromium.org/10828172/diff/15001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:24: //
TODO(thorogood): What if data is not of a serializable type?
On 2012/08/29 12:19:21, benwells wrote:
> We should answer this question before landing this change.

I have answered it, as discussed in person! Non-serializable content does not
work. Neither does it for hosted apps replying to Web Intents.

[commit-bot: I haz the power, 2012-09-03 03:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10828172/31019

[commit-bot: I haz the power, 2012-09-03 06:00:25]

Change committed as 154658