Refactor chrome.alarms interface to support absolute alarm deadlines.

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 2861a036ffd0857ca6eb47db6a1d7ddd4d73e5b7..020c63ad2a60fe70a1132b9ba8968e2381279c6a 100644
--- a/chrome/browser/extensions/api/alarms/alarm_manager.cc
+++ b/chrome/browser/extensions/api/alarms/alarm_manager.cc
@@ -32,10 +32,10 @@ class DefaultAlarmDelegate : public AlarmManager::Delegate {
   virtual ~DefaultAlarmDelegate() {}
 
   virtual void OnAlarm(const std::string& extension_id,
-                       const AlarmManager::Alarm& alarm) {
+                       const Alarm& alarm) {
     ListValue args;
     std::string json_args;
-    args.Append(alarm.ToValue().release());
+    args.Append(alarm.js_alarm->ToValue().release());
     base::JSONWriter::Write(&args, &json_args);
     ExtensionSystem::Get(profile_)->event_router()->DispatchEventToExtension(
         extension_id, kOnAlarmEvent, json_args, NULL, GURL());
@@ -55,8 +55,9 @@ base::TimeDelta TimeDeltaFromDelay(double delay_in_minutes) {
 
 // AlarmManager
 
-AlarmManager::AlarmManager(Profile* profile)
+AlarmManager::AlarmManager(Profile* profile, TimeProvider now)
     : profile_(profile),
+      now_(now),
       delegate_(new DefaultAlarmDelegate(profile)),
       last_poll_time_(base::Time()) {
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
@@ -67,18 +68,17 @@ AlarmManager::~AlarmManager() {
 }
 
 void AlarmManager::AddAlarm(const std::string& extension_id,
-                            const linked_ptr<Alarm>& alarm) {
-  base::TimeDelta alarm_time = TimeDeltaFromDelay(alarm->delay_in_minutes);
-  AddAlarmImpl(extension_id, alarm, alarm_time);
+                            const Alarm& alarm) {
+  AddAlarmImpl(extension_id, alarm);
   WriteToPrefs(extension_id);
 }
 
-const AlarmManager::Alarm* AlarmManager::GetAlarm(
+const 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();
+  return &*it.second;
 }
 
 const AlarmManager::AlarmList* AlarmManager::GetAllAlarms(
@@ -97,7 +97,7 @@ AlarmManager::AlarmIterator AlarmManager::GetAlarmIterator(
 
   for (AlarmList::iterator it = list->second.begin();
        it != list->second.end(); ++it) {
-    if ((*it)->name == name)
+    if (it->js_alarm->name == name)
       return make_pair(list, it);
   }
 
@@ -130,51 +130,45 @@ void AlarmManager::RemoveAllAlarms(const std::string& extension_id) {
 }
 
 void AlarmManager::RemoveAlarmIterator(const AlarmIterator& iter) {
-  // Cancel the timer if there are no more alarms.
-  // We don't need to reschedule the poll otherwise, because in
-  // the worst case we would just poll one extra time.
-  scheduled_times_.erase(iter.second->get());
-  if (scheduled_times_.empty())
-    timer_.Stop();
-
-  // Clean up our alarm list.
   AlarmList& list = iter.first->second;
   list.erase(iter.second);
   if (list.empty())
     alarms_.erase(iter.first);
+
+  // Cancel the timer if there are no more alarms.
+  // We don't need to reschedule the poll otherwise, because in
+  // the worst case we would just poll one extra time.
+  if (alarms_.empty())
+    timer_.Stop();
 }
 
-void AlarmManager::OnAlarm(const std::string& extension_id,
-                           const std::string& name) {
-  AlarmIterator it = GetAlarmIterator(extension_id, name);
+void AlarmManager::OnAlarm(AlarmIterator it) {
   CHECK(it.first != alarms_.end());
-  const Alarm* alarm = it.second->get();
-  delegate_->OnAlarm(extension_id, *alarm);
-
-  std::string extension_id_copy(extension_id);
-  if (!alarm->repeating) {
-    RemoveAlarmIterator(it);
+  Alarm& alarm = *it.second;
+  std::string extension_id_copy(it.first->first);
+  delegate_->OnAlarm(extension_id_copy, alarm);
+
+  // Update our scheduled time for the next alarm.
+  if (double* period_in_minutes =

[Matt Perry, 2012/06/11 22:32:44]

I always thought our style guide banned this, but I now see it's explicitly
allowed. Cool.

+      alarm.js_alarm->period_in_minutes.get()) {
+    alarm.js_alarm->scheduled_time =
+        (last_poll_time_ +
+         TimeDeltaFromDelay(*period_in_minutes)).ToJsTime();
   } else {
-    // Update our scheduled time for the next alarm.
-    scheduled_times_[alarm].time =
-        last_poll_time_ + TimeDeltaFromDelay(alarm->delay_in_minutes);
+    RemoveAlarmIterator(it);
   }
   WriteToPrefs(extension_id_copy);
 }
 
 void AlarmManager::AddAlarmImpl(const std::string& extension_id,
-                                const linked_ptr<Alarm>& alarm,
-                                base::TimeDelta time_delay) {
+                                const Alarm& alarm) {
   // Override any old alarm with the same name.
-  AlarmIterator old_alarm = GetAlarmIterator(extension_id, alarm->name);
+  AlarmIterator old_alarm = GetAlarmIterator(extension_id,
+                                             alarm.js_alarm->name);
   if (old_alarm.first != alarms_.end())
     RemoveAlarmIterator(old_alarm);
 
   alarms_[extension_id].push_back(alarm);
-  AlarmRuntimeInfo info;
-  info.extension_id = extension_id;
-  info.time = base::Time::Now() + time_delay;
-  scheduled_times_[alarm.get()] = info;
 
   // TODO(yoz): Is 0 really sane? There could be thrashing.
   ScheduleNextPoll(base::TimeDelta::FromMinutes(0));
@@ -186,16 +180,13 @@ void AlarmManager::WriteToPrefs(const std::string& extension_id) {
   if (!service || !service->extension_prefs())
     return;
 
-  std::vector<AlarmPref> alarm_prefs;
+  std::vector<Alarm> alarm_prefs;
 
   AlarmMap::iterator list = alarms_.find(extension_id);
   if (list != alarms_.end()) {
     for (AlarmList::iterator it = list->second.begin();
          it != list->second.end(); ++it) {
-      AlarmPref pref;
-      pref.alarm = *it;
-      pref.scheduled_run_time = scheduled_times_[it->get()].time;
-      alarm_prefs.push_back(pref);
+      alarm_prefs.push_back(*it);
     }
   }
 
@@ -208,21 +199,16 @@ void AlarmManager::ReadFromPrefs(const std::string& extension_id) {
   if (!service || !service->extension_prefs())
     return;
 
-  std::vector<AlarmPref> alarm_prefs =
+  std::vector<Alarm> 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);
+    AddAlarmImpl(extension_id, alarm_prefs[i]);
   }
 }
 
 void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
   // 0. If there are no alarms, stop the timer.
-  if (scheduled_times_.empty()) {
+  if (alarms_.empty()) {
     timer_.Stop();
     return;
   }
@@ -233,13 +219,20 @@ void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
   base::Time next_poll(last_poll_time_ + min_period);
 
   // Find the soonest alarm that is scheduled to run.
-  AlarmRuntimeInfoMap::iterator min_it = scheduled_times_.begin();
-  for (AlarmRuntimeInfoMap::iterator it = min_it;
-           it != scheduled_times_.end(); ++it) {
-    if (it->second.time < min_it->second.time)
-      min_it = it;
+  // alarms_ guarantees that none of its contained lists are empty.
+  base::Time soonest_alarm_time = base::Time::FromJsTime(
+      alarms_.begin()->second.begin()->js_alarm->scheduled_time);
+  for (AlarmMap::const_iterator m_it = alarms_.begin(), m_end = alarms_.end();
+       m_it != m_end; ++m_it) {
+    for (AlarmList::const_iterator
+             l_it = m_it->second.begin(), l_end = m_it->second.end();

[Matt Perry, 2012/06/11 22:32:44]

nit: getting rid of l_end would actually require less lines, since you wouldn't
need to wrap here.

[Jeffrey Yasskin, 2012/06/12 19:12:31]

True. I got into this habit in LLVM, where they point out that it's actually a
bit faster as soon as the compiler can't analyze any piece of code inside the
loop. I've removed it here.

+         l_it != l_end; ++l_it) {
+      base::Time cur_alarm_time =
+          base::Time::FromJsTime(l_it->js_alarm->scheduled_time);
+      if (cur_alarm_time < soonest_alarm_time)
+        soonest_alarm_time = cur_alarm_time;
+    }
   }
-  base::Time soonest_alarm_time(min_it->second.time);
 
   // If the next alarm is more than min_period in the future, wait for it.
   // Otherwise, only poll as often as min_period.
@@ -250,7 +243,7 @@ void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
   // Schedule the poll.
   next_poll_time_ = next_poll;
   base::TimeDelta delay = std::max(base::TimeDelta::FromSeconds(0),
-                                   next_poll - base::Time::Now());
+                                   next_poll - now_());
   timer_.Start(FROM_HERE,
                delay,
                this,
@@ -258,27 +251,44 @@ void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
 }
 
 void AlarmManager::PollAlarms() {
-  last_poll_time_ = base::Time::Now();
-
-  // Run any alarms scheduled in the past. Note that we could remove alarms
-  // during iteration if they are non-repeating.
-  AlarmRuntimeInfoMap::iterator iter = scheduled_times_.begin();
-  while (iter != scheduled_times_.end()) {
-    AlarmRuntimeInfoMap::iterator it = iter;
-    ++iter;
-    if (it->second.time <= next_poll_time_) {
-      OnAlarm(it->second.extension_id, it->first->name);
+  last_poll_time_ = now_();
+
+  // Run any alarms scheduled in the past. OnAlarm uses vector::erase to remove
+  // elements from the AlarmList, and map::erase to remove AlarmLists from the
+  // AlarmMap, so we have to be careful in the iteration to avoid using
+  // iterators after it's invalidated them.
+  for (AlarmMap::iterator m_it = alarms_.begin(), m_end = alarms_.end();
+       m_it != m_end;) {
+    AlarmMap::iterator cur_extension = m_it;
+    // cur_extension may be invalidated by OnAlarm in the last iteration of the
+    // nested loop, so increment the iterator before that.
+    ++m_it;

[Matt Perry, 2012/06/11 22:32:44]

nit: no need to explain iterator details here. Doing
  AlarmMap::iterator current = it++;
is a pretty common pattern and readers will understand why it's done.

[Jeffrey Yasskin, 2012/06/12 19:12:31]

Sounds good. I left most of the other explanations because I had to stare at the
original code for a bit to figure out why it was safe.

+
+    // Iterate with indices and backwards so that (a) removing elements doesn't
+    // affect upcoming iterations, and (b) if the last iteration destroys the
+    // AlarmList, I'm not about to use the end iterator that the destruction
+    // invalidates.
+    for (size_t i = cur_extension->second.size(); i > 0; --i) {
+      AlarmList::iterator cur_alarm = cur_extension->second.begin() + i - 1;
+      if (base::Time::FromJsTime(cur_alarm->js_alarm->scheduled_time) <=
+          next_poll_time_) {
+        OnAlarm(make_pair(cur_extension, cur_alarm));
+      }
     }
   }
 
   // Schedule the next poll. The soonest it may happen is after
-  // kDefaultMinPollPeriod or after the shortest scheduled delay of any alarm,
+  // kDefaultMinPollPeriod or after the shortest granularity of any alarm,
   // whichever comes sooner.
   base::TimeDelta min_poll_period = kDefaultMinPollPeriod;
-  for (AlarmRuntimeInfoMap::iterator it = scheduled_times_.begin();
-       it != scheduled_times_.end(); ++it) {
-    min_poll_period = std::min(TimeDeltaFromDelay(it->first->delay_in_minutes),
-                               min_poll_period);
+  for (AlarmMap::const_iterator m_it = alarms_.begin(), m_end = alarms_.end();
+       m_it != m_end; ++m_it) {
+    for (AlarmList::const_iterator
+             l_it = m_it->second.begin(), l_end = m_it->second.end();
+         l_it != l_end; ++l_it) {
+      if (l_it->granularity < min_poll_period)
+        min_poll_period = l_it->granularity;
+    }
   }
   ScheduleNextPoll(min_poll_period);
 }
@@ -300,10 +310,50 @@ void AlarmManager::Observe(
   }
 }
 
-AlarmPref::AlarmPref() {
+// AlarmManager::Alarm
+
+Alarm::Alarm()
+    : js_alarm(new extensions::api::alarms::Alarm()) {
+}
+
+Alarm::Alarm(const std::string& name,
+             const extensions::api::alarms::AlarmOneShotCreateInfo& create_info,
+             base::TimeDelta min_granularity,
+             TimeProvider now)
+    : js_alarm(new extensions::api::alarms::Alarm()) {
+  js_alarm->name = name;
+  if (create_info.delay_in_minutes.get()) {
+    // Relative scheduling.
+    base::TimeDelta delay = TimeDeltaFromDelay(*create_info.delay_in_minutes);
+    js_alarm->scheduled_time = (now() + delay).ToJsTime();
+    granularity = std::max(min_granularity, delay);
+  } else {
+    // Absolute scheduling.
+    CHECK(create_info.time.get())
+        << "ValidateAlarmOneShotCreateInfo in alarms_api.cc should have "
+        << "prevented this call.";
+    js_alarm->scheduled_time = *create_info.time;
+    granularity =
+        std::max(min_granularity,
+                 base::Time::FromJsTime(js_alarm->scheduled_time) - now());
+  }
+}
+
+Alarm::Alarm(
+    const std::string& name,
+    const extensions::api::alarms::AlarmRepeatingCreateInfo& create_info,
+    base::TimeDelta min_granularity,
+    TimeProvider now)
+    : js_alarm(new extensions::api::alarms::Alarm()) {
+  js_alarm->name = name;
+  base::TimeDelta delay = TimeDeltaFromDelay(create_info.period_in_minutes);
+  js_alarm->scheduled_time = (now() + delay).ToJsTime();
+  js_alarm->period_in_minutes.reset(
+      new double(create_info.period_in_minutes));
+  granularity = std::max(min_granularity, delay);
 }
 
-AlarmPref::~AlarmPref() {
+Alarm::~Alarm() {
 }
 
 }  // namespace extensions