Pull browser action click logic into ToolbarModelController, so that when I

Pull browser action click logic into ToolbarModelController, so that the upcoming
changes in adding the activeTab permission can be done without changing any
UI code.


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

[not at google - send to devlin, 2012-05-24 00:37:20]

yoz: extensions changes. I deleted the Observer stuff because it doesn't seem
necessary any more (just adding complexity); if we're putting all that logic in
the same place, there's not much point in allowing them to be Observed.

estade: gtk
sky: views
nico: cocoa

[Yoyo Zhou, 2012-05-24 00:55:25]

LGTM

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:107: return ACTION_NONE;
It seems slightly confusing to say that the action is NONE when you actually
executed the browser action. Maybe Action should be called UIAction or some
such. If there's precedent for Action, though, you can keep it.

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.h (right):

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.h:374: void
ShowPopup(BrowserActionButton* button, const GURL& popup_url);
Does this need to be public?

[sky, 2012-05-24 03:58:54]

LGTM

[Nico, 2012-05-24 04:05:41]

cocoa lgtm

[tfarina, 2012-05-24 18:44:55]

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.h:71: int
GetVisibleIconCount() { return visible_icon_count_; }
nit: make this function const.

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.h:136: };
nit: if we don't want copy and assign semantics, probably worth adding
DISALLOW_COPY_AND_ASSIGN here.

[Evan Stade, 2012-05-24 23:12:55]

gtk lgtm

[not at google - send to devlin, 2012-05-25 00:55:40]

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:107: return ACTION_NONE;
On 2012/05/24 00:55:26, Yoyo Zhou wrote:
> It seems slightly confusing to say that the action is NONE when you actually
> executed the browser action. Maybe Action should be called UIAction or some
> such. If there's precedent for Action, though, you can keep it.

Yeah, UIAction would have been more accurate, but there is a precedent kind of
(with the page action stuff I'm also doing, which has already been submitted).

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.h:71: int
GetVisibleIconCount() { return visible_icon_count_; }
On 2012/05/24 18:44:55, tfarina wrote:
> nit: make this function const.

Done.

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.h:136: };
On 2012/05/24 18:44:55, tfarina wrote:
> nit: if we don't want copy and assign semantics, probably worth adding
> DISALLOW_COPY_AND_ASSIGN here.

Done.

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.h (right):

http://codereview.chromium.org/10412052/diff/7012/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.h:374: void
ShowPopup(BrowserActionButton* button, const GURL& popup_url);
On 2012/05/24 00:55:26, Yoyo Zhou wrote:
> Does this need to be public?

Nope. Thanks, made private (I put it here to be alongside its friend HidePopup
which apparently needs to be public for some testing util stuff).

[commit-bot: I haz the power, 2012-05-25 01:13:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10412052/17001

[commit-bot: I haz the power, 2012-05-25 02:37:21]

Change committed as 138977

[Aaron Boodman, 2012-05-25 07:22:43]

Nit: there is no such class as 'ToolbarModelController'

[not at google - send to devlin, 2012-05-25 07:23:32]

Yeah. Brain not functioning. Too late now!