Force browser action icon size to always be 19px

Force browser action icon size to always be 19px

Fixed the browser action bar so that it will render the icon normal size
if an icon is not specified. Also re-worked a test case for dynamically changing
the browser action icon (BrowserActionApiTest.DynamicBrowserAction).
Made the test more robust by checking the pixels of the icon to make sure it is actually
changed. Screenshots of before and after are posted on the bug.

BUG=111301
TEST=Load an extension with a browser action and no icon specified. The icon
now should be the default extension icon.

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

[Devlin, 2012-02-25 18:41:00]

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
File chrome/browser/extensions/browser_action_apitest.cc (right):

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:119: // Reset prev_id for
next test.
Please use proper grammar in comments.

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:122: // Tell the extension
to update using setIcon({path:...});
I know it's not your code, but you should change the ; to a . in this comment
(if you keep the line - see my comment on 130).  It makes it match the style on
106, and it also makes it exceedingly clear that this is a comment, even if you
only skim the right half of the line - code ends in semicolons; comments end in
periods.

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:130: // Test to make sure
the images are different.
The second set of comments seems superfluous, and there's a policy of not having
"useless" comments.  Since these are all described in detail in the process
above, I might consider changing the comment on line 119 to read something like
"Reset prev_id for the next test and repeat the process using
setIcon({path:...})."  You could then remove comments on lines 122, 127, and
130.  But if you think that it's more clear like this, then that's fine.

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:134: // TODO(aa): Would be
nice here to actually compare that the pixels change.
Isn't this what we are doing now?  It's safe to remove the TODO.

[cduvall, 2012-02-25 19:23:06]

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
File chrome/browser/extensions/browser_action_apitest.cc (right):

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:119: // Reset prev_id for
next test.
On 2012/02/25 18:41:00, D Cronin wrote:
> Please use proper grammar in comments.

Done.

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:122: // Tell the extension
to update using setIcon({path:...});
On 2012/02/25 18:41:00, D Cronin wrote:
> I know it's not your code, but you should change the ; to a . in this comment
> (if you keep the line - see my comment on 130).  It makes it match the style
on
> 106, and it also makes it exceedingly clear that this is a comment, even if
you
> only skim the right half of the line - code ends in semicolons; comments end
in
> periods.

Done.

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:130: // Test to make sure
the images are different.
On 2012/02/25 18:41:00, D Cronin wrote:
> The second set of comments seems superfluous, and there's a policy of not
having
> "useless" comments.  Since these are all described in detail in the process
> above, I might consider changing the comment on line 119 to read something
like
> "Reset prev_id for the next test and repeat the process using
> setIcon({path:...})."  You could then remove comments on lines 122, 127, and
> 130.  But if you think that it's more clear like this, then that's fine.

Done.

https://chromiumcodereview.appspot.com/9424024/diff/3001/chrome/browser/exten...
chrome/browser/extensions/browser_action_apitest.cc:134: // TODO(aa): Would be
nice here to actually compare that the pixels change.
On 2012/02/25 18:41:00, D Cronin wrote:
> Isn't this what we are doing now?  It's safe to remove the TODO.

Done.

[Devlin, 2012-02-25 19:26:24]

LGTM.

[cduvall, 2012-02-25 20:49:06]

Here's a CL for the small icon bug. Any feedback is appreciated. Thanks.

[cduvall, 2012-02-29 02:14:20]

Here's a CL for the small icon bug. Any feedback is appreciated. Thanks.

[Aaron Boodman, 2012-02-29 20:51:48]

Defaulting to the puzzle piece icon is a good idea, and is something we want to
do soon. But when we do it, it should be done across all platforms consistently.
This patch will only change GTK.

Bug 111301 is really just that the browser action icon adapts to the size of its
icon. It should not. It should always center the icon it is given within a 19px
square.

[cduvall, 2012-03-02 06:26:13]

On 2012/02/29 20:51:48, Aaron Boodman wrote:
> Defaulting to the puzzle piece icon is a good idea, and is something we want
to
> do soon. But when we do it, it should be done across all platforms
consistently.
> This patch will only change GTK.
> 
> Bug 111301 is really just that the browser action icon adapts to the size of
its
> icon. It should not. It should always center the icon it is given within a
19px
> square.

I took out the puzzle piece, and just made the icon render the standard size all
the time.

Before: http://imgur.com/LDa3N
After: http://imgur.com/2CuL4

Would you like me to make the puzzle piece default for all platforms? Would it
be best to do this in a new bug?

[Aaron Boodman, 2012-03-02 20:16:39]

Seems OK to me, but you need a GTK owner to also say: LGTM

http://codereview.chromium.org/9424024/diff/16001/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_apitest.cc (right):

http://codereview.chromium.org/9424024/diff/16001/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_apitest.cc:115: // Test to make sure
the images are different.
If you just want to ensure that the icons are difference, couldn't you also just
store the SkBitmap on line 104 and compare it here on 121?

[Evan Stade, 2012-03-02 21:10:38]

gtk lgtm

[cduvall, 2012-03-02 22:18:30]

http://codereview.chromium.org/9424024/diff/16001/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_apitest.cc (right):

http://codereview.chromium.org/9424024/diff/16001/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_apitest.cc:115: // Test to make sure
the images are different.
On 2012/03/02 20:16:39, Aaron Boodman wrote:
> If you just want to ensure that the icons are difference, couldn't you also
just
> store the SkBitmap on line 104 and compare it here on 121?

That would also work. I just wanted to make sure the actual pixels in the
SkBitmap changed, and not just compare the two SkBitmaps. Would you like me to
switch it to comparing the SkBitmaps?

[Aaron Boodman, 2012-03-02 22:34:46]

LGTM

http://codereview.chromium.org/9424024/diff/16001/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_apitest.cc (right):

http://codereview.chromium.org/9424024/diff/16001/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_apitest.cc:115: // Test to make sure
the images are different.
On 2012/03/02 22:18:30, cduvall wrote:
> On 2012/03/02 20:16:39, Aaron Boodman wrote:
> > If you just want to ensure that the icons are difference, couldn't you also
> just
> > store the SkBitmap on line 104 and compare it here on 121?
> 
> That would also work. I just wanted to make sure the actual pixels in the
> SkBitmap changed, and not just compare the two SkBitmaps. Would you like me to
> switch it to comparing the SkBitmaps?

Hm, I thought there was functionality in SkBitmap to compare pixels between two
instances, but I can't find it now.

I read the docs on getGenerationID() and it seems fine for what you're doing
here.

[commit-bot: I haz the power, 2012-03-02 22:49:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/9424024/16001

[Aaron Boodman, 2012-03-02 22:57:45]

I would love the puzzle piece as default on all platforms (we have
been wanting to do this for other reasons...).

It's going to be a bit more challenging since it will likely require
cocoa/macosx changes. I thought that your team did not have access to
machines you can do mac ui work on.

Perhaps you could implement the linux and windows changes, then we can
recruit someone on the chrome team to do the mac side.

- a

On Thu, Mar 1, 2012 at 10:26 PM,  <cduvall@chromium.org> wrote:
> On 2012/02/29 20:51:48, Aaron Boodman wrote:
>>
>> Defaulting to the puzzle piece icon is a good idea, and is something we
>> want
>
> to
>>
>> do soon. But when we do it, it should be done across all platforms
>
> consistently.
>>
>> This patch will only change GTK.
>
>
>> Bug 111301 is really just that the browser action icon adapts to the size
>> of
>
> its
>>
>> icon. It should not. It should always center the icon it is given within a
>
> 19px
>>
>> square.
>
>
> I took out the puzzle piece, and just made the icon render the standard size
> all
> the time.
>
> Before: http://imgur.com/LDa3N
> After: http://imgur.com/2CuL4
>
> Would you like me to make the puzzle piece default for all platforms? Would
> it
> be best to do this in a new bug?
>
> https://chromiumcodereview.appspot.com/9424024/

[commit-bot: I haz the power, 2012-03-03 00:11:40]

Try job failure for 9424024-16001 (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-03-03 18:32:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/9424024/22001

[commit-bot: I haz the power, 2012-03-03 19:49:12]

Try job failure for 9424024-22001 (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-03-07 01:15:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/9424024/31001

[commit-bot: I haz the power, 2012-03-07 03:18:47]

Change committed as 125314