[Web Intents] Add ability to dispatch intents from extension-caused code (i.e. context menus)

[Web Intents] Add ability to dispatch intents from extension-caused code (i.e. context menus)

R=ben@chromium.org,groby@chromium.org
BUG=109425, 128964
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150876

[groby-ooo-7-16, 2012-07-31 19:26:17]

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:422: void
ExtensionHost::WebIntentDispatch(
This seems to have odd ownership rules for intents_dispatcher. Always takes
ownership? (In which case, should it be a scoped_ptr?)

[Greg Billock, 2012-08-01 17:02:26]

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:422: void
ExtensionHost::WebIntentDispatch(
On 2012/07/31 19:26:17, groby wrote:
> This seems to have odd ownership rules for intents_dispatcher. Always takes
> ownership? (In which case, should it be a scoped_ptr?)

Yes, callee takes ownership. This is documented in the WebIntentsDispatcher and
WebContentsDelegate API.

[groby-ooo-7-16, 2012-08-01 18:17:06]

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:422: void
ExtensionHost::WebIntentDispatch(
In this case, please use scoped_ptr. (And .release() in the call to
WebIntentDispatch)
On 2012/08/01 17:02:26, Greg Billock wrote:
> On 2012/07/31 19:26:17, groby wrote:
> > This seems to have odd ownership rules for intents_dispatcher. Always takes
> > ownership? (In which case, should it be a scoped_ptr?)
> 
> Yes, callee takes ownership. This is documented in the WebIntentsDispatcher
and
> WebContentsDelegate API.

[Greg Billock, 2012-08-07 00:23:57]

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:422: void
ExtensionHost::WebIntentDispatch(
On 2012/08/01 18:17:06, groby wrote:
> In this case, please use scoped_ptr. (And .release() in the call to
> WebIntentDispatch)
> On 2012/08/01 17:02:26, Greg Billock wrote:
> > On 2012/07/31 19:26:17, groby wrote:
> > > This seems to have odd ownership rules for intents_dispatcher. Always
takes
> > > ownership? (In which case, should it be a scoped_ptr?)
> > 
> > Yes, callee takes ownership. This is documented in the WebIntentsDispatcher
> and
> > WebContentsDelegate API.
> 

Done.

[Greg Billock, 2012-08-07 20:16:39]

On 2012/08/07 00:23:57, Greg Billock wrote:
>
http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
> File chrome/browser/extensions/extension_host.cc (right):
> 
>
http://codereview.chromium.org/10830102/diff/1/chrome/browser/extensions/exte...
> chrome/browser/extensions/extension_host.cc:422: void
> ExtensionHost::WebIntentDispatch(
> On 2012/08/01 18:17:06, groby wrote:
> > In this case, please use scoped_ptr. (And .release() in the call to
> > WebIntentDispatch)
> > On 2012/08/01 17:02:26, Greg Billock wrote:
> > > On 2012/07/31 19:26:17, groby wrote:
> > > > This seems to have odd ownership rules for intents_dispatcher. Always
> takes
> > > > ownership? (In which case, should it be a scoped_ptr?)
> > > 
> > > Yes, callee takes ownership. This is documented in the
WebIntentsDispatcher
> > and
> > > WebContentsDelegate API.
> > 
> 
> Done.

ping

[groby-ooo-7-16, 2012-08-07 20:32:24]

lgtm

[Greg Billock, 2012-08-07 20:33:52]

On 2012/08/07 20:32:24, groby wrote:
> lgtm

Thanks. Ben, can you take a look? This is the addition to the BrowserList friend
list we discussed.

[Ben Goodger (Google), 2012-08-07 21:18:16]

http://codereview.chromium.org/10830102/diff/9/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10830102/diff/9/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:428: :
browser::FindBrowserWithWebContents(web_contents);
Browser* browser = view() ? view()->browser() :
    browser::FindBrowserWith..(..);

http://codereview.chromium.org/10830102/diff/9/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:439: return;
return seems unnecessary

http://codereview.chromium.org/10830102/diff/9/chrome/browser/intents/web_int...
File chrome/browser/intents/web_intents_util.h (right):

http://codereview.chromium.org/10830102/diff/9/chrome/browser/intents/web_int...
chrome/browser/intents/web_intents_util.h:27: Browser*
GetWebIntentDeliveryBrowser(Profile* profile);
Can you name this method something that indicates its only for use by background
contents, not a general purpose mechanism? e.g.
GetWebIntentDeliveryBrowserForBackgroundContents() or something like that.

[Greg Billock, 2012-08-07 22:48:45]

http://codereview.chromium.org/10830102/diff/9/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10830102/diff/9/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:428: :
browser::FindBrowserWithWebContents(web_contents);
On 2012/08/07 21:18:16, Ben Goodger (Google) wrote:
> Browser* browser = view() ? view()->browser() :
>     browser::FindBrowserWith..(..);

Done.

http://codereview.chromium.org/10830102/diff/9/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:439: return;
On 2012/08/07 21:18:16, Ben Goodger (Google) wrote:
> return seems unnecessary

Done. Forgot to kill this after switching to scoped_ptr.

http://codereview.chromium.org/10830102/diff/9/chrome/browser/intents/web_int...
File chrome/browser/intents/web_intents_util.h (right):

http://codereview.chromium.org/10830102/diff/9/chrome/browser/intents/web_int...
chrome/browser/intents/web_intents_util.h:27: Browser*
GetWebIntentDeliveryBrowser(Profile* profile);
Done.

On 2012/08/07 21:18:16, Ben Goodger (Google) wrote:
> Can you name this method something that indicates its only for use by
background
> contents, not a general purpose mechanism? e.g.
> GetWebIntentDeliveryBrowserForBackgroundContents() or something like that.

[Ben Goodger (Google), 2012-08-07 23:33:04]

lgtm

[commit-bot: I haz the power, 2012-08-08 22:37:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10830102/1009

[commit-bot: I haz the power, 2012-08-08 23:22:43]

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

[commit-bot: I haz the power, 2012-08-09 01:18:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10830102/11003

[commit-bot: I haz the power, 2012-08-09 01:59:03]

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

[commit-bot: I haz the power, 2012-08-09 17:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10830102/10005

[commit-bot: I haz the power, 2012-08-09 20:25:46]

Change committed as 150876