Fix up status area clock code.

Fix up status area clock code.

This fixes a straightforward crash from the clock menu, and also includes some code cleanup done while investigating (see inline comments).

BUG=106947
TEST=Test the clock display and menu in the status area, including changing 24 hour and timezone preferences. Also, see issue.


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

[stevenjb, 2011-12-13 02:17:41]

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/login/b...
File chrome/browser/chromeos/login/background_view.h (left):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/login/b...
chrome/browser/chromeos/login/background_view.h:92: void
SetDefaultUse24HourClock(bool use_24hour_clock);
This function had no effect and is unnecessary.

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.cc (left):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.cc:54:
registrar_.Add(prefs::kUse24HourClock, this);
This was not actually doing anything, since it only observed the pre-login
default profile.

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.cc:93: Profile* profile =
ProfileManager::GetDefaultProfile();
ProfileManager::GetDefaultProfile() will actually create a new Profile if
necessary. Doing this from a function called on a timer is very awkward for
debugging, and might possibly be responsible for some bizarre edge cases?
Regardless, it seems like a bad idea.

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.cc:124: Profile* profile =
ProfileManager::GetDefaultProfile();
This will only ever get called after the logged in profile has been created.

[oshima, 2011-12-13 06:06:21]

a few minor style comments.

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.cc:58: #if defined(OS_CHROMEOS)
 // See note at top of file
do we need this ifdef? This file is chromeos only anyway, isn't it?

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.h (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.h:36: public:
remove public:

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/status_area_button.h:77: virtual void
SetMenuActive(bool active);
Are you going to use this virtual in separate CL? Will it make sense to document
this as (I assume) it'll do more than just setting menu_active_?

[stevenjb, 2011-12-14 02:19:21]

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.cc:58: #if defined(OS_CHROMEOS)
 // See note at top of file
On 2011/12/13 06:06:21, oshima wrote:
> do we need this ifdef? This file is chromeos only anyway, isn't it?
"See note at top of file" :)

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.h (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.h:36: public:
On 2011/12/13 06:06:21, oshima wrote:
> remove public:
Oops. Done.

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/status_area_button.h:77: virtual void
SetMenuActive(bool active);
On 2011/12/13 06:06:21, oshima wrote:
> Are you going to use this virtual in separate CL? Will it make sense to
document
> this as (I assume) it'll do more than just setting menu_active_?

I'm overriding it in ClockMenuButton. I'll add a comment and make this protected
since it is only called internally.

[oshima, 2011-12-14 16:57:03]

LGTM

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/8930001/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/clock_menu_button.cc:58: #if defined(OS_CHROMEOS)
 // See note at top of file
On 2011/12/14 02:19:21, stevenjb wrote:
> On 2011/12/13 06:06:21, oshima wrote:
> > do we need this ifdef? This file is chromeos only anyway, isn't it?
> "See note at top of file" :)
> 

oops sorry about that.

[commit-bot: I haz the power, 2011-12-14 20:14:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/8930001/10001

[commit-bot: I haz the power, 2011-12-14 21:21:40]

Change committed as 114492

[Nikita (slow), 2011-12-26 10:48:38]

lgtm

http://codereview.chromium.org/8930001/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/status_area_host_aura.cc (right):

http://codereview.chromium.org/8930001/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/status_area_host_aura.cc:109: browser =
Browser::Create(profile);
This will happen when there're no browser windows open?
Is this even possible?