extensions: Add ExtensionView interface.

extensions: Add ExtensionView interface.

We abstract all ports (android, cocoa, gtk and views) behind this interface.
That way chrome/browser/extensions/ does not need to include any platform-specific ui header files.

BUG=125846
R=ben@chromium.org,aa@chromium.org,rsesek@chromium.org,erg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158199

[tfarina, 2012-09-13 03:23:07]

Hi Ben, Aaron, this is not ready yet for review. It barely compiles yet, but you
can get a taste of what it will look like.

I didn't even start the cocoa/gtk/android ports yet :/

Feel free to take a look a send me comments.

Thanks,

http://codereview.chromium.org/10913243/diff/2001/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/extension_infobar.cc (right):

http://codereview.chromium.org/10913243/diff/2001/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/extension_infobar.cc:125:
AddChildView(extension_host->extension_view());
we can't do this way, because extension_view/ExtensionView will become a
cross-platform interface.

[Ben Goodger (Google), 2012-09-13 15:36:53]

Nice refactor!

I like the direction.

[tfarina, 2012-09-14 01:56:29]

http://codereview.chromium.org/10913243/diff/12043/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_view_views.h (right):

http://codereview.chromium.org/10913243/diff/12043/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_view_views.h:25: public
views::NativeViewHost {
Looks like we will need to have a ExtensionViewHost. Something similar to what
we have for BalloonViewHost.

[tfarina, 2012-09-14 13:36:59]

http://codereview.chromium.org/10913243/diff/11015/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10913243/diff/11015/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:106:
host->GetExtensionView()->set_background(
Aaron, Ben, any idea how I will get the ExtensionViewViews here?

[Aaron Boodman, 2012-09-16 01:07:38]

sgtm

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

http://codereview.chromium.org/10913243/diff/11015/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:54: void
SetExtensionView(ExtensionView* view);
Nit: These should be unix_hacker style, like the old ones were.

http://codereview.chromium.org/10913243/diff/11015/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10913243/diff/11015/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:106:
host->GetExtensionView()->set_background(
On 2012/09/14 13:36:59, tfarina wrote:
> Aaron, Ben, any idea how I will get the ExtensionViewViews here?

You can static_cast, I assume. You know that it will be ExtensionViewViews
because this is views-specific code.

[tfarina, 2012-09-16 10:54:02]

http://codereview.chromium.org/10913243/diff/14026/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/10913243/diff/14026/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_popup.cc:80:
static_cast<ExtensionViewViews*>(host->GetExtensionView());
Aaron, Ben, the problem of doing this is that ExtensionHost owns the view, but
then in the following line we add it to the views hierarchy, which means it'll
be deleted when this popup is deleted. Looks like this is going to be deleted
twice. May be I should just call set_owned_by_client() here?

[tfarina, 2012-09-16 16:20:42]

Guys this is almost ready for review.

Ben -> chrome/browser/ui/views/ and overall high-level review.
Aaron -> chrome/browser/extensions/ and everything else.
Robert -> chrome/browser/ui/cocoa/
Elliot -> chrome/browser/ui/gtk/

[tfarina, 2012-09-16 22:12:33]

Bots are green on all ports.

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

http://codereview.chromium.org/10913243/diff/11015/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:54: void
SetExtensionView(ExtensionView* view);
On 2012/09/16 01:07:38, Aaron Boodman wrote:
> Nit: These should be unix_hacker style, like the old ones were.
That way I can forward declare ExtensionView instead of including its header
file unnecessarily.

[tfarina, 2012-09-17 01:47:50]

CL description updated!

[Ben Goodger (Google), 2012-09-17 16:07:16]

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_view.h (right):

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/extensions/e...
chrome/browser/extensions/extension_view.h:25: class ExtensionView {
It'd be good to document this interface, including ownership.

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_view_views.h (right):

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_view_views.h:24: class
ExtensionViewViews : public ExtensionView,
Because ExtensionViews are owned by the ExtensionHost, I think you'll need to
set owned by client on this one to prevent a double-free.

[tfarina, 2012-09-19 01:21:39]

Ben, please, take another look.

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_view.h (right):

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/extensions/e...
chrome/browser/extensions/extension_view.h:25: class ExtensionView {
On 2012/09/17 16:07:17, Ben Goodger (Google) wrote:
> It'd be good to document this interface, including ownership.

Done.

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_view_views.h (right):

http://codereview.chromium.org/10913243/diff/6085/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_view_views.h:24: class
ExtensionViewViews : public ExtensionView,
On 2012/09/17 16:07:17, Ben Goodger (Google) wrote:
> Because ExtensionViews are owned by the ExtensionHost, I think you'll need to
> set owned by client on this one to prevent a double-free.

Done.

[tfarina, 2012-09-20 02:04:28]

ping?

[memyy, 2012-09-20 03:07:27]

good'.   memyi04@ovi.com

[memyy, 2012-09-20 07:44:43]

2012/9/20, metanata4@gmail.com <metanata4@gmail.com>:
> good'.   memyi04@ovi.com
>
> https://chromiumcodereview.appspot.com/10913243/
>

[Ben Goodger (Google), 2012-09-21 16:04:32]

lgtm

[Robert Sesek, 2012-09-21 18:06:51]

cocoa/ LGTM

https://codereview.chromium.org/10913243/diff/18001/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.h (right):

https://codereview.chromium.org/10913243/diff/18001/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/extension_view_mac.h:67:
ExtensionViewContainer* container_;  // Not owned.
"Not owned." -> "weak" is the convention (c.f. lines 55, 57)

[tfarina, 2012-09-21 20:42:47]

https://codereview.chromium.org/10913243/diff/18001/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.h (right):

https://codereview.chromium.org/10913243/diff/18001/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/extension_view_mac.h:67:
ExtensionViewContainer* container_;  // Not owned.
On 2012/09/21 18:06:51, rsesek wrote:
> "Not owned." -> "weak" is the convention (c.f. lines 55, 57)

Done.