Extension alarms now persist in Preferences.

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

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/extensions/api/alarms/alarm_manager.h"
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #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 {
 
 namespace {
 
 const char kOnAlarmEvent[] = "experimental.alarms.onAlarm";
 
 class DefaultAlarmDelegate : public AlarmManager::Delegate {
  public:
   explicit DefaultAlarmDelegate(Profile* profile) : profile_(profile) {}
   virtual ~DefaultAlarmDelegate() {}
 
   virtual void OnAlarm(const std::string& extension_id,
                        const AlarmManager::Alarm& alarm) {
     ListValue args;
     std::string json_args;
     args.Append(alarm.ToValue().release());
     base::JSONWriter::Write(&args, &json_args);
     ExtensionSystem::Get(profile_)->event_router()->DispatchEventToExtension(
         extension_id, kOnAlarmEvent, json_args, NULL, GURL());
   }
 
  private:
   Profile* profile_;
 };
 
 }
 
+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() {
 }
 
 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(
     const std::string& extension_id, const std::string& name) {
   AlarmIterator it = GetAlarmIterator(extension_id, name);
   if (it.first == alarms_.end())
     return NULL;
   return it.second->get();
 }
 
@@ skipping 18 matching lines @@
   }
 
   return make_pair(alarms_.end(), AlarmList::iterator());
 }
 
 bool AlarmManager::RemoveAlarm(const std::string& extension_id,
                                const std::string& name) {
   AlarmIterator it = GetAlarmIterator(extension_id, name);
   if (it.first == alarms_.end())
     return false;
+
   RemoveAlarmIterator(it);
+  WriteToPrefs(extension_id);
   return true;
 }
 
 void AlarmManager::RemoveAllAlarms(const std::string& extension_id) {
   AlarmMap::iterator list = alarms_.find(extension_id);
   if (list == alarms_.end())
     return;
 
   // Note: I'm using indices rather than iterators here because
   // RemoveAlarmIterator will delete the list when it becomes empty.
   for (size_t i = 0, size = list->second.size(); i < size; ++i)
     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.
   AlarmList& list = iter.first->second;
   list.erase(iter.second);
   if (list.empty())
     alarms_.erase(iter.first);
 }
 
 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