Fix up status area clock code.

chrome/browser/chromeos/status/clock_menu_button.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Note: this file is used by Aura on all platforms, even though it is currently
 // in a chromeos specific location.
 
 #include "chrome/browser/chromeos/status/clock_menu_button.h"
 
 #include "base/i18n/time_formatting.h"
@@ skipping 25 matching lines @@
 };
 
 }  // namespace
 
 // Amount of slop to add into the timer to make sure we're into the next minute
 // when the timer goes off.
 const int kTimerSlopSeconds = 1;
 
 ClockMenuButton::ClockMenuButton(StatusAreaButton::Delegate* delegate)
     : StatusAreaButton(delegate, this),
-      default_use_24hour_clock_(false) {
+      pref_service_(NULL),
+      use_24hour_clock_(false) {
   set_id(VIEW_ID_STATUS_BUTTON_CLOCK);
-
-#if defined(OS_CHROMEOS)  // See note at top of file
-  // Start monitoring the kUse24HourClock preference.
-  Profile* profile = ProfileManager::GetDefaultProfile();
-  if (profile) {  // This can be NULL in the login screen.
-    registrar_.Init(profile->GetPrefs());
-    registrar_.Add(prefs::kUse24HourClock, this);

[stevenjb, 2011/12/13 02:17:42]

This was not actually doing anything, since it only observed the pre-login
default profile.

-  }
-#endif
+  UpdateProfile();
   UpdateTextAndSetNextTimer();
 }
 
 ClockMenuButton::~ClockMenuButton() {
   timer_.Stop();
 }
 
+void ClockMenuButton::UpdateProfile() {
+#if defined(OS_CHROMEOS)  // See note at top of file

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

do we need this ifdef? This file is chromeos only anyway, isn't it?

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

"See note at top of file" :)

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

oops sorry about that.

+  // Start monitoring the kUse24HourClock preference.
+  Profile* profile = ProfileManager::GetDefaultProfile();
+  if (profile && profile->GetPrefs() != pref_service_) {
+    pref_service_ = profile->GetPrefs();
+    use_24hour_clock_ = pref_service_->GetBoolean(prefs::kUse24HourClock);
+    registrar_.reset(new PrefChangeRegistrar);
+    registrar_->Init(pref_service_);
+    registrar_->Add(prefs::kUse24HourClock, this);
+    UpdateText();
+  }
+#endif
+}
+
 void ClockMenuButton::UpdateTextAndSetNextTimer() {
   UpdateText();
 
   // Try to set the timer to go off at the next change of the minute. We don't
   // want to have the timer go off more than necessary since that will cause
   // the CPU to wake up and consume power.
   base::Time now = base::Time::Now();
   base::Time::Exploded exploded;
   now.LocalExplode(&exploded);
 
   // Often this will be called at minute boundaries, and we'll actually want
   // 60 seconds from now.
   int seconds_left = 60 - exploded.second;
   if (seconds_left == 0)
     seconds_left = 60;
 
   // Make sure that the timer fires on the next minute. Without this, if it is
   // called just a teeny bit early, then it will skip the next minute.
   seconds_left += kTimerSlopSeconds;
 
   timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(seconds_left), this,
                &ClockMenuButton::UpdateTextAndSetNextTimer);
 }
 
 void ClockMenuButton::UpdateText() {
   base::Time time(base::Time::Now());
-  bool use_24hour_clock = default_use_24hour_clock_;
-#if defined(OS_CHROMEOS)  // See note at top of file
-  // If the profie is present, check the use 24-hour clock preference.
-  Profile* profile = ProfileManager::GetDefaultProfile();

[stevenjb, 2011/12/13 02:17:42]

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.

-  if (profile)
-    use_24hour_clock = profile->GetPrefs()->GetBoolean(prefs::kUse24HourClock);
-#endif
   SetText(base::TimeFormatTimeOfDayWithHourClockType(
       time,
-      use_24hour_clock ? base::k24HourClock : base::k12HourClock,
+      use_24hour_clock_ ? base::k24HourClock : base::k12HourClock,
       base::kDropAmPm));
-  SetTooltipText(base::TimeFormatFriendlyDateAndTime(time));
-  SetAccessibleName(base::TimeFormatFriendlyDateAndTime(time));
+  string16 friendly_time_string(base::TimeFormatFriendlyDateAndTime(time));
+  SetTooltipText(friendly_time_string);
+  SetAccessibleName(friendly_time_string);
   SchedulePaint();
 }
 
-void ClockMenuButton::SetDefaultUse24HourClock(bool use_24hour_clock) {
-  if (default_use_24hour_clock_ == use_24hour_clock)
+void ClockMenuButton::SetUse24HourClock(bool use_24hour_clock) {
+  if (use_24hour_clock_ == use_24hour_clock)
     return;
-
-  default_use_24hour_clock_ = use_24hour_clock;
+  use_24hour_clock_ = use_24hour_clock;
   UpdateText();
 }
 
 // ClockMenuButton, content::NotificationObserver implementation:
 
 void ClockMenuButton::Observe(int type,
                               const content::NotificationSource& source,
                               const content::NotificationDetails& details) {
 #if defined(OS_CHROMEOS)  // See note at top of file
   if (type == chrome::NOTIFICATION_PREF_CHANGED) {
     std::string* pref_name = content::Details<std::string>(details).ptr();
     if (*pref_name == prefs::kUse24HourClock) {
-      UpdateText();
+      Profile* profile = ProfileManager::GetDefaultProfile();

[stevenjb, 2011/12/13 02:17:42]

This will only ever get called after the logged in profile has been created.

+      if (profile) {
+        SetUse24HourClock(
+            profile->GetPrefs()->GetBoolean(prefs::kUse24HourClock));
+      }
     }
   }
 #endif
 }
 
 // ClockMenuButton, views::MenuDelegate implementation:
 string16 ClockMenuButton::GetLabel(int id) const {
   DCHECK_EQ(CLOCK_DISPLAY_ITEM, id);
   return base::TimeFormatFriendlyDate(base::Time::Now());
 }
 
 bool ClockMenuButton::IsCommandEnabled(int id) const {
   DCHECK(id == CLOCK_DISPLAY_ITEM || id == CLOCK_OPEN_OPTIONS_ITEM);
   return id == CLOCK_OPEN_OPTIONS_ITEM;
 }
 
 void ClockMenuButton::ExecuteCommand(int id) {
   DCHECK_EQ(CLOCK_OPEN_OPTIONS_ITEM, id);
   delegate()->ExecuteStatusAreaCommand(
       this, StatusAreaButton::Delegate::SHOW_SYSTEM_OPTIONS);
 }
 
+// StatusAreaButton implementation
+void ClockMenuButton::SetMenuActive(bool active) {
+  // Activation gets updated when we change login state, so profile may change.
+  UpdateProfile();
+  StatusAreaButton::SetMenuActive(active);
+}
+
 int ClockMenuButton::horizontal_padding() {
   return 3;
 }
 
 // ClockMenuButton, views::ViewMenuDelegate implementation:
 
 void ClockMenuButton::RunMenu(views::View* source, const gfx::Point& pt) {
   // View passed in must be a views::MenuButton, i.e. the ClockMenuButton.
   DCHECK_EQ(source, this);
 
@@ skipping 30 matching lines @@
   if (delegate()->ShouldExecuteStatusAreaCommand(
           this, StatusAreaButton::Delegate::SHOW_SYSTEM_OPTIONS)) {
     menu->AppendSeparator();
 
     const string16 clock_open_options_label =
         l10n_util::GetStringUTF16(IDS_STATUSBAR_CLOCK_OPEN_OPTIONS_DIALOG);
     menu->AppendMenuItemWithLabel(CLOCK_OPEN_OPTIONS_ITEM,
                                   clock_open_options_label);
   }
 }