Refactor UI "badge" (page action) logic into a BadgeController interface.

Refactor UI "badge" (page action) logic into a BadgeController interface,
and convert the GTK code to use it (mac/views come next).

This is for the currentTab permission work where the page action area will
be used to display icons for extensions with scripts running on the current
page ("script badges"), and will also need to have various different UIs for
them (hence the "decoration" stuff). To implement this I plan on injecting
a different BadgeController implementation given a command line flag for the
new behaviour.

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

[not at google - send to devlin, 2012-05-11 03:55:25]

.

[not at google - send to devlin, 2012-05-11 04:06:14]

mpcomplete: extensions stuff
estade: GTK stuff
avi: tabcontentswrapper

[Avi (use Gerrit), 2012-05-11 04:08:56]

https://chromiumcodereview.appspot.com/10381105/diff/1009/chrome/browser/ui/t...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

https://chromiumcodereview.appspot.com/10381105/diff/1009/chrome/browser/ui/t...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:16: #include
"chrome/browser/extensions/badge_controller.h"
Do you need this include? You only directly use PageActionController below.

[not at google - send to devlin, 2012-05-11 04:14:47]

dont include badgecontroller.h

[not at google - send to devlin, 2012-05-11 04:15:44]

Cheers.

(avi: did you see my other review too? http://codereview.chromium.org/10383104/
it's the same kind of thing)

https://chromiumcodereview.appspot.com/10381105/diff/1009/chrome/browser/ui/t...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

https://chromiumcodereview.appspot.com/10381105/diff/1009/chrome/browser/ui/t...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:16: #include
"chrome/browser/extensions/badge_controller.h"
On 2012/05/11 04:08:57, Avi wrote:
> Do you need this include? You only directly use PageActionController below.

Nope. Thanks, removed.

[Avi (use Gerrit), 2012-05-11 04:20:25]

TCW LGTM

[Matt Perry, 2012-05-11 21:15:12]

lgtm

https://chromiumcodereview.appspot.com/10381105/diff/7002/chrome/browser/exte...
File chrome/browser/extensions/badge_controller.h (right):

https://chromiumcodereview.appspot.com/10381105/diff/7002/chrome/browser/exte...
chrome/browser/extensions/badge_controller.h:34: enum Reaction {
Just "Action" ?

https://chromiumcodereview.appspot.com/10381105/diff/7002/chrome/browser/exte...
File chrome/browser/extensions/page_action_controller.cc (right):

https://chromiumcodereview.appspot.com/10381105/diff/7002/chrome/browser/exte...
chrome/browser/extensions/page_action_controller.cc:39:
GetExtensionService()->GetExtensionById(extension_id, false);
nit: use extensions()->GetByID(). I think we're phasing out GetExtensionById

[Evan Stade, 2012-05-11 21:52:07]

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
File chrome/browser/extensions/badge_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
chrome/browser/extensions/badge_controller.h:19: // Controller of the "badges"
(aka "page actions") in the UI.
confused about the terminology. I thought "badge" referred to the text that you
can add to a browser action, e.g. the number of unread gmail messages.

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
File chrome/browser/extensions/page_action_controller.cc (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.cc:37: const std::string&
extension_id, int button_type) {
is an integer button_type going to easily work for non-gtk platforms? don't we
have some sort of generic enum or cross-platfrom event type?

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.cc:61: return
extension->ShowConfigureContextMenus() ? SHOW_CONTEXT_MENU : NONE;
doesn't this complain about missing default case?

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
File chrome/browser/extensions/page_action_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.h:32: };
DISALLOW_COPY_AND_ASSIGN

[Matt Perry, 2012-05-11 22:01:17]

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
File chrome/browser/extensions/badge_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
chrome/browser/extensions/badge_controller.h:19: // Controller of the "badges"
(aka "page actions") in the UI.
On 2012/05/11 21:52:08, Evan Stade wrote:
> confused about the terminology. I thought "badge" referred to the text that
you
> can add to a browser action, e.g. the number of unread gmail messages.

Good point. This should be named ExtensionActionController.

[not at google - send to devlin, 2012-05-14 03:58:19]

comments

[not at google - send to devlin, 2012-05-14 03:59:01]

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
File chrome/browser/extensions/badge_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
chrome/browser/extensions/badge_controller.h:19: // Controller of the "badges"
(aka "page actions") in the UI.
On 2012/05/11 22:01:17, Matt Perry wrote:
> On 2012/05/11 21:52:08, Evan Stade wrote:
> > confused about the terminology. I thought "badge" referred to the text that
> you
> > can add to a browser action, e.g. the number of unread gmail messages.
> 
> Good point. This should be named ExtensionActionController.

Cool. "Badge" was an attempt to make an agnostic name wrt what these are about
to become, "script badges", so I think ExtensionActionController would be
likewise misleading.

How about "PageBoxController"? This is what that area is / will be called I
think.

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
chrome/browser/extensions/badge_controller.h:34: enum Reaction {
On 2012/05/11 21:15:12, Matt Perry wrote:
> Just "Action" ?

Done.

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
File chrome/browser/extensions/page_action_controller.cc (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.cc:37: const std::string&
extension_id, int button_type) {
On 2012/05/11 21:52:08, Evan Stade wrote:
> is an integer button_type going to easily work for non-gtk platforms? don't we
> have some sort of generic enum or cross-platfrom event type?

The int matches the value that's being passed into ExtensionBrowserEventRouter;
I was maintaining that system. Looking into it further though, it's only needed
for a deprecated API ("pageActions") so I don't think it's worth putting any
effort into duplication that interface here.

I looked around for an existing enum for this, and looks like there's an event
defined in ui/view/events that's not appropriate, and a bunch of places where
people have written their own enum types. I might as well just do that.

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.cc:39:
GetExtensionService()->GetExtensionById(extension_id, false);
On 2012/05/11 21:15:12, Matt Perry wrote:
> nit: use extensions()->GetByID(). I think we're phasing out GetExtensionById

Done.

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.cc:61: return
extension->ShowConfigureContextMenus() ? SHOW_CONTEXT_MENU : NONE;
On 2012/05/11 21:52:08, Evan Stade wrote:
> doesn't this complain about missing default case?

Not clang, at least. This is an enum now anyway.

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
File chrome/browser/extensions/page_action_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.h:32: };
On 2012/05/11 21:52:08, Evan Stade wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Matt Perry, 2012-05-14 18:48:56]

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
File chrome/browser/extensions/badge_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
chrome/browser/extensions/badge_controller.h:19: // Controller of the "badges"
(aka "page actions") in the UI.
On 2012/05/14 03:59:01, kalman wrote:
> On 2012/05/11 22:01:17, Matt Perry wrote:
> > On 2012/05/11 21:52:08, Evan Stade wrote:
> > > confused about the terminology. I thought "badge" referred to the text
that
> > you
> > > can add to a browser action, e.g. the number of unread gmail messages.
> > 
> > Good point. This should be named ExtensionActionController.
> 
> Cool. "Badge" was an attempt to make an agnostic name wrt what these are about
> to become, "script badges", so I think ExtensionActionController would be
> likewise misleading.
> 
> How about "PageBoxController"? This is what that area is / will be called I
> think.

I'm not a fan of PageBox. The generic name for Page/Browser Actions is
ExtensionAction. What's wrong with ExtensionActionController?

[Evan Stade, 2012-05-14 19:24:06]

gtk lgtm, I'll leave others to decide the right class name.

https://chromiumcodereview.appspot.com/10381105/diff/8003/chrome/browser/exte...
File chrome/browser/extensions/page_action_controller.cc (right):

https://chromiumcodereview.appspot.com/10381105/diff/8003/chrome/browser/exte...
chrome/browser/extensions/page_action_controller.cc:72:
GetExtensionService()->browser_event_router()->PageActionExecuted(
ah, I see now that this function takes an integer for the button type. I don't
think that's particularly awesome but I think we should match that with
OnClicked, i.e. revert back to what you had before.

https://chromiumcodereview.appspot.com/10381105/diff/8003/chrome/browser/exte...
File chrome/browser/extensions/page_box_controller.h (right):

https://chromiumcodereview.appspot.com/10381105/diff/8003/chrome/browser/exte...
chrome/browser/extensions/page_box_controller.h:37: MOUSE_LEFT,
if you make this = 1, and the next = 2, etc., then conversion back to ints is
easier.

[not at google - send to devlin, 2012-05-15 01:23:56]

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
File chrome/browser/extensions/badge_controller.h (right):

http://codereview.chromium.org/10381105/diff/7002/chrome/browser/extensions/b...
chrome/browser/extensions/badge_controller.h:19: // Controller of the "badges"
(aka "page actions") in the UI.
On 2012/05/14 18:48:57, Matt Perry wrote:
> On 2012/05/14 03:59:01, kalman wrote:
> > On 2012/05/11 22:01:17, Matt Perry wrote:
> > > On 2012/05/11 21:52:08, Evan Stade wrote:
> > > > confused about the terminology. I thought "badge" referred to the text
> that
> > > you
> > > > can add to a browser action, e.g. the number of unread gmail messages.
> > > 
> > > Good point. This should be named ExtensionActionController.
> > 
> > Cool. "Badge" was an attempt to make an agnostic name wrt what these are
about
> > to become, "script badges", so I think ExtensionActionController would be
> > likewise misleading.
> > 
> > How about "PageBoxController"? This is what that area is / will be called I
> > think.
> 
> I'm not a fan of PageBox. The generic name for Page/Browser Actions is
> ExtensionAction. What's wrong with ExtensionActionController?

As discussed on IRC, made it ActionBoxController.

http://codereview.chromium.org/10381105/diff/8003/chrome/browser/extensions/p...
File chrome/browser/extensions/page_action_controller.cc (right):

http://codereview.chromium.org/10381105/diff/8003/chrome/browser/extensions/p...
chrome/browser/extensions/page_action_controller.cc:72:
GetExtensionService()->browser_event_router()->PageActionExecuted(
On 2012/05/14 19:24:07, Evan Stade wrote:
> ah, I see now that this function takes an integer for the button type. I don't
> think that's particularly awesome but I think we should match that with
> OnClicked, i.e. revert back to what you had before.

Done.

[commit-bot: I haz the power, 2012-05-15 01:30:11]

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

[commit-bot: I haz the power, 2012-05-15 02:17:30]

Try job failure for 10381105-14003 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-15 03:03:43]

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

[commit-bot: I haz the power, 2012-05-15 05:14:31]

Change committed as 137079