Refactor cros library code into central location and have the UI elements obs...

Refactor cros library code into central location and have the UI elements observe changes.
- implemented power menu button showing remaining battery info.
- implemented clock menu button showing detailed clock info.
- also implemented monitoring of network status change.
- don't show highlighted state for these menu buttons
BUG=23923
TEST=none

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

[sky, 2009-10-06 16:02:26]

I didn't make it very far and instead got hung upon how the classes are
designed. Let me know if you disagree with my comments.

http://codereview.chromium.org/251099/diff/5006/37
File chrome/browser/chromeos/cros_library.cc (right):

http://codereview.chromium.org/251099/diff/5006/37#newcode23
Line 23: observer->PowerChanged(status.line_power_on, status.battery_state ==
This is very unusual to have an observer notified immediately from the
AddObserver method. At a minimum you should document this.

http://codereview.chromium.org/251099/diff/5006/37#newcode52
Line 52: : last_power_status_(),
nit: no need to include last_power_status_ and ssid_ here, they are handled for
you.

http://codereview.chromium.org/251099/diff/5006/37#newcode57
Line 57: loaded_ = false;
default initialize this rather then setting it.

http://codereview.chromium.org/251099/diff/5006/35
File chrome/browser/chromeos/cros_library.h (right):

http://codereview.chromium.org/251099/diff/5006/35#newcode40
Line 40: class CrosLibrary {
This doesn't feel like it scales rather well. I think it would be cleaner to
break this into smaller chunks:

1. The CrosLibrary part whose only job is to LoadCros and tell whether it
successfully loaded the library.
2. The network chunk that has all the network functionality, making use of
CrosLibrary.
3. The power chunk, again using CrosLibrary.

This way CrosLibrary doesn't become a dumping ground for new functionality. Any
time we have a new functional unit we expose ends in a separate class, similar
to what's on the chromeos side.

[Charlie Lee (do not use), 2009-10-06 16:11:16]

See below...

http://codereview.chromium.org/251099/diff/5006/37
File chrome/browser/chromeos/cros_library.cc (right):

http://codereview.chromium.org/251099/diff/5006/37#newcode23
Line 23: observer->PowerChanged(status.line_power_on, status.battery_state ==
On 2009/10/06 16:02:27, sky wrote:
> This is very unusual to have an observer notified immediately from the
> AddObserver method. At a minimum you should document this.

The reason it notifies immediately is so that I don't have to provide a method
to get the initial state. Is that reasonable?

http://codereview.chromium.org/251099/diff/5006/37#newcode52
Line 52: : last_power_status_(),
On 2009/10/06 16:02:27, sky wrote:
> nit: no need to include last_power_status_ and ssid_ here, they are handled
for
> you.

Done.

http://codereview.chromium.org/251099/diff/5006/37#newcode57
Line 57: loaded_ = false;
On 2009/10/06 16:02:27, sky wrote:
> default initialize this rather then setting it.

Done.

http://codereview.chromium.org/251099/diff/5006/35
File chrome/browser/chromeos/cros_library.h (right):

http://codereview.chromium.org/251099/diff/5006/35#newcode40
Line 40: class CrosLibrary {
On 2009/10/06 16:02:27, sky wrote:
> This doesn't feel like it scales rather well. I think it would be cleaner to
> break this into smaller chunks:
> 
> 1. The CrosLibrary part whose only job is to LoadCros and tell whether it
> successfully loaded the library.
> 2. The network chunk that has all the network functionality, making use of
> CrosLibrary.
> 3. The power chunk, again using CrosLibrary.
> 
> This way CrosLibrary doesn't become a dumping ground for new functionality.
Any
> time we have a new functional unit we expose ends in a separate class, similar
> to what's on the chromeos side.

Would the network/power chunk be children of this singleton CrosLibrary class?
So a call would be something like:

Singleton<CrosLibrary>()->GetNetworkLibrary()->AddObserver(this);

[sky, 2009-10-06 16:17:46]

On Tue, Oct 6, 2009 at 9:11 AM,  <chocobo@google.com> wrote:
>
> See below...
>
>
> http://codereview.chromium.org/251099/diff/5006/37
> File chrome/browser/chromeos/cros_library.cc (right):
>
> http://codereview.chromium.org/251099/diff/5006/37#newcode23
> Line 23: observer->PowerChanged(status.line_power_on,
> status.battery_state ==
> On 2009/10/06 16:02:27, sky wrote:
>>
>> This is very unusual to have an observer notified immediately from the
>> AddObserver method. At a minimum you should document this.
>
> The reason it notifies immediately is so that I don't have to provide a
> method to get the initial state. Is that reasonable?

Seems like folks might have other needs to query at some random point,
and I could see having real methods for querying making testing
easier.

> http://codereview.chromium.org/251099/diff/5006/35
> File chrome/browser/chromeos/cros_library.h (right):
>
> http://codereview.chromium.org/251099/diff/5006/35#newcode40
> Line 40: class CrosLibrary {
> On 2009/10/06 16:02:27, sky wrote:
>>
>> This doesn't feel like it scales rather well. I think it would be
>
> cleaner to
>>
>> break this into smaller chunks:
>
>> 1. The CrosLibrary part whose only job is to LoadCros and tell whether
>
> it
>>
>> successfully loaded the library.
>> 2. The network chunk that has all the network functionality, making
>
> use of
>>
>> CrosLibrary.
>> 3. The power chunk, again using CrosLibrary.
>
>> This way CrosLibrary doesn't become a dumping ground for new
>
> functionality. Any
>>
>> time we have a new functional unit we expose ends in a separate class,
>
> similar
>>
>> to what's on the chromeos side.
>
> Would the network/power chunk be children of this singleton CrosLibrary
> class?

No, they would internally use CrosLibrary, but not be defined or used
by CrosLibrary.

> So a call would be something like:
>
> Singleton<CrosLibrary>()->GetNetworkLibrary()->AddObserver(this);

I was thinking more of:

CrosNetworking::Get()->AddObserver(this);

and

CrosPower::Get()->AddObserver(this);

The get would internally use a Singleton<Foo>.


  -Scott

[Charlie Lee (do not use), 2009-10-06 19:54:49]

I refactored CrosLibrary and pulled out CrosNetworkLibrary and CrosPowerLibrary.

[sky, 2009-10-06 21:58:53]

http://codereview.chromium.org/251099/diff/103/5026
File chrome/browser/chromeos/clock_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/103/5026#newcode21
Line 21: SetFont(ResourceBundle::GetSharedInstance().GetFont(
Any reason you want this bold?

http://codereview.chromium.org/251099/diff/103/5025
File chrome/browser/chromeos/clock_menu_button.h (right):

http://codereview.chromium.org/251099/diff/103/5025#newcode14
Line 14: // This button will show the current time.
nit: will show -> shows

http://codereview.chromium.org/251099/diff/103/5025#newcode15
Line 15: class ClockMenuButton : public views::MenuButton,
Showing a menu with the time seems horrific here. It seems like a tooltip is
what you want.

http://codereview.chromium.org/251099/diff/103/5023
File chrome/browser/chromeos/cros_library.cc (right):

http://codereview.chromium.org/251099/diff/103/5023#newcode24
Line 24: if (error) {
nit: nuke {}

http://codereview.chromium.org/251099/diff/103/5023#newcode37
Line 37: 
nit: nuke extra line.

http://codereview.chromium.org/251099/diff/103/5021
File chrome/browser/chromeos/cros_library.h (right):

http://codereview.chromium.org/251099/diff/103/5021#newcode8
Line 8: 
nit: nuke the extra line here.

http://codereview.chromium.org/251099/diff/103/5021#newcode15
Line 15: // library APIs. Classes can add themselves as observers.
nit: last sentence doesn't apply anymore.

http://codereview.chromium.org/251099/diff/103/5021#newcode18
Line 18: 
nit: nuke extra line here.

http://codereview.chromium.org/251099/diff/103/5021#newcode20
Line 20: CrosPowerLibrary* GetPowerLibrary();
All this class need have is:

// Returns true if the chrome os library was loaded.
static bool loaded();

Push the getters for the network/power library into their respective classes.

http://codereview.chromium.org/251099/diff/103/5030
File chrome/browser/chromeos/cros_network_library.cc (right):

http://codereview.chromium.org/251099/diff/103/5030#newcode11
Line 11: void CrosNetworkLibrary::AddObserver(NetworkObserver* observer) {
Order of methods should match that of header.

http://codereview.chromium.org/251099/diff/103/5030#newcode13
Line 13: if (loaded_) {
nit: no {}

http://codereview.chromium.org/251099/diff/103/5030#newcode38
Line 38: DLOG(WARNING) << "Network " << service.ssid <<
Does this really need to be a warning? At best it should be INFO.

http://codereview.chromium.org/251099/diff/103/5030#newcode46
Line 46: network->ethernet_ = service.state == chromeos::STATE_READY;
Do we support being connected to both ethernet and wifi?

http://codereview.chromium.org/251099/diff/103/5030#newcode80
Line 80: DLOG(WARNING) << "Init " << service.ssid <<
WARNING -> INFO

http://codereview.chromium.org/251099/diff/103/5030#newcode86
Line 86: if (service.type == chromeos::TYPE_ETHERNET) {
Can this code be shared with NetworkStatusChangedHandler?

http://codereview.chromium.org/251099/diff/103/5030#newcode112
Line 112: chromeos::ServiceInfo service = service_status->services[i];
This results in copying service. You should instead have service be a const &.

http://codereview.chromium.org/251099/diff/103/5030#newcode116
Line 116: service.encryption, service.signal_strength));
nit: align with second (

http://codereview.chromium.org/251099/diff/103/5031
File chrome/browser/chromeos/cros_network_library.h (right):

http://codereview.chromium.org/251099/diff/103/5031#newcode36
Line 36: // Classes can add themselves as observers.
You should document folks should get an instance from the CrosLibrary.

http://codereview.chromium.org/251099/diff/103/5031#newcode39
Line 39: class NetworkObserver {
nit: as clients are already prefixing this with CrosNetworkLibrary::, you may as
well just call it Observer.

http://codereview.chromium.org/251099/diff/103/5031#newcode41
Line 41: virtual void NetworkChanged(std::string ssid,
You should be consistent with other observers and push this state as getters
into CrosNetworkLibrary and have NetworkChanged take only the
CrosNetworkLibrary. And again, I wouldn't call NetworkChanged from AddObserver.

http://codereview.chromium.org/251099/diff/103/5031#newcode48
Line 48: virtual ~CrosNetworkLibrary() {}
Does this really need to be virtual?

http://codereview.chromium.org/251099/diff/103/5031#newcode69
Line 69: bool loaded_;
Nuke this and instead call into CrosLibrary::loaded().

http://codereview.chromium.org/251099/diff/103/5032
File chrome/browser/chromeos/cros_power_library.cc (right):

http://codereview.chromium.org/251099/diff/103/5032#newcode9
Line 9: void CrosPowerLibrary::AddObserver(PowerObserver* observer) {
nit: order of methods should match header.

http://codereview.chromium.org/251099/diff/103/5032#newcode28
Line 28: : loaded_(loaded) {
You should initialize power_status_connection_, and PowerStatus doesn't have an
empty constructor, so you either need to add one to PowerStatus, or reset all
the fields as is appropriate.

http://codereview.chromium.org/251099/diff/103/5032#newcode44
Line 44: DLOG(WARNING) << "Power" <<
WARNING -> INFO

http://codereview.chromium.org/251099/diff/103/5029
File chrome/browser/chromeos/cros_power_library.h (right):

http://codereview.chromium.org/251099/diff/103/5029#newcode15
Line 15: class PowerObserver {
nit: as clients are already prefixing this with CrosPowerLibrary::, you may as
well just call it Observer.

http://codereview.chromium.org/251099/diff/103/5029#newcode17
Line 17: virtual void PowerChanged(bool line_power_on,
Same comment as NetworkObserver: push getters into class, and have PowerChanged
take CrosPowerLibrary*.

http://codereview.chromium.org/251099/diff/103/5029#newcode20
Line 20: int64 battery_time_to_empty,
Use base::TimeDelta for times.

http://codereview.chromium.org/251099/diff/103/5029#newcode25
Line 25: virtual ~CrosPowerLibrary();
Do you really need this to be virtual?

http://codereview.chromium.org/251099/diff/103/5029#newcode37
Line 37: bool loaded_;
Nuke this and instead call into CrosLibrary::loaded().

http://codereview.chromium.org/251099/diff/103/5018
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/103/5018#newcode102
Line 102: Singleton<CrosLibrary>()->GetNetworkLibrary()->
Use of Singleton<Foo> is an implementation detail, instead you should have
CrosLibrary::Get(), which internally does whatever it needs to.

http://codereview.chromium.org/251099/diff/103/5022
File chrome/browser/chromeos/network_menu_button.h (right):

http://codereview.chromium.org/251099/diff/103/5022#newcode101
Line 101: // The current connected (or connecting) wireless ssid.
Is there a reason to cache these here rather than keeping them in
CrosNetworkLibrary?

http://codereview.chromium.org/251099/diff/103/5019
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/103/5019#newcode64
Line 64: // The first item shows the percentage of battery left.
As with the time button, these don't feel like these should be menus, rather
tooltips or a bubble.

http://codereview.chromium.org/251099/diff/103/5020
File chrome/browser/chromeos/power_menu_button.h (right):

http://codereview.chromium.org/251099/diff/103/5020#newcode17
Line 17: // It will also handle the status icon changing and connecting to
another
nit: no will, it does.

http://codereview.chromium.org/251099/diff/103/5020#newcode76
Line 76: bool line_power_on_;
Again, is there a reason to cache these here rather than getting them from the
PowerLibrary?

http://codereview.chromium.org/251099/diff/103/5028
File views/controls/button/text_button.h (right):

http://codereview.chromium.org/251099/diff/103/5028#newcode97
Line 97: void SetFont(gfx::Font font);
const gfx::Font&

[Charlie Lee (do not use), 2009-10-07 09:53:04]

Thanks. Made the fixes.

http://codereview.chromium.org/251099/diff/103/5026
File chrome/browser/chromeos/clock_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/103/5026#newcode21
Line 21: SetFont(ResourceBundle::GetSharedInstance().GetFont(
On 2009/10/06 21:58:53, sky wrote:
> Any reason you want this bold?

This was bold in the previous implementation, so I kept it that way.

http://codereview.chromium.org/251099/diff/103/5025
File chrome/browser/chromeos/clock_menu_button.h (right):

http://codereview.chromium.org/251099/diff/103/5025#newcode14
Line 14: // This button will show the current time.
On 2009/10/06 21:58:53, sky wrote:
> nit: will show -> shows

Done.

http://codereview.chromium.org/251099/diff/103/5025#newcode15
Line 15: class ClockMenuButton : public views::MenuButton,
On 2009/10/06 21:58:53, sky wrote:
> Showing a menu with the time seems horrific here. It seems like a tooltip is
> what you want.

I envision the menu having a few options to change how the time is displayed,
maybe an option to change the timezone, and also a menu item that will jump to
the date/time section of the options dialog.

So that's why I made it a menu button. This is how it works on my Mac and it
seems to be pretty intuitive.

http://codereview.chromium.org/251099/diff/103/5023
File chrome/browser/chromeos/cros_library.cc (right):

http://codereview.chromium.org/251099/diff/103/5023#newcode24
Line 24: if (error) {
On 2009/10/06 21:58:53, sky wrote:
> nit: nuke {}

Done.

http://codereview.chromium.org/251099/diff/103/5023#newcode37
Line 37: 
On 2009/10/06 21:58:53, sky wrote:
> nit: nuke extra line.

Done.

http://codereview.chromium.org/251099/diff/103/5021
File chrome/browser/chromeos/cros_library.h (right):

http://codereview.chromium.org/251099/diff/103/5021#newcode8
Line 8: 
On 2009/10/06 21:58:53, sky wrote:
> nit: nuke the extra line here.

Done.

http://codereview.chromium.org/251099/diff/103/5021#newcode15
Line 15: // library APIs. Classes can add themselves as observers.
On 2009/10/06 21:58:53, sky wrote:
> nit: last sentence doesn't apply anymore.

Done.

http://codereview.chromium.org/251099/diff/103/5021#newcode18
Line 18: 
On 2009/10/06 21:58:53, sky wrote:
> nit: nuke extra line here.

Done.

http://codereview.chromium.org/251099/diff/103/5021#newcode20
Line 20: CrosPowerLibrary* GetPowerLibrary();
On 2009/10/06 21:58:53, sky wrote:
> All this class need have is:
> 
> // Returns true if the chrome os library was loaded.
> static bool loaded();
> 
> Push the getters for the network/power library into their respective classes.

Done.

http://codereview.chromium.org/251099/diff/103/5030
File chrome/browser/chromeos/cros_network_library.cc (right):

http://codereview.chromium.org/251099/diff/103/5030#newcode11
Line 11: void CrosNetworkLibrary::AddObserver(NetworkObserver* observer) {
On 2009/10/06 21:58:53, sky wrote:
> Order of methods should match that of header.

Done.

http://codereview.chromium.org/251099/diff/103/5030#newcode13
Line 13: if (loaded_) {
On 2009/10/06 21:58:53, sky wrote:
> nit: no {}

Done.

http://codereview.chromium.org/251099/diff/103/5030#newcode38
Line 38: DLOG(WARNING) << "Network " << service.ssid <<
On 2009/10/06 21:58:53, sky wrote:
> Does this really need to be a warning? At best it should be INFO.

Done.

http://codereview.chromium.org/251099/diff/103/5030#newcode46
Line 46: network->ethernet_ = service.state == chromeos::STATE_READY;
On 2009/10/06 21:58:53, sky wrote:
> Do we support being connected to both ethernet and wifi?

Yes

http://codereview.chromium.org/251099/diff/103/5030#newcode80
Line 80: DLOG(WARNING) << "Init " << service.ssid <<
On 2009/10/06 21:58:53, sky wrote:
> WARNING -> INFO

Done.

http://codereview.chromium.org/251099/diff/103/5030#newcode86
Line 86: if (service.type == chromeos::TYPE_ETHERNET) {
On 2009/10/06 21:58:53, sky wrote:
> Can this code be shared with NetworkStatusChangedHandler?

Done.

http://codereview.chromium.org/251099/diff/103/5030#newcode112
Line 112: chromeos::ServiceInfo service = service_status->services[i];
On 2009/10/06 21:58:53, sky wrote:
> This results in copying service. You should instead have service be a const &.

Done.

http://codereview.chromium.org/251099/diff/103/5030#newcode116
Line 116: service.encryption, service.signal_strength));
On 2009/10/06 21:58:53, sky wrote:
> nit: align with second (

Done.

http://codereview.chromium.org/251099/diff/103/5031
File chrome/browser/chromeos/cros_network_library.h (right):

http://codereview.chromium.org/251099/diff/103/5031#newcode36
Line 36: // Classes can add themselves as observers.
On 2009/10/06 21:58:53, sky wrote:
> You should document folks should get an instance from the CrosLibrary.

Done.

http://codereview.chromium.org/251099/diff/103/5031#newcode39
Line 39: class NetworkObserver {
On 2009/10/06 21:58:53, sky wrote:
> nit: as clients are already prefixing this with CrosNetworkLibrary::, you may
as
> well just call it Observer.

Done.

http://codereview.chromium.org/251099/diff/103/5031#newcode41
Line 41: virtual void NetworkChanged(std::string ssid,
On 2009/10/06 21:58:53, sky wrote:
> You should be consistent with other observers and push this state as getters
> into CrosNetworkLibrary and have NetworkChanged take only the
> CrosNetworkLibrary. And again, I wouldn't call NetworkChanged from
AddObserver.

Done.

http://codereview.chromium.org/251099/diff/103/5031#newcode48
Line 48: virtual ~CrosNetworkLibrary() {}
On 2009/10/06 21:58:53, sky wrote:
> Does this really need to be virtual?

Done.

http://codereview.chromium.org/251099/diff/103/5031#newcode69
Line 69: bool loaded_;
On 2009/10/06 21:58:53, sky wrote:
> Nuke this and instead call into CrosLibrary::loaded().

Done.

http://codereview.chromium.org/251099/diff/103/5032
File chrome/browser/chromeos/cros_power_library.cc (right):

http://codereview.chromium.org/251099/diff/103/5032#newcode9
Line 9: void CrosPowerLibrary::AddObserver(PowerObserver* observer) {
On 2009/10/06 21:58:53, sky wrote:
> nit: order of methods should match header.

Done.

http://codereview.chromium.org/251099/diff/103/5032#newcode28
Line 28: : loaded_(loaded) {
On 2009/10/06 21:58:53, sky wrote:
> You should initialize power_status_connection_, and PowerStatus doesn't have
an
> empty constructor, so you either need to add one to PowerStatus, or reset all
> the fields as is appropriate.

Is that really needed if I never access the object until it is initialized
below?

If so, I will make a note to fix the Cros shared library to provide an empty
constructor.

http://codereview.chromium.org/251099/diff/103/5032#newcode44
Line 44: DLOG(WARNING) << "Power" <<
On 2009/10/06 21:58:53, sky wrote:
> WARNING -> INFO

Done.

http://codereview.chromium.org/251099/diff/103/5029
File chrome/browser/chromeos/cros_power_library.h (right):

http://codereview.chromium.org/251099/diff/103/5029#newcode15
Line 15: class PowerObserver {
On 2009/10/06 21:58:53, sky wrote:
> nit: as clients are already prefixing this with CrosPowerLibrary::, you may as
> well just call it Observer.

Done.

http://codereview.chromium.org/251099/diff/103/5029#newcode17
Line 17: virtual void PowerChanged(bool line_power_on,
On 2009/10/06 21:58:53, sky wrote:
> Same comment as NetworkObserver: push getters into class, and have
PowerChanged
> take CrosPowerLibrary*.

Done.

http://codereview.chromium.org/251099/diff/103/5029#newcode20
Line 20: int64 battery_time_to_empty,
On 2009/10/06 21:58:53, sky wrote:
> Use base::TimeDelta for times.

Done.

http://codereview.chromium.org/251099/diff/103/5029#newcode25
Line 25: virtual ~CrosPowerLibrary();
On 2009/10/06 21:58:53, sky wrote:
> Do you really need this to be virtual?

Done.

http://codereview.chromium.org/251099/diff/103/5029#newcode37
Line 37: bool loaded_;
On 2009/10/06 21:58:53, sky wrote:
> Nuke this and instead call into CrosLibrary::loaded().

Done.

http://codereview.chromium.org/251099/diff/103/5018
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/103/5018#newcode102
Line 102: Singleton<CrosLibrary>()->GetNetworkLibrary()->
On 2009/10/06 21:58:53, sky wrote:
> Use of Singleton<Foo> is an implementation detail, instead you should have
> CrosLibrary::Get(), which internally does whatever it needs to.

Done.

http://codereview.chromium.org/251099/diff/103/5022
File chrome/browser/chromeos/network_menu_button.h (right):

http://codereview.chromium.org/251099/diff/103/5022#newcode101
Line 101: // The current connected (or connecting) wireless ssid.
On 2009/10/06 21:58:53, sky wrote:
> Is there a reason to cache these here rather than keeping them in
> CrosNetworkLibrary?

Done.

http://codereview.chromium.org/251099/diff/103/5019
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/103/5019#newcode64
Line 64: // The first item shows the percentage of battery left.
On 2009/10/06 21:58:53, sky wrote:
> As with the time button, these don't feel like these should be menus, rather
> tooltips or a bubble.

For power, I think we will eventually want a menuitem to configure whether or
not to show the percentage next to the icon and a menuitem that jumps to the
power section of the options dialog.

If you are really against these being menu buttons, lets talk in person and I
can show you how it looks right now and how it looks on my Mac.

http://codereview.chromium.org/251099/diff/103/5020
File chrome/browser/chromeos/power_menu_button.h (right):

http://codereview.chromium.org/251099/diff/103/5020#newcode17
Line 17: // It will also handle the status icon changing and connecting to
another
On 2009/10/06 21:58:53, sky wrote:
> nit: no will, it does.

Done.

http://codereview.chromium.org/251099/diff/103/5020#newcode76
Line 76: bool line_power_on_;
On 2009/10/06 21:58:53, sky wrote:
> Again, is there a reason to cache these here rather than getting them from the
> PowerLibrary?

Done.

http://codereview.chromium.org/251099/diff/103/5028
File views/controls/button/text_button.h (right):

http://codereview.chromium.org/251099/diff/103/5028#newcode97
Line 97: void SetFont(gfx::Font font);
On 2009/10/06 21:58:53, sky wrote:
> const gfx::Font&

Done.

[sky, 2009-10-07 14:32:42]

For the clock button could you convert it to using a tooltip now. If we end up
with wanting to have menu items that open  options and all that, then we can
convert it back.

I think a better option for both is a richer bubble type interface (as with the
bookmark bubble) that shows more information, perhaps with buttons that take you
to the options. But that's just my opinion, you should run these by Nicholas to
get his feedback.

http://codereview.chromium.org/251099/diff/12001/13008
File chrome/browser/chromeos/cros_library.h (right):

http://codereview.chromium.org/251099/diff/12001/13008#newcode17
Line 17: static bool loaded_;
Make the constructor private so no one creates a CrosLibrary and
DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/251099/diff/12001/13017
File chrome/browser/chromeos/cros_network_library.cc (right):

http://codereview.chromium.org/251099/diff/12001/13017#newcode18
Line 18: chromeos::MonitorNetworkStatus(&NetworkStatusChangedHandler, this);
Do you need to call an unmonitor or something from the destructor? While you
only create one instance for the life of the app, it's good practice to clean
up.

http://codereview.chromium.org/251099/diff/12001/13017#newcode56
Line 56: chromeos::ServiceInfo service = service_status->services[i];
const chromeos::ServiceInfo& service = ...

http://codereview.chromium.org/251099/diff/12001/13017#newcode150
Line 150: for (int i = 0; i < service_status->size; i++) {
nit: no {}

http://codereview.chromium.org/251099/diff/12001/13018
File chrome/browser/chromeos/cros_network_library.h (right):

http://codereview.chromium.org/251099/diff/12001/13018#newcode17
Line 17: WifiNetwork() : encrypted(false), strength(0) {}
You need to default initialize encryption here too (sorry for missing that
earlier).

http://codereview.chromium.org/251099/diff/12001/13018#newcode47
Line 47: CrosNetworkLibrary();
Make these private, you want folks to use Get.

http://codereview.chromium.org/251099/diff/12001/13018#newcode56
Line 56: bool ethernet_connected();
Make all these methods const, and you can inline them since they're just
returning a value.

http://codereview.chromium.org/251099/diff/12001/13018#newcode57
Line 57: std::string wifi_ssid();
const std::string&

http://codereview.chromium.org/251099/diff/12001/13016
File chrome/browser/chromeos/cros_power_library.h (right):

http://codereview.chromium.org/251099/diff/12001/13016#newcode23
Line 23: CrosPowerLibrary();
Make these private.

http://codereview.chromium.org/251099/diff/12001/13016#newcode32
Line 32: bool line_power_on();
Make all these const, and inline them.

http://codereview.chromium.org/251099/diff/12001/13006
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13006#newcode26
Line 26: power_menu_(this) {
Use the macro ALLOW_THIS_IN_INIALIAZER_LIST when using this in a member
initializer list.

http://codereview.chromium.org/251099/diff/12001/13006#newcode51
Line 51: int PowerMenuButton::GetItemCount() const {
You should override IsEnabledAt to return false for both items.

http://codereview.chromium.org/251099/diff/12001/13004
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/251099/diff/12001/13004#newcode16
Line 16: #include "chrome/browser/chromeos/cros_library.h"
Do you need the cros_library include?

http://codereview.chromium.org/251099/diff/12001/13004#newcode48
Line 48: WifiNetwork GetWifiNetworkAt(int index);
const WifiNetwork&

http://codereview.chromium.org/251099/diff/12001/13004#newcode69
Line 69: return wifi_networks_.empty() ?
Why is the first item an empty string if we there are networks?

[DaveMoore, 2009-10-07 16:21:34]

http://codereview.chromium.org/251099/diff/12001/13013
File chrome/browser/chromeos/clock_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13013#newcode50
Line 50: SetText(base::TimeFormatTimeOfDay(base::Time::Now()));
Cole was specific that he wanted the time formated as "3:23a". I believe that
TimeFormatTimeOfDay uses "am" and "pm".

http://codereview.chromium.org/251099/diff/12001/13005
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13005#newcode36
Line 36: if (!initialized) {
There's no reason to cache all the images in the object. The ResourceBundle does
it as well. Plus if these are themed images the resource bundle's cache will be
invalidated if the theme changes.

http://codereview.chromium.org/251099/diff/12001/13006
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13006#newcode31
Line 31: int discharging_index = IDR_STATUSBAR_BATTERY_DISCHARGING_1;
The resource bundle caches the icons, so there's no advantage in doing it here
as well, it just wastes memory. You can just look them up by id each time.

http://codereview.chromium.org/251099/diff/12001/13006#newcode59
Line 59: string16 PowerMenuButton::GetLabelAt(int index) const {
The time remaining works better as a tooltip. I'd rather see xx% (20 minutes to
charged) than have to click to see two lines. Has Cole been consulted?

http://codereview.chromium.org/251099/diff/12001/13006#newcode117
Line 117: CrosPowerLibrary* cros = CrosPowerLibrary::Get();
Since the PowerMenuButton mainly communicates with the CrosPowerLibrary it
should find out from it whether it should be displaying the unknown icon.

http://codereview.chromium.org/251099/diff/12001/13006#newcode124
Line 124: // is from 0 to 100, so we need to convert that to 0 to 7.
Comment shouldn't be "0 to 7". It should be "0 to kNumPowerImages - 1"

http://codereview.chromium.org/251099/diff/12001/13007
File chrome/browser/chromeos/power_menu_button.h (right):

http://codereview.chromium.org/251099/diff/12001/13007#newcode17
Line 17: // This class will handle getting the wifi powers and populating the
menu.
wifi powers?

[Charlie Lee (do not use), 2009-10-07 20:59:18]

Made fixes. I also added a change to not show the highlighted state for our menu
buttons as requested by Cole.

http://codereview.chromium.org/251099/diff/12001/13013
File chrome/browser/chromeos/clock_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13013#newcode50
Line 50: SetText(base::TimeFormatTimeOfDay(base::Time::Now()));
On 2009/10/07 16:21:34, davemoore wrote:
> Cole was specific that he wanted the time formated as "3:23a". I believe that
> TimeFormatTimeOfDay uses "am" and "pm".
> 

Done.

http://codereview.chromium.org/251099/diff/12001/13008
File chrome/browser/chromeos/cros_library.h (right):

http://codereview.chromium.org/251099/diff/12001/13008#newcode17
Line 17: static bool loaded_;
On 2009/10/07 14:32:42, sky wrote:
> Make the constructor private so no one creates a CrosLibrary and
> DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/251099/diff/12001/13017
File chrome/browser/chromeos/cros_network_library.cc (right):

http://codereview.chromium.org/251099/diff/12001/13017#newcode18
Line 18: chromeos::MonitorNetworkStatus(&NetworkStatusChangedHandler, this);
On 2009/10/07 14:32:42, sky wrote:
> Do you need to call an unmonitor or something from the destructor? While you
> only create one instance for the life of the app, it's good practice to clean
> up.

Ok, I will add it to the Cros API and then fix it up here.

http://codereview.chromium.org/251099/diff/12001/13017#newcode56
Line 56: chromeos::ServiceInfo service = service_status->services[i];
On 2009/10/07 14:32:42, sky wrote:
> const chromeos::ServiceInfo& service = ...

Done.

http://codereview.chromium.org/251099/diff/12001/13017#newcode150
Line 150: for (int i = 0; i < service_status->size; i++) {
On 2009/10/07 14:32:42, sky wrote:
> nit: no {}

Done.

http://codereview.chromium.org/251099/diff/12001/13018
File chrome/browser/chromeos/cros_network_library.h (right):

http://codereview.chromium.org/251099/diff/12001/13018#newcode17
Line 17: WifiNetwork() : encrypted(false), strength(0) {}
On 2009/10/07 14:32:42, sky wrote:
> You need to default initialize encryption here too (sorry for missing that
> earlier).

Done.

http://codereview.chromium.org/251099/diff/12001/13018#newcode47
Line 47: CrosNetworkLibrary();
On 2009/10/07 14:32:42, sky wrote:
> Make these private, you want folks to use Get.

Done.

http://codereview.chromium.org/251099/diff/12001/13018#newcode56
Line 56: bool ethernet_connected();
On 2009/10/07 14:32:42, sky wrote:
> Make all these methods const, and you can inline them since they're just
> returning a value.

Done.

http://codereview.chromium.org/251099/diff/12001/13018#newcode57
Line 57: std::string wifi_ssid();
On 2009/10/07 14:32:42, sky wrote:
> const std::string&

Done.

http://codereview.chromium.org/251099/diff/12001/13016
File chrome/browser/chromeos/cros_power_library.h (right):

http://codereview.chromium.org/251099/diff/12001/13016#newcode23
Line 23: CrosPowerLibrary();
On 2009/10/07 14:32:42, sky wrote:
> Make these private.

Done.

http://codereview.chromium.org/251099/diff/12001/13016#newcode32
Line 32: bool line_power_on();
On 2009/10/07 14:32:42, sky wrote:
> Make all these const, and inline them.

Done.

http://codereview.chromium.org/251099/diff/12001/13005
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13005#newcode36
Line 36: if (!initialized) {
On 2009/10/07 16:21:34, davemoore wrote:
> There's no reason to cache all the images in the object. The ResourceBundle
does
> it as well. Plus if these are themed images the resource bundle's cache will
be
> invalidated if the theme changes.

Done.

http://codereview.chromium.org/251099/diff/12001/13006
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/12001/13006#newcode26
Line 26: power_menu_(this) {
On 2009/10/07 14:32:42, sky wrote:
> Use the macro ALLOW_THIS_IN_INIALIAZER_LIST when using this in a member
> initializer list.

Done.

http://codereview.chromium.org/251099/diff/12001/13006#newcode31
Line 31: int discharging_index = IDR_STATUSBAR_BATTERY_DISCHARGING_1;
On 2009/10/07 16:21:34, davemoore wrote:
> The resource bundle caches the icons, so there's no advantage in doing it here
> as well, it just wastes memory. You can just look them up by id each time.

Done.

http://codereview.chromium.org/251099/diff/12001/13006#newcode51
Line 51: int PowerMenuButton::GetItemCount() const {
On 2009/10/07 14:32:42, sky wrote:
> You should override IsEnabledAt to return false for both items.

I already do that in the .h file. Is that a bad place to put simple
implementations like this?

http://codereview.chromium.org/251099/diff/12001/13006#newcode59
Line 59: string16 PowerMenuButton::GetLabelAt(int index) const {
On 2009/10/07 16:21:34, davemoore wrote:
> The time remaining works better as a tooltip. I'd rather see xx% (20 minutes
to
> charged) than have to click to see two lines. Has Cole been consulted?

Yeah, Cole thinks having the menu button is good. We will eventually put more
stuff in there.

http://codereview.chromium.org/251099/diff/12001/13006#newcode117
Line 117: CrosPowerLibrary* cros = CrosPowerLibrary::Get();
On 2009/10/07 16:21:34, davemoore wrote:
> Since the PowerMenuButton mainly communicates with the CrosPowerLibrary it
> should find out from it whether it should be displaying the unknown icon.

Done.

http://codereview.chromium.org/251099/diff/12001/13006#newcode124
Line 124: // is from 0 to 100, so we need to convert that to 0 to 7.
On 2009/10/07 16:21:34, davemoore wrote:
> Comment shouldn't be "0 to 7". It should be "0 to kNumPowerImages - 1"

Done.

http://codereview.chromium.org/251099/diff/12001/13007
File chrome/browser/chromeos/power_menu_button.h (right):

http://codereview.chromium.org/251099/diff/12001/13007#newcode17
Line 17: // This class will handle getting the wifi powers and populating the
menu.
On 2009/10/07 16:21:34, davemoore wrote:
> wifi powers?

Done.

http://codereview.chromium.org/251099/diff/12001/13004
File chrome/browser/chromeos/settings_contents_view.cc (right):

http://codereview.chromium.org/251099/diff/12001/13004#newcode16
Line 16: #include "chrome/browser/chromeos/cros_library.h"
On 2009/10/07 14:32:42, sky wrote:
> Do you need the cros_library include?

Done.

http://codereview.chromium.org/251099/diff/12001/13004#newcode48
Line 48: WifiNetwork GetWifiNetworkAt(int index);
On 2009/10/07 14:32:42, sky wrote:
> const WifiNetwork&

Done.

http://codereview.chromium.org/251099/diff/12001/13004#newcode69
Line 69: return wifi_networks_.empty() ?
On 2009/10/07 14:32:42, sky wrote:
> Why is the first item an empty string if we there are networks?

The empty string is selected if we are not connected to any network.

[sky, 2009-10-07 21:15:48]

Almost there.

http://codereview.chromium.org/251099/diff/17001/17014
File chrome/browser/chromeos/clock_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/17001/17014#newcode61
Line 61: now_exploded.hour < 12 ? L'p' : L'p');
Doesn't this result in p always?
Also, if you're going to use a format like this it should be localized.

http://codereview.chromium.org/251099/diff/17001/17019
File chrome/browser/chromeos/cros_network_library.h (right):

http://codereview.chromium.org/251099/diff/17001/17019#newcode56
Line 56: bool ethernet_connected() const { return ethernet_connected_;}
nit: space between ; and } for inlined methods like this.

[DaveMoore, 2009-10-07 21:33:01]

THere were also several lint errors that should be looked at.

http://codereview.chromium.org/251099/diff/17001/17017
File chrome/browser/chromeos/cros_power_library.h (right):

http://codereview.chromium.org/251099/diff/17001/17017#newcode16
Line 16: class CrosPowerLibrary {
It doesn't seem worth it to have a class that contains all the values from the
already existing status object. Any time we add a new behavior we'll have to
copy the values. Why don't you just make the PowerChanged() method take the
chromeos::PowerStatus object?

http://codereview.chromium.org/251099/diff/17001/17006
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/17001/17006#newcode156
Line 156: CrosNetworkLibrary* cros = CrosNetworkLibrary::Get();
Like PowerButton, this would be easier to read if you had just one call to
SetIcon() with the value of a local var.

http://codereview.chromium.org/251099/diff/17001/17007
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/17001/17007#newcode101
Line 101: if (!CrosPowerLibrary::loaded()) {
This would be a lot easier to read if you had a local var for the id of the
image and had just one call to SetIcon() at the bottom

[Charlie Lee (do not use), 2009-10-07 22:42:36]

Made the fixes. The lint errors are not real problems as far as I know.

http://codereview.chromium.org/251099/diff/17001/17014
File chrome/browser/chromeos/clock_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/17001/17014#newcode61
Line 61: now_exploded.hour < 12 ? L'p' : L'p');
On 2009/10/07 21:15:48, sky wrote:
> Doesn't this result in p always?
> Also, if you're going to use a format like this it should be localized.

You are right. I just copied this from the previous implementation. So I guess
it was wrong all along. I fixed it.

http://codereview.chromium.org/251099/diff/17001/17019
File chrome/browser/chromeos/cros_network_library.h (right):

http://codereview.chromium.org/251099/diff/17001/17019#newcode56
Line 56: bool ethernet_connected() const { return ethernet_connected_;}
On 2009/10/07 21:15:48, sky wrote:
> nit: space between ; and } for inlined methods like this.

Done.

http://codereview.chromium.org/251099/diff/17001/17017
File chrome/browser/chromeos/cros_power_library.h (right):

http://codereview.chromium.org/251099/diff/17001/17017#newcode16
Line 16: class CrosPowerLibrary {
On 2009/10/07 21:33:02, davemoore wrote:
> It doesn't seem worth it to have a class that contains all the values from the
> already existing status object. Any time we add a new behavior we'll have to
> copy the values. Why don't you just make the PowerChanged() method take the
> chromeos::PowerStatus object?

Done.

http://codereview.chromium.org/251099/diff/17001/17006
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/17001/17006#newcode156
Line 156: CrosNetworkLibrary* cros = CrosNetworkLibrary::Get();
On 2009/10/07 21:33:02, davemoore wrote:
> Like PowerButton, this would be easier to read if you had just one call to
> SetIcon() with the value of a local var.

Done.

http://codereview.chromium.org/251099/diff/17001/17007
File chrome/browser/chromeos/power_menu_button.cc (right):

http://codereview.chromium.org/251099/diff/17001/17007#newcode101
Line 101: if (!CrosPowerLibrary::loaded()) {
On 2009/10/07 21:33:02, davemoore wrote:
> This would be a lot easier to read if you had a local var for the id of the
> image and had just one call to SetIcon() at the bottom

Done.

[sky, 2009-10-07 22:51:45]

LGTM