Remove browser from web intents UI code

Remove browser from web intents UI code.

BUG=None
TEST=Tested WebIntents still work

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

[benwells, 2012-04-26 03:23:45]

The extension_host.* changes for this patch are not final (they're waiting on
another patch), but the rest is ready for review.

gbillock@ - for browser/ui/intents and other changes to intents UI code
sky@ - for browser/ui/

[sky, 2012-04-26 15:49:27]

LGTM

[Greg Billock, 2012-04-26 21:32:07]

This looks good to me wrt web intents. Can you note in the description that it
gets rid of Browser in the intents internals? We've been meaning to do that for
a while, so this is a good change!

Switching extension_host to use the wrapper I don't have a good sense for.
Another reviewer? Is that Scott?

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:144:
host_contents_wrapper_.reset(new TabContentsWrapper(host_contents_));
I don't have a good sense of the cost of switching from a WebContents to the
wrapper. An extensions person should review this.

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:228: content::WebContents*
host_contents_;
Note what the ownership situation is for host_contents_. It looks like you're
giving it to the wrapper? We can just get it out with
TabContentsWrapper::web_contents() right? Maybe just switch all internal usage
to that...

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/intents/web...
File chrome/browser/intents/web_intents_util.h (right):

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/intents/web...
chrome/browser/intents/web_intents_util.h:18: // the Profile to check that
WebIntents are enable for.
"enabled"

[benwells, 2012-04-26 23:59:37]

+aa@ for extensions review.

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:144:
host_contents_wrapper_.reset(new TabContentsWrapper(host_contents_));
On 2012/04/26 21:32:07, Greg Billock wrote:
> I don't have a good sense of the cost of switching from a WebContents to the
> wrapper. An extensions person should review this.

Indeed. aa@ added for extensions.

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:228: content::WebContents*
host_contents_;
On 2012/04/26 21:32:07, Greg Billock wrote:
> Note what the ownership situation is for host_contents_. It looks like you're
> giving it to the wrapper? We can just get it out with
> TabContentsWrapper::web_contents() right? Maybe just switch all internal usage
> to that...

This code was not final. A new version of this is now up.

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/intents/web...
File chrome/browser/intents/web_intents_util.h (right):

http://codereview.chromium.org/10134026/diff/16001/chrome/browser/intents/web...
chrome/browser/intents/web_intents_util.h:18: // the Profile to check that
WebIntents are enable for.
On 2012/04/26 21:32:07, Greg Billock wrote:
> "enabled"

Done.

[Greg Billock, 2012-04-27 00:10:29]

On 2012/04/26 23:59:37, benwells wrote:
> +aa@ for extensions review.
> 
>
http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_host.cc (right):
> 
>
http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_host.cc:144:
> host_contents_wrapper_.reset(new TabContentsWrapper(host_contents_));
> On 2012/04/26 21:32:07, Greg Billock wrote:
> > I don't have a good sense of the cost of switching from a WebContents to the
> > wrapper. An extensions person should review this.
> 
> Indeed. aa@ added for extensions.
> 
>
http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_host.h (right):
> 
>
http://codereview.chromium.org/10134026/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_host.h:228: content::WebContents*
> host_contents_;
> On 2012/04/26 21:32:07, Greg Billock wrote:
> > Note what the ownership situation is for host_contents_. It looks like
you're
> > giving it to the wrapper? We can just get it out with
> > TabContentsWrapper::web_contents() right? Maybe just switch all internal
usage
> > to that...
> 
> This code was not final. A new version of this is now up.
> 
>
http://codereview.chromium.org/10134026/diff/16001/chrome/browser/intents/web...
> File chrome/browser/intents/web_intents_util.h (right):
> 
>
http://codereview.chromium.org/10134026/diff/16001/chrome/browser/intents/web...
> chrome/browser/intents/web_intents_util.h:18: // the Profile to check that
> WebIntents are enable for.
> On 2012/04/26 21:32:07, Greg Billock wrote:
> > "enabled"
> 
> Done.

lgtm intents

[benwells, 2012-05-01 21:53:43]

ExtensionHost changes removed. Would like to land the rest of the change now to
avoid future merge headaches.

Greg - wanna take another look? Nothing has changed apart from rebasing and
reverting the changes in extensions/

[Greg Billock, 2012-05-02 15:00:50]

On 2012/05/01 21:53:43, benwells wrote:
> ExtensionHost changes removed. Would like to land the rest of the change now
to
> avoid future merge headaches.
> 
> Greg - wanna take another look? Nothing has changed apart from rebasing and
> reverting the changes in extensions/

lgtm

[commit-bot: I haz the power, 2012-05-02 17:44:53]

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

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

Change committed as 134964