Page actions behave as disableable browser actions when script badges are enabled.

Page actions behave as disableable browser actions when script badges are enabled.

Some code is based off http://codereview.chromium.org/10703090/

Includes: transparentization of icons on views.
Does not include: overflow menu on cocoa updating properly. It currently only updates when the set of actions in the overflow menu changes, _not_ when any of those actions change state.

BUG=133094
TEST=Enable script badges; have extensions with page actions; observe them (as browser actions) changing state, both on the toolbar and in the overflow menu.

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

[Yoyo Zhou, 2012-07-11 23:03:15]

kalman: this is about 90% of the way there. Can you please review?

thakis: Can you please review cocoa? And about that other 10%: since the state
of the overflow menu is now going to change much more often than it is ever
opened, it would make sense to recreate the overflow menu at the time it is
clicked. But I have not seen how to do that with MenuButton, which just shows
its attachedMenu. Most of my other ideas are too ugly to behold. Any
suggestions?

[not at google - send to devlin, 2012-07-12 01:07:29]

lgtm

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:38: alpha.eraseColor(0x40000000);
0x40 isn't 40% alpha, it's 25% alpha? Or am I going crazy...?

Also might want to use SkColorSetARGB rather than hard-coding hex.

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:141:
views::CustomButton::BS_NORMAL :
This should be PUSHED If it's pushed; can use the value of menu_visible_ it
looks like.

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/ext...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:92:
return view->button()->IsEnabled(owner_->GetCurrentTabId());
Same comment here and in the GTK code:

ExtensionToolbarModel already knows to ignore events when browser actions are
disabled. Do we need this too? It's just less code to let the model deal with
it.

(otoh if this kind of thing changes the appearance of buttons, or doesn't play a
sound, or is used by some accessibility tool, or whatever...)

[Yoyo Zhou, 2012-07-12 01:33:27]

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:38: alpha.eraseColor(0x40000000);
On 2012/07/12 01:07:29, kalman wrote:
> 0x40 isn't 40% alpha, it's 25% alpha? Or am I going crazy...?
> 
> Also might want to use SkColorSetARGB rather than hard-coding hex.

Yup, 25% alpha. I was trying to find some good SkColor functions and failed, so
I'll try that one.

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:141:
views::CustomButton::BS_NORMAL :
On 2012/07/12 01:07:29, kalman wrote:
> This should be PUSHED If it's pushed; can use the value of menu_visible_ it
> looks like.

Done.

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/ext...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/10700007/diff/1002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:92:
return view->button()->IsEnabled(owner_->GetCurrentTabId());
On 2012/07/12 01:07:29, kalman wrote:
> Same comment here and in the GTK code:
> 
> ExtensionToolbarModel already knows to ignore events when browser actions are
> disabled. Do we need this too? It's just less code to let the model deal with
> it.
> 
> (otoh if this kind of thing changes the appearance of buttons, or doesn't play
a
> sound, or is used by some accessibility tool, or whatever...)

I seem to recall this changes the appearance of menu items to make them
disabled.

[Yoyo Zhou, 2012-07-12 18:06:10]

sky: can you please review views?
estade: can you please review gtk?

[Evan Stade, 2012-07-13 08:42:44]

gtk lgtm

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:241:
gtk_widget_set_sensitive(button(), enabled ? TRUE : FALSE);
just enabled is fine.

[sky, 2012-07-16 15:43:46]

LGTM

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:31: SkBitmap
TransparentizeImage(const SkBitmap& image) {
How about MakeTransparent and add a description.

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:151: // if (!enabled())
Remove commented out line.

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.h (right):

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.h:92: // the built-in views
enabled/SetEnabled because disabled views cannot
cannot -> do not

[Yoyo Zhou, 2012-07-16 17:45:45]

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:241:
gtk_widget_set_sensitive(button(), enabled ? TRUE : FALSE);
On 2012/07/13 08:42:44, Evan Stade wrote:
> just enabled is fine.

Done.

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:31: SkBitmap
TransparentizeImage(const SkBitmap& image) {
On 2012/07/16 15:43:46, sky wrote:
> How about MakeTransparent and add a description.

Done.

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:151: // if (!enabled())
On 2012/07/16 15:43:46, sky wrote:
> Remove commented out line.

Done.

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.h (right):

http://codereview.chromium.org/10700007/diff/2004/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.h:92: // the built-in views
enabled/SetEnabled because disabled views cannot
On 2012/07/16 15:43:46, sky wrote:
> cannot -> do not

Done.

[Yoyo Zhou, 2012-07-16 22:09:59]

rsesek: since thakis seems to be OOO, can you please review cocoa?

Also, see the first message on this CL. To update the overflow menu properly,
one thought I've had is to override MenuButton's attachedMenu to instead
delegate to the BrowserActionsController to rebuild the menu each time it's
called. This is only twice per call to showMenu. But it might violate the
expectation of attachedMenu being a simple accessor. Any thoughts on this?

[Robert Sesek, 2012-07-17 15:06:45]

The Cocoa pieces here LGTM.

On 2012/07/16 22:09:59, Yoyo Zhou wrote:
> rsesek: since thakis seems to be OOO, can you please review cocoa?
> 
> Also, see the first message on this CL. To update the overflow menu properly,
> one thought I've had is to override MenuButton's attachedMenu to instead
> delegate to the BrowserActionsController to rebuild the menu each time it's
> called. This is only twice per call to showMenu. But it might violate the
> expectation of attachedMenu being a simple accessor. Any thoughts on this?

Whatever is the NSMenu's NSMenuDelegate can/should override |-menuNeedsUpdate:|
and perform any changes to the menu there. That's called before each time the
menu is shown. See wrench_menu_controller.mm for an example.

[Yoyo Zhou, 2012-07-18 05:04:15]

On Tue, Jul 17, 2012 at 8:06 AM,  <rsesek@chromium.org> wrote:
> Whatever is the NSMenu's NSMenuDelegate can/should override
> |-menuNeedsUpdate:|
> and perform any changes to the menu there. That's called before each time
> the
> menu is shown. See wrench_menu_controller.mm for an example.
>
> http://codereview.chromium.org/10700007/

Thanks; I will explore this further in a followup CL.

[commit-bot: I haz the power, 2012-07-18 05:04:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/10700007/19001

[commit-bot: I haz the power, 2012-07-18 06:19:57]

Change committed as 147188