Create ExtensionActionView class

Create ExtensionActionView class

Create an ExtensionActionView class which represents the core of either a
PageActionView or a BrowserActionView. The goal here is to move all logic for
executing the action itself into ExtensionActionView, so that
PageActionImageView and BrowserActionButton (the current implementation classes)
become nothing more than a view wrapper for the ExtensionActionView.

This should allow us to show page actions in the now-incorrectly-named
BrowserActionsContainer and have them render in the same fashion as the current
browser actions.

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

[Devlin, 2014-08-05 20:08:58]

Hey Scott, Peter took the last one, so this one is going to you. :)  Cheers!

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.cc (left):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.cc:144: void
BrowserActionButton::Destroy() {
Pretty sure we don't need this - when menu_runner_ is deleted, the
MenuRunnerImpl stays around until it closes.  I added a test to make sure it
works, and it seems fine.

[sky, 2014-08-05 22:29:47]

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_action_view.h (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:42: // All common
logic for executing extension actions should go in this class;
No one is going to subclass this, right?

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:45: class
ExtensionActionView : public ExtensionActionIconFactory::Observer,
I don't like naming this class a View if its not actually a View. I realize we
are not consistent in Chrome with usage of view. But this file lives in the the
views directory, so I think we should at least be consistent in views.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:50: class
ViewDelegate {
I prefer this to live in its own file and be named ExtensionActionViewDelegate.
Style guide says: "Prefer putting delegate classes in their own header files"

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:52: virtual
~ViewDelegate() {}
make protected so that it's clear the delegate is not owned.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:58: // Returns true
if this is a nested view.
What does 'nested' mean here?

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:89: // Called once
the context menu has closed.
Make it clear this may not be called (if the context menu is showing and 'this'
is deleted).

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:31:
extension_action_view_->RegisterCommand();
At the time this is invoked does GetFocusManagerForAccelerator() return a
non-null focusmanager. In particular I'm worried that during setup if owner_
isn't in a widget you'll get NULL and won't register accelerators.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.h (left):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.h:71: // Overridden
from ui::AcceleratorTarget:
Should ExtensionActionView by the AcceleratorTarget?

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.h (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.h:92: BrowserActionButton*
button_;
Wrap in scoped_ptr now? If delete needs to be run from the destructor add a
comment in the destructor.

[Devlin, 2014-08-05 23:51:10]

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_action_view.h (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:42: // All common
logic for executing extension actions should go in this class;
On 2014/08/05 22:29:46, sky wrote:
> No one is going to subclass this, right?

Correct. :)

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:45: class
ExtensionActionView : public ExtensionActionIconFactory::Observer,
On 2014/08/05 22:29:46, sky wrote:
> I don't like naming this class a View if its not actually a View. I realize we
> are not consistent in Chrome with usage of view. But this file lives in the
the
> views directory, so I think we should at least be consistent in views.

I know :/  This was a desperate attempt at a name that makes sense.  Didn't
wanna do just ExtensionActionController, since this class is still heavily
views-specific, but (as discussed offline), ExtensionActionViewController should
work.  (ExtensionActionViewPopupController was also mentioned offline, but this
class does slightly more than just popups, like notifying the extension when the
action is activated).

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:50: class
ViewDelegate {
On 2014/08/05 22:29:46, sky wrote:
> I prefer this to live in its own file and be named
ExtensionActionViewDelegate.
> Style guide says: "Prefer putting delegate classes in their own header files"

Ah, didn't know that - I think I've only seen delegates in separate files a very
small number of times.  Thanks for the tip!

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:52: virtual
~ViewDelegate() {}
On 2014/08/05 22:29:46, sky wrote:
> make protected so that it's clear the delegate is not owned.

Done.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:58: // Returns true
if this is a nested view.
On 2014/08/05 22:29:47, sky wrote:
> What does 'nested' mean here?

Rephrased for clarity and correctness.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_action_view.h:89: // Called once
the context menu has closed.
On 2014/08/05 22:29:46, sky wrote:
> Make it clear this may not be called (if the context menu is showing and
'this'
> is deleted).

Done.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:31:
extension_action_view_->RegisterCommand();
On 2014/08/05 22:29:47, sky wrote:
> At the time this is invoked does GetFocusManagerForAccelerator() return a
> non-null focusmanager. In particular I'm worried that during setup if owner_
> isn't in a widget you'll get NULL and won't register accelerators.

This shouldn't be any different than the existing code (I'm hoping that very
little of this patch is functionally different).  In the current code, we
register directly on line 67, without giving any more time for setup, so this
_should_ be fine.  That's assuming, of course, the current code is correct. 
Would it be better to delay registration like we do in BrowserActionButton?

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.h (left):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.h:71: // Overridden
from ui::AcceleratorTarget:
On 2014/08/05 22:29:47, sky wrote:
> Should ExtensionActionView by the AcceleratorTarget?

I thought about it, but for some reason thought it wouldn't work since the view
class was the AcceleratorTarget.  Not sure why I thought that was a block...
Done.

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/browser_action_view.h (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/browser_action_view.h:92: BrowserActionButton*
button_;
On 2014/08/05 22:29:47, sky wrote:
> Wrap in scoped_ptr now? If delete needs to be run from the destructor add a
> comment in the destructor.

Done.

[sky, 2014-08-06 00:28:25]

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:31:
extension_action_view_->RegisterCommand();
On 2014/08/05 23:51:10, Devlin wrote:
> On 2014/08/05 22:29:47, sky wrote:
> > At the time this is invoked does GetFocusManagerForAccelerator() return a
> > non-null focusmanager. In particular I'm worried that during setup if owner_
> > isn't in a widget you'll get NULL and won't register accelerators.
> 
> This shouldn't be any different than the existing code (I'm hoping that very
> little of this patch is functionally different).  In the current code, we
> register directly on line 67, without giving any more time for setup, so this
> _should_ be fine.  That's assuming, of course, the current code is correct. 
> Would it be better to delay registration like we do in BrowserActionButton?

No need to delay if everything is good. How about a DCHECK that 'this' is in a
Widget?

[Devlin, 2014-08-06 16:19:03]

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/431173002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:31:
extension_action_view_->RegisterCommand();
On 2014/08/06 00:28:25, sky wrote:
> On 2014/08/05 23:51:10, Devlin wrote:
> > On 2014/08/05 22:29:47, sky wrote:
> > > At the time this is invoked does GetFocusManagerForAccelerator() return a
> > > non-null focusmanager. In particular I'm worried that during setup if
owner_
> > > isn't in a widget you'll get NULL and won't register accelerators.
> > 
> > This shouldn't be any different than the existing code (I'm hoping that very
> > little of this patch is functionally different).  In the current code, we
> > register directly on line 67, without giving any more time for setup, so
this
> > _should_ be fine.  That's assuming, of course, the current code is correct. 
> > Would it be better to delay registration like we do in BrowserActionButton?
> 
> No need to delay if everything is good. How about a DCHECK that 'this' is in a
> Widget?

Done, via DCHECK(GetFocusManagerForAccelerator()).

[sky, 2014-08-06 18:01:59]

LGTM

[Devlin, 2014-08-06 19:53:00]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-08-06 19:54:10]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/431173002/...

[commit-bot: I haz the power, 2014-08-06 20:43:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38143)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43350)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-06 20:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 20:46:23]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Devlin, 2014-08-06 21:18:51]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-08-06 21:19:50]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/431173002/...

[commit-bot: I haz the power, 2014-08-07 03:21:30]

Change committed as 287951