Extension alarms now persist in Preferences.

chrome/browser/extensions/api/alarms/alarm_manager.cc

Index: chrome/browser/extensions/api/alarms/alarm_manager.cc
diff --git a/chrome/browser/extensions/api/alarms/alarm_manager.cc b/chrome/browser/extensions/api/alarms/alarm_manager.cc
index 17a48898c3b2317fc039120069fcbe6af7426cbb..b7621048cce45429a1762b122b18050585b9a0e2 100644
--- a/chrome/browser/extensions/api/alarms/alarm_manager.cc
+++ b/chrome/browser/extensions/api/alarms/alarm_manager.cc
@@ -9,8 +9,12 @@
 #include "base/message_loop.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_event_router.h"
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_service.h"
 
 namespace extensions {
 
@@ -39,9 +43,22 @@ class DefaultAlarmDelegate : public AlarmManager::Delegate {
 
 }
 
+struct AlarmManager::TimerInfo {
+  base::Timer timer;
+  base::Time scheduled_run_time;

[Aaron Boodman, 2012/04/20 17:58:22]

I don't understand why you introduced this struct. The only thing it adds is
scheduled_run_time, which is only used once and can easily be computed at that
time. And keeping scheduled_run_time up to date requires bookkeeping that could
be avoided if it was computed when needed.

[Matt Perry, 2012/04/20 19:38:49]

It's used when persisting to prefs. Each time we persist, we need to know the
next time the alarm is going to fire. That info is not available for alarms that
already exist.

I could avoid this if I changed the prefs API to have Add/RemoveAlarm instead of
SetAllAlarms, but I tried that and it just ends up transferring the complexity
to ExtensionPrefs. I think it's cleaner and less error-prone this way.

[Aaron Boodman, 2012/04/20 23:15:24]

I see.

+
+  explicit TimerInfo(bool repeating) : timer(true, repeating) {}
+
+  void UpdateScheduledRunTime() {
+    scheduled_run_time = base::Time::Now() + timer.GetCurrentDelay();
+  }
+};
+
 AlarmManager::AlarmManager(Profile* profile)
     : profile_(profile),
       delegate_(new DefaultAlarmDelegate(profile)) {
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
+                 content::Source<Profile>(profile_));
 }
 
 AlarmManager::~AlarmManager() {
@@ -49,21 +66,9 @@ AlarmManager::~AlarmManager() {
 
 void AlarmManager::AddAlarm(const std::string& extension_id,
                             const linked_ptr<Alarm>& alarm) {
-  // TODO(mpcomplete): Better handling of granularity.
-  // http://crbug.com/122683
-
-  // Override any old alarm with the same name.
-  AlarmIterator old_alarm = GetAlarmIterator(extension_id, alarm->name);
-  if (old_alarm.first != alarms_.end())
-    RemoveAlarmIterator(old_alarm);
-
-  alarms_[extension_id].push_back(alarm);
-  base::Timer* timer = new base::Timer(true, alarm->repeating);
-  timers_[alarm.get()] = make_linked_ptr(timer);
-  timer->Start(FROM_HERE,
-      base::TimeDelta::FromSeconds(alarm->delay_in_seconds),
-      base::Bind(&AlarmManager::OnAlarm, base::Unretained(this),
-                 extension_id, alarm->name));
+  AddAlarmImpl(extension_id, alarm,
+               base::TimeDelta::FromSeconds(alarm->delay_in_seconds));
+  WriteToPrefs(extension_id);
 }
 
 const AlarmManager::Alarm* AlarmManager::GetAlarm(
@@ -102,7 +107,9 @@ bool AlarmManager::RemoveAlarm(const std::string& extension_id,
   AlarmIterator it = GetAlarmIterator(extension_id, name);
   if (it.first == alarms_.end())
     return false;
+
   RemoveAlarmIterator(it);
+  WriteToPrefs(extension_id);
   return true;
 }
 
@@ -117,11 +124,12 @@ void AlarmManager::RemoveAllAlarms(const std::string& extension_id) {
     RemoveAlarmIterator(AlarmIterator(list, list->second.begin()));
 
   CHECK(alarms_.find(extension_id) == alarms_.end());
+  WriteToPrefs(extension_id);
 }
 
 void AlarmManager::RemoveAlarmIterator(const AlarmIterator& iter) {
   // Cancel the timer first.
-  timers_[iter.second->get()]->Stop();
+  timers_[iter.second->get()]->timer.Stop();
   timers_.erase(iter.second->get());
 
   // Clean up our alarm list.
@@ -135,10 +143,105 @@ void AlarmManager::OnAlarm(const std::string& extension_id,
                            const std::string& name) {
   AlarmIterator it = GetAlarmIterator(extension_id, name);
   CHECK(it.first != alarms_.end());
-  delegate_->OnAlarm(extension_id, *it.second->get());
+  const Alarm* alarm = it.second->get();
+  delegate_->OnAlarm(extension_id, *alarm);
 
-  if (!(*it.second)->repeating)
+  if (!alarm->repeating) {
     RemoveAlarmIterator(it);
+  } else {
+    // Restart the timer, since it may have been set with a shorter delay.
+    TimerInfo* timer = timers_[alarm].get();
+    timer->timer.Start(FROM_HERE,
+        base::TimeDelta::FromSeconds(alarm->delay_in_seconds),
+        base::Bind(&AlarmManager::OnAlarm, base::Unretained(this),
+                   extension_id, alarm->name));
+    timer->UpdateScheduledRunTime();
+  }
+
+  WriteToPrefs(extension_id);
+}
+
+void AlarmManager::AddAlarmImpl(const std::string& extension_id,
+                                const linked_ptr<Alarm>& alarm,
+                                base::TimeDelta timer_delay) {
+  // Override any old alarm with the same name.
+  AlarmIterator old_alarm = GetAlarmIterator(extension_id, alarm->name);
+  if (old_alarm.first != alarms_.end())
+    RemoveAlarmIterator(old_alarm);
+
+  alarms_[extension_id].push_back(alarm);
+
+  TimerInfo* timer = new TimerInfo(alarm->repeating);

[Aaron Boodman, 2012/04/20 17:58:22]

Nit: Maybe name this timer_info so you don't have timer-timer-> ?

+  timers_[alarm.get()] = make_linked_ptr(timer);
+  timer->timer.Start(FROM_HERE,
+      timer_delay,
+      base::Bind(&AlarmManager::OnAlarm, base::Unretained(this),
+                 extension_id, alarm->name));
+  timer->UpdateScheduledRunTime();
+}
+
+void AlarmManager::WriteToPrefs(const std::string& extension_id) {
+  ExtensionService* service =
+      ExtensionSystem::Get(profile_)->extension_service();

[Aaron Boodman, 2012/04/20 17:58:22]

Consider passing in ExtensionPrefs to make this more easily testable.

[Matt Perry, 2012/04/20 19:38:49]

I don't think that's necessary. TestExtensionSystem already has a facility for
creating a mock ExtensionService/ExtensionPrefs duo.

[Aaron Boodman, 2012/04/20 23:15:24]

It's not necessary, but it seems nice to keep the class's external dependencies
as low as possible. It doesn't actually need ExtensionService, only
ExtensionPrefs.

[Matt Perry, 2012/04/20 23:39:42]

Yeah, I see what you're saying, but I'm not a fan of that style. We have the
Profile to represent the environment, and anything we want mocked we can do by
mocking the Profile-centric objects. Passing dependencies piecemeal irks me.

[Aaron Boodman, 2012/04/20 23:50:12]

That is really odd to me. Creating a mock profile and configuring it correctly
so that it has all the bits setup that this class will end up relying on is a
lot more mental overhead than the class just declaring (in the form of its
constructor params) its dependencies on the test creating them and passing them
in.

But I don't feel super strongly about it.

+  if (!service || !service->extension_prefs())
+    return;
+
+  std::vector<AlarmPref> alarm_prefs;
+
+  AlarmMap::iterator list = alarms_.find(extension_id);

[Aaron Boodman, 2012/04/20 17:58:22]

This reminds me: We should limit extensions to some total number of alarms, just
so that they don't fill up the preferences. Maybe 3?

[Matt Perry, 2012/04/20 19:38:49]

Are you worried about malicious extensions carpet-bombing users with alarms?

[Aaron Boodman, 2012/04/20 23:15:24]

I'm was thinking about buggy extensions filling up the preferences file with
tons of alarm registrations.

[Matt Perry, 2012/04/20 23:39:42]

They would each have to have a different name. I suppose we can put a limit, but
I'd rather it be really high (like 100).

[Aaron Boodman, 2012/04/20 23:50:12]

Hm, ok. Let's talk about this more next week. Different bug no matter what.

+  if (list != alarms_.end()) {
+    for (AlarmList::iterator it = list->second.begin();
+         it != list->second.end(); ++it) {
+      TimerInfo* timer = timers_[it->get()].get();
+      AlarmPref pref;
+      pref.alarm = *it;
+      pref.scheduled_run_time = timer->scheduled_run_time;
+      alarm_prefs.push_back(pref);
+    }
+  }
+
+  service->extension_prefs()->SetRegisteredAlarms(extension_id, alarm_prefs);
+}
+
+void AlarmManager::ReadFromPrefs(const std::string& extension_id) {
+  ExtensionService* service =
+      ExtensionSystem::Get(profile_)->extension_service();
+  if (!service || !service->extension_prefs())
+    return;
+
+  std::vector<AlarmPref> alarm_prefs =
+      service->extension_prefs()->GetRegisteredAlarms(extension_id);
+  for (size_t i = 0; i < alarm_prefs.size(); ++i) {
+    base::TimeDelta delay =
+        alarm_prefs[i].scheduled_run_time - base::Time::Now();
+    if (delay < base::TimeDelta::FromSeconds(0))
+      delay = base::TimeDelta::FromSeconds(0);
+
+    AddAlarmImpl(extension_id, alarm_prefs[i].alarm, delay);
+  }
+}
+
+void AlarmManager::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_LOADED: {
+      const Extension* extension =
+          content::Details<const Extension>(details).ptr();
+      ReadFromPrefs(extension->id());
+      break;
+    }
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
+AlarmPref::AlarmPref() {
+}
+
+AlarmPref::~AlarmPref() {
 }
 
 }  // namespace extensions