Implement Action Box button

Adds the 'action box' button to the location bar, which will soon be connected to a menu with lots of 'actions'.

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

[yefimt, 2012-05-16 21:28:30]

Just a button for now, no menu yet and images are not final

[Aaron Boodman, 2012-05-17 20:42:05]

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right):

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:21:
command_updater_(command_updater) {
The command_updater_ is not used. Better to leave it out until it is.

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:32: void
ActionBoxButtonView::SetImages() {
In the case of AppMenuButtonView, the container does this, which seems sensible
to me. You might be able to get rid of this class if you move all the
configuration into the container.

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:35: if (icon)
There's no case in which these icons won't be present, so remove the if
statement.

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/action_box_button_view.h:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/action_box_button_view.h:19: class
ActionBoxButtonView : public views::MenuButton,
In the case of the wrench menu, the containing view plays the role of
MenuButtonListener. In your case, this would be LocationBarView.

I'm not sure if there's a reason why the containing view should be the listener,
but it seems more right somehow, and it would be good to be consistent if
nothing else.

http://codereview.chromium.org/10406016/diff/1009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/action_box_button_view.h:28: virtual void
GetAccessibleState(ui::AccessibleViewState* state) OVERRIDE;
When implementing interfaces, it is customary to format like so:

// MenuButton
virtual void GetAccessibleState(...) OVERRIDE;

// MenuButtonListener
virtual void OnMenuButtonClicked(...) OVERRIDE;

[Aaron Boodman, 2012-05-17 20:48:04]

Nevermind my comments about moving code into location bar. Per our real-life
discussion, I'm fine trying this approach.

You'll still need approval from someone in this directory though...

[Aaron Boodman, 2012-05-17 20:51:02]

+sky for ui reviews

[sky, 2012-05-17 21:21:07]

Dare I ask what the action box is going to be?

ALso, please link to a BUG= in your description.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:260:
action_box_button_view_->SetVisible(true);
visible is the default.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:661: if
(action_box_button_view_ && action_box_button_view_->visible()) {
You always make it visible, so no need for the visible check.

[yefimt, 2012-05-17 22:42:52]

Action box is a plus button and associated menu in a right hand end of the
location bar. See
https://sites.google.com/a/google.com/extensions-vnext/features/extension-ui-...

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right):

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:21:
command_updater_(command_updater) {
On 2012/05/17 20:42:05, Aaron Boodman wrote:
> The command_updater_ is not used. Better to leave it out until it is.

Done.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:35: if (icon)
On 2012/05/17 20:42:05, Aaron Boodman wrote:
> There's no case in which these icons won't be present, so remove the if
> statement.

Done.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2012/05/17 20:42:05, Aaron Boodman wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:28: virtual void
GetAccessibleState(ui::AccessibleViewState* state) OVERRIDE;
On 2012/05/17 20:42:05, Aaron Boodman wrote:
> When implementing interfaces, it is customary to format like so:
> 
> // MenuButton
> virtual void GetAccessibleState(...) OVERRIDE;
> 
> // MenuButtonListener
> virtual void OnMenuButtonClicked(...) OVERRIDE;

Done.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:260:
action_box_button_view_->SetVisible(true);
On 2012/05/17 21:21:07, sky wrote:
> visible is the default.

Done.

https://chromiumcodereview.appspot.com/10406016/diff/1009/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:661: if
(action_box_button_view_ && action_box_button_view_->visible()) {
On 2012/05/17 21:21:07, sky wrote:
> You always make it visible, so no need for the visible check.

Done.

[Aaron Boodman, 2012-05-17 22:46:23]

lgtm

[sky, 2012-05-17 23:25:50]

LGTM

[not at google - send to devlin, 2012-05-17 23:40:24]

thanks for CC'ing me, drive-by nits (but lgtm for what it's worth)

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:20: class
ActionBoxButtonView : public views::MenuButton,
comment with what the action box is, perhaps?

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:34: const
gfx::Point& point) OVERRIDE;
Isn't the usual practice to have inherited methods as public?

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:39:
DISALLOW_EVIL_CONSTRUCTORS(ActionBoxButtonView);
wot?

Oh, this should be DISALLOW_COPY_AND_ASSIGN, the basictypes.h header says to use
that one instead.

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:259:
AddChildView(action_box_button_view_);
nit: could you combine this with the above statement to minimise the number of
checks to action_box_enabled?  like

if (extensions::switch_utils::IsActionBoxEnabled() {
  ...
} else if (browser_defaults::bookmarks_enabled && (mode_ == NORMAL) {
  ...
}

?
then you don't need the local variable either.

[Aaron Boodman, 2012-05-17 23:48:35]

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:34: const
gfx::Point& point) OVERRIDE;
On 2012/05/17 23:40:24, kalman wrote:
> Isn't the usual practice to have inherited methods as public?

No. In fact I prefer them private if they aren't going to be called.

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:39:
DISALLOW_EVIL_CONSTRUCTORS(ActionBoxButtonView);
On 2012/05/17 23:40:24, kalman wrote:
> wot?
> 
> Oh, this should be DISALLOW_COPY_AND_ASSIGN, the basictypes.h header says to
use
> that one instead.

Whoops, yeah. DISALLOW_EVIL_CONSTRUCTORS was the name used internally at Google,
which is probably why Yefim was familiar with it.

[not at google - send to devlin, 2012-05-17 23:52:03]

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:34: const
gfx::Point& point) OVERRIDE;
On 2012/05/17 23:48:35, Aaron Boodman wrote:
> On 2012/05/17 23:40:24, kalman wrote:
> > Isn't the usual practice to have inherited methods as public?
> 
> No. In fact I prefer them private if they aren't going to be called.

They're not going to be called? Am I missing something, or, why would methods
not be called?

[Aaron Boodman, 2012-05-17 23:56:27]

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:34: const
gfx::Point& point) OVERRIDE;
On 2012/05/17 23:52:03, kalman wrote:
> On 2012/05/17 23:48:35, Aaron Boodman wrote:
> > On 2012/05/17 23:40:24, kalman wrote:
> > > Isn't the usual practice to have inherited methods as public?
> > 
> > No. In fact I prefer them private if they aren't going to be called.
> 
> They're not going to be called? Am I missing something, or, why would methods
> not be called?

In C++, you can override a virtual method 'privately'. It can be called through
the base class, but not the overriding class.

If the only way a virtual method is going to be called is through a base class,
I prefer to make it private.

The more things are private, the less readers have to worry about who could
potentially be calling something. It reduces the overall cognitive overhead of
understanding the code.

[yefimt, 2012-05-18 00:20:01]

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:20: class
ActionBoxButtonView : public views::MenuButton,
On 2012/05/17 23:40:24, kalman wrote:
> comment with what the action box is, perhaps?

Done.

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:39:
DISALLOW_EVIL_CONSTRUCTORS(ActionBoxButtonView);
On 2012/05/17 23:40:24, kalman wrote:
> wot?
> 
> Oh, this should be DISALLOW_COPY_AND_ASSIGN, the basictypes.h header says to
use
> that one instead.

Done.

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10406016/diff/11001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:259:
AddChildView(action_box_button_view_);
On 2012/05/17 23:40:24, kalman wrote:
> nit: could you combine this with the above statement to minimise the number of
> checks to action_box_enabled?  like
> 
> if (extensions::switch_utils::IsActionBoxEnabled() {
>   ...
> } else if (browser_defaults::bookmarks_enabled && (mode_ == NORMAL) {
>   ...
> }
> 
> ?
> then you don't need the local variable either.

Done.

[commit-bot: I haz the power, 2012-05-18 18:59:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10406016/5016

[commit-bot: I haz the power, 2012-05-18 19:18:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10406016/13004

[commit-bot: I haz the power, 2012-05-18 19:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10406016/13005

[commit-bot: I haz the power, 2012-05-18 20:43:34]

Change committed as 137950