Abstract base 'ExtensionView' to Fix DEPS violation in extension_view_host.h

Abstract base 'ExtensionView' to Fix DEPS violation in extension_view_host.h

c/b/e/extension_view_host.h currently includes platform-specific UI code
and has a "temporary" exception in extensions/DEPS.

This CL removes the exception and resolves the layering by having the
platform-specific UI code provide an implementation of (static)
ExtensionViewHost::CreateExtensionView(). An abstract base,
ExtensionView, is added for use in the platform agnostic code.

This removes a lot of #ifdefs from extension_view_host.h which were in
the way while trying to get toolkit-views building on Mac for the
`chrome` target.

BUG=125846, 390755
TEST=Refactoring. Should be no functional changes for extension views
(e.g. Google Cast toolbar popup, Lookup Companion for Wikipedia) -
appearance and keyboard interaction should be unchanged.

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

[tapted, 2014-07-03 12:36:45]

Hi Finnur and Peter, please take a look.

[tapted, 2014-07-03 12:45:29]

oops - meant to add - suggested split:
 finnur: extensions stuff (all of it really)
 peter: infobars and chrome/browser/ui OWNERS (or anything else you'd like to
look at)

Thanks!

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/extension...
File chrome/browser/extensions/extension_view_host.cc (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/extension...
chrome/browser/extensions/extension_view_host.cc:99: }
note that ExtensionView*Host*Mac didn't use this implementation of
UnhandledKeyboardEvent due to an override (which is now moved into from the Host
into ExtensionViewMac::HandleKeyboardEvent() via the generic base). I think this
is the only subtle change - the rest is fairly mechanical.

[tfarina, 2014-07-03 12:56:37]

yoz, jamescook, fyi.

Trent, you are flying! :)

[tfarina, 2014-07-03 12:58:01]

Just curious, how did you test this? Which extension did you use? I think that
would be good to add to the CL description.

[Finnur, 2014-07-03 14:27:06]

LGTM

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/extension...
File chrome/browser/extensions/extension_view.h (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/extension...
chrome/browser/extensions/extension_view.h:50: // Method for the ExtensionHost
to notify that the extension page is loaded.
nit: is -> has ?

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.h (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_view_mac.h:56: // Overridden from
extensions::ExtensionView:
nit: Prefer just 

  // extensions::ExtensionView:
  virtual voit Init() ... etc

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.mm (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_view_mac.mm:94: }
nit: You don't need braces if the if clause is single line (the style rule does
not apply to multi-line condititionals.

[tapted, 2014-07-04 00:08:04]

On 2014/07/03 12:58:01, tfarina wrote:
> Just curious, how did you test this? Which extension did you use? I think that
> would be good to add to the CL description.

Poked around with the Google Cast extension on Linux and Mac. There's also some
coverage offered by ExtensionViewHostFactoryTest.CreateExtensionHosts (and
`DownloadExtensionTest.DownloadExtensionTest_AcceptDanger` it turns out)

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/extension...
File chrome/browser/extensions/extension_view.h (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/extension...
chrome/browser/extensions/extension_view.h:50: // Method for the ExtensionHost
to notify that the extension page is loaded.
On 2014/07/03 14:27:05, Finnur wrote:
> nit: is -> has ?

Done.

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.h (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_view_mac.h:56: // Overridden from
extensions::ExtensionView:
On 2014/07/03 14:27:05, Finnur wrote:
> nit: Prefer just 
> 
>   // extensions::ExtensionView:
>   virtual voit Init() ... etc

Done.

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.mm (right):

https://codereview.chromium.org/363233002/diff/50022/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_view_mac.mm:94: }
On 2014/07/03 14:27:05, Finnur wrote:
> nit: You don't need braces if the if clause is single line (the style rule
does
> not apply to multi-line condititionals.

Done.

[James Cook, 2014-07-07 16:41:58]

Hooray! Thanks for doing this.

[Peter Kasting, 2014-07-07 22:42:23]

I can't review ObjC code, but the rest of c/b/ui LGTM

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_dialog.cc:108: ExtensionViewViews*
ExtensionDialog::GetExtensionView() const {
Nit: Something this simple should be defined inline in the header and named
unix_hacker()-style.

Optionally, you could make it take an extensions::ExtensionViewHost* arg.  If
you did this, you could (a) define it in this file in an anonymous namespace and
(b) use it in Show() above, as well.

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:118: return
static_cast<ExtensionViewViews*>(host()->view());
Nit: Same comments

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/extension_infobar.cc (right):

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/extension_infobar.cc:233: const
ExtensionViewViews* ExtensionInfoBar::GetExtensionView() const {
Nit: Looks like you only use this function one place, I might just inline its
contents there.

[tapted, 2014-07-08 07:11:51]

+rsesek for OWNERS coverage for the Objective C stuff. There are 4 files under
c/b/ui/cocoa/ please take a look.

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_dialog.cc:108: ExtensionViewViews*
ExtensionDialog::GetExtensionView() const {
On 2014/07/07 22:42:23, Peter Kasting wrote:
> Nit: Something this simple should be defined inline in the header and named
> unix_hacker()-style.
> 
> Optionally, you could make it take an extensions::ExtensionViewHost* arg.  If
> you did this, you could (a) define it in this file in an anonymous namespace
and
> (b) use it in Show() above, as well.

Done - went with the anonymous namespace. This avoids extension_view_host.h
leaking into extension_dialog.h (in addition to the forward-dec of
ExtensionViewViews which can also be removed from the headers).

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:118: return
static_cast<ExtensionViewViews*>(host()->view());
On 2014/07/07 22:42:23, Peter Kasting wrote:
> Nit: Same comments

Done.

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/extension_infobar.cc (right):

https://codereview.chromium.org/363233002/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/extension_infobar.cc:233: const
ExtensionViewViews* ExtensionInfoBar::GetExtensionView() const {
On 2014/07/07 22:42:23, Peter Kasting wrote:
> Nit: Looks like you only use this function one place, I might just inline its
> contents there.

Done.

[Robert Sesek, 2014-07-08 12:50:37]

cocoa/ LGTM

https://codereview.chromium.org/363233002/diff/230001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.mm (right):

https://codereview.chromium.org/363233002/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_view_mac.mm:35:
content::RenderViewHost* ExtensionViewMac::render_view_host() const {
nit: implementation order should match header order. Here and throughout this
file.

https://codereview.chromium.org/363233002/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_view_mac.mm:96:
static_cast<ChromeEventProcessingWindow*>([GetNativeView() window]);
Can use ObjCCastStrict here.

[tapted, 2014-07-08 23:54:58]

https://codereview.chromium.org/363233002/diff/230001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_view_mac.mm (right):

https://codereview.chromium.org/363233002/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_view_mac.mm:35:
content::RenderViewHost* ExtensionViewMac::render_view_host() const {
On 2014/07/08 12:50:37, rsesek wrote:
> nit: implementation order should match header order. Here and throughout this
> file.

Done.

https://codereview.chromium.org/363233002/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_view_mac.mm:96:
static_cast<ChromeEventProcessingWindow*>([GetNativeView() window]);
On 2014/07/08 12:50:37, rsesek wrote:
> Can use ObjCCastStrict here.

Done.

[tapted, 2014-07-09 00:21:11]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-07-09 00:25:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/363233002/270001

[commit-bot: I haz the power, 2014-07-09 05:48:48]

Change committed as 281962