Fix disabled browser action appearance on Mac.

Fix disabled browser action appearance on Mac.

BUG=150795
TEST=Using chrome.browserAction.disable(), check that a disabled browser action is grayed out and not clickable.


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

[Nico, 2012-09-21 02:02:02]

Thanks for working on this!

https://codereview.chromium.org/10963016/diff/3001/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/10963016/diff/3001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:259: [self
setImage:gfx::Image(image).ToNSImage()];
It looks like we're round-tripping to skia and back just for dimming. Can you do
this in paint (see below)?

If you decide to keep it here,can you avoid the roundtrip in the "enabled" case
at least?

https://codereview.chromium.org/10963016/diff/3001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:262: [self
setEnabled:enabled ? YES : NO];
just "enabled" is enough. Drop the ?YES:NO.

https://codereview.chromium.org/10963016/diff/3001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:326: fraction:1.0
Can't you just change this to "enabled ? 1.0 : 0.4" instead?

[Yoyo Zhou, 2012-09-21 18:04:59]

By the way, I just noticed there's a bunch of very similar code between
-compositedImage and -drawWithFrame: inView: , but I don't understand the subtle
differences well enough to try to refactor them.

https://chromiumcodereview.appspot.com/10963016/diff/3001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://chromiumcodereview.appspot.com/10963016/diff/3001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:259: [self
setImage:gfx::Image(image).ToNSImage()];
On 2012/09/21 02:02:02, Nico wrote:
> It looks like we're round-tripping to skia and back just for dimming. Can you
do
> this in paint (see below)?
> 
> If you decide to keep it here,can you avoid the roundtrip in the "enabled"
case
> at least?

Ah, I didn't realize this was something to avoid. Doing it below seems to work.

https://chromiumcodereview.appspot.com/10963016/diff/3001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:262: [self
setEnabled:enabled ? YES : NO];
On 2012/09/21 02:02:02, Nico wrote:
> just "enabled" is enough. Drop the ?YES:NO.

Done.

https://chromiumcodereview.appspot.com/10963016/diff/3001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:326: fraction:1.0
On 2012/09/21 02:02:02, Nico wrote:
> Can't you just change this to "enabled ? 1.0 : 0.4" instead?

Done. Slightly scared by operator precendence.

[Nico, 2012-09-24 00:33:54]

lgtm

[commit-bot: I haz the power, 2012-09-24 06:15:26]

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

[commit-bot: I haz the power, 2012-09-24 15:26:34]

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

[commit-bot: I haz the power, 2012-09-24 15:26:53]

Change committed as 158280