Implemented nicer battery status

Implemented nicer battery status


BUG=chromium-os:17469
TEST=Click battery icon, confirm it meets spec

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

[achuithb, 2011-08-09 18:58:07]

I don't understand enough about the menu/submenu controls in views to review
that part of the code in any meaningful way, so maybe another reviewer would be
appropriate for those changes?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:166: class PowerLibraryStubImpl :
public PowerLibrary {
Maybe a comment here explaining what you're doing? I understand it from the code
but it's not obvious.

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:244: }
newline here?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:94: SkBitmap image;
Maybe move this to right before extractSubset?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:176: 
Shouldn't this newline be after DrawBitmapInt?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:257: return std::wstring();
What is GetLabel used for?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:340: status_ = NULL;
Is status_ leaked or does AddChildView transfer ownership to the submenu?

Also, why create MenuItemView and StatusView on the heap? Why not make these
stack variables and get rid of the class variables menu_ and status_. Am I
missing something?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:414: // or got down to 0
period at end of comment.

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:339: virtual std::string GetClassName()
const;
could you please also add an OVERRIDE to this function for consistency?

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view_gtk.cc (right):

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view_gtk.cc:33: if (child_count() == 1 &&
title_.size() == 0) {
title_.empty instead of comparison of size with 0?

I don't understand this code sufficiently - you should probably get somebody
else familiar with this file to review this change. What happens with
child_count is not 1?

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view_win.cc (right):

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view_win.cc:22: if (child_count() == 1 &&
title_.size() == 0) {
title_.empty() would be clearer

Is this code for Windows? Is this change necessary?

[DaveMoore, 2011-08-12 21:03:10]

Adding stevenjb

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:166: class PowerLibraryStubImpl :
public PowerLibrary {
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> Maybe a comment here explaining what you're doing? I understand it from the
code
> but it's not obvious.

Done.

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:244: }
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> newline here?

Done.

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:94: SkBitmap image;
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> Maybe move this to right before extractSubset?

Done.

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:176: 
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> Shouldn't this newline be after DrawBitmapInt?

Done.

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:340: status_ = NULL;
AddChildView transfers ownership.
That aren't stack variables because they're accessed from within the status
view.

On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> Is status_ leaked or does AddChildView transfer ownership to the submenu?
> 
> Also, why create MenuItemView and StatusView on the heap? Why not make these
> stack variables and get rid of the class variables menu_ and status_. Am I
> missing something?

http://codereview.chromium.org/7491083/diff/5001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.cc:414: // or got down to 0
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> period at end of comment.

Done.

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:339: virtual std::string GetClassName()
const;
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> could you please also add an OVERRIDE to this function for consistency?

Done.

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view_win.cc (right):

http://codereview.chromium.org/7491083/diff/5001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view_win.cc:22: if (child_count() == 1 &&
title_.size() == 0) {
Done...It's to make Windows consistent.
On 2011/08/09 18:58:07, achuith.bhandarkar wrote:
> title_.empty() would be clearer
> 
> Is this code for Windows? Is this change necessary?

[stevenjb, 2011-08-12 22:03:49]

http://codereview.chromium.org/7491083/diff/12001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/7491083/diff/12001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:216: if (battery_percentage_ == 0)
100?

[DaveMoore, 2011-08-12 22:23:11]

http://codereview.chromium.org/7491083/diff/12001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/7491083/diff/12001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:216: if (battery_percentage_ == 0)
On 2011/08/12 22:03:49, Steven Bennetts wrote:
> 100?

Done.

[stevenjb, 2011-08-12 22:31:57]

LGTM