Cache network connecting bitmaps.

Cache network connecting bitmaps.
Improve performance by reducing the number of bitmap copying.
BUG=chromium-os:12543
TEST=make sure network connecting looks the same

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

[Charlie Lee, 2011-02-25 23:25:16]

One more review for you. No rush.

[stevenjb, 2011-02-28 20:39:05]

I added some comments, but in a nutshell I think we should either enforce const*
correctness throughout and test or CHECK for NULL where appropriate, or switch
to const& for the icon (but not the badge).

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:87: SkBitmap*
NetworkMenu::kAnimatingImages = new SkBitmap[kNumAnimatingImages];
Any reason to use 'new' here instead of putting this into a local namespace and
using 'SkBitmap kAnimatingImages[kAnimatingImages]' ?

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:368: bool black) {
Since the result is never allowed to be NULL, and since we do not test for NULL
where we use the result, I would return a const SkBitmap& instead of a *.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:384: bool black) {
ditto

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:402: bool black) {
ditto

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:430: SkBitmap*
NetworkMenu::BadgeForNetworkTechnology(
const SkBitmap*

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:502: SkBitmap
NetworkMenu::IconForDisplay(SkBitmap* icon, SkBitmap* badge) {
Do we want to copy the SkBitmap here, or return a reference?
SkBitmap* icon should be const SkBitmap&, since we dereference it without
checking.
const SkBitmap* badge.

Alternately, if you prefer both icon and badge to be const*, there should be a
test or a CHECK wherever the values are dereferenced.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu_button.h (right):

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:77: void
SetIconAndBadge(SkBitmap* icon, SkBitmap* badge);
const SkBitmap& icon, const SkBitmap* badge

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:79: void
SetIconOnly(SkBitmap* icon);
const SkBitmap& icon

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:81: void
SetBadgeOnly(SkBitmap* badge);
const SkBitmap* badge

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:93: SkBitmap* icon_;
const SkBitmap& icon_

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:95: SkBitmap* badge_;
const SkBitmap* badge_

[Charlie Lee, 2011-02-28 21:26:33]

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:87: SkBitmap*
NetworkMenu::kAnimatingImages = new SkBitmap[kNumAnimatingImages];
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> Any reason to use 'new' here instead of putting this into a local namespace
and
> using 'SkBitmap kAnimatingImages[kAnimatingImages]' ?

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:368: bool black) {
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> Since the result is never allowed to be NULL, and since we do not test for
NULL
> where we use the result, I would return a const SkBitmap& instead of a *.

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:384: bool black) {
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> ditto

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:402: bool black) {
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> ditto

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:430: SkBitmap*
NetworkMenu::BadgeForNetworkTechnology(
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> const SkBitmap*

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:502: SkBitmap
NetworkMenu::IconForDisplay(SkBitmap* icon, SkBitmap* badge) {
Since we are not storing the newly created SkBitmap anywhere, it makes sense to
just return a copy, right?

On 2011/02/28 20:39:05, Steven Bennetts wrote:
> Do we want to copy the SkBitmap here, or return a reference?
> SkBitmap* icon should be const SkBitmap&, since we dereference it without
> checking.
> const SkBitmap* badge.
> 
> Alternately, if you prefer both icon and badge to be const*, there should be a
> test or a CHECK wherever the values are dereferenced.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu_button.h (right):

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:77: void
SetIconAndBadge(SkBitmap* icon, SkBitmap* badge);
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> const SkBitmap& icon, const SkBitmap* badge

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:79: void
SetIconOnly(SkBitmap* icon);
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> const SkBitmap& icon

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:81: void
SetBadgeOnly(SkBitmap* badge);
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> const SkBitmap* badge

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:93: SkBitmap* icon_;
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> const SkBitmap& icon_

Done.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_button.h:95: SkBitmap* badge_;
On 2011/02/28 20:39:05, Steven Bennetts wrote:
> const SkBitmap* badge_

Done.

[stevenjb, 2011-02-28 23:02:37]

LGTM with nits.

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6591017/diff/3009/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:502: SkBitmap
NetworkMenu::IconForDisplay(SkBitmap* icon, SkBitmap* badge) {
Ah, yes, my bad, I didn't notice that this was creating a new bitmap.

On 2011/02/28 21:26:33, Charlie Lee wrote:
> Since we are not storing the newly created SkBitmap anywhere, it makes sense
to
> just return a copy, right?
> 
> On 2011/02/28 20:39:05, Steven Bennetts wrote:
> > Do we want to copy the SkBitmap here, or return a reference?
> > SkBitmap* icon should be const SkBitmap&, since we dereference it without
> > checking.
> > const SkBitmap* badge.
> > 
> > Alternately, if you prefer both icon and badge to be const*, there should be
a
> > test or a CHECK wherever the values are dereferenced.
>

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:366: // static
nit: Because we rely on IconForFoo returning non-NULL values, but
BadgeForNetworkTechnology does return NULL, lett's at least put a comment in
front of these along the lines of "Expected to never return NULL"

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:417: double alpha =
static_cast<double>(index) / (kNumAnimatingImages - 1) / 3;
nit: parens are helpful around multiple / operators.

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:418: ResourceBundle& rb =
ResourceBundle::GetSharedInstance();
nit: const&

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/internet_options_handler.cc (right):

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/internet_options_handler.cc:936: SkBitmap* icon =
rb.GetBitmapNamed(IDR_STATUSBAR_WIRED_BLACK);
nit: const*

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/internet_options_handler.cc:937: SkBitmap* badge =
!ethernet_network ||
nit: const*

[Charlie Lee, 2011-02-28 23:54:13]

I will check this in. Thanks.

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:366: // static
On 2011/02/28 23:02:37, Steven Bennetts wrote:
> nit: Because we rely on IconForFoo returning non-NULL values, but
> BadgeForNetworkTechnology does return NULL, lett's at least put a comment in
> front of these along the lines of "Expected to never return NULL"

Done.

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:417: double alpha =
static_cast<double>(index) / (kNumAnimatingImages - 1) / 3;
On 2011/02/28 23:02:37, Steven Bennetts wrote:
> nit: parens are helpful around multiple / operators.

Done.

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:418: ResourceBundle& rb =
ResourceBundle::GetSharedInstance();
Can't. GetBitmapNamed is not a const function.

On 2011/02/28 23:02:37, Steven Bennetts wrote:
> nit: const&

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/webu...
File chrome/browser/chromeos/webui/internet_options_handler.cc (right):

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/internet_options_handler.cc:936: SkBitmap* icon =
rb.GetBitmapNamed(IDR_STATUSBAR_WIRED_BLACK);
On 2011/02/28 23:02:37, Steven Bennetts wrote:
> nit: const*

Done.

http://codereview.chromium.org/6591017/diff/1015/chrome/browser/chromeos/webu...
chrome/browser/chromeos/webui/internet_options_handler.cc:937: SkBitmap* badge =
!ethernet_network ||
On 2011/02/28 23:02:37, Steven Bennetts wrote:
> nit: const*

Done.