Fix a bug where the minimum granular time for Alarms was not calculated properly.

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 aa483ee9363e160ee2011a141b2f97e8ac6f8d22..38a772405fe8800d13fdf1a7a9634aa040ed5f85 100644
--- a/chrome/browser/extensions/api/alarms/alarm_manager.cc
+++ b/chrome/browser/extensions/api/alarms/alarm_manager.cc
@@ -299,7 +299,7 @@ void AlarmManager::ReadFromStorage(const std::string& extension_id,
 }
 
 void AlarmManager::ScheduleNextPoll() {
-  // 0. If there are no alarms, stop the timer.
+  // If there are no alarms, stop the timer.
   if (alarms_.empty()) {
     timer_.Stop();
     return;
@@ -324,6 +324,11 @@ void AlarmManager::ScheduleNextPoll() {
         soonest_alarm_time = cur_alarm_time;
       if (l_it->granularity < min_granularity)
         min_granularity = l_it->granularity;
+      base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now();
+      if (cur_alarm_delta < min_granularity)
+        min_granularity = cur_alarm_delta;
+      if (min_granularity < l_it->minimum_granularity)

[Matt Perry, 2014/01/16 22:58:34]

I think these 2 ifs are reversed. The idea here is that
l_it->minimum_granularity is the extension's granularity (0 for an unpacked
extension, 1 min otherwise). So if a release-mode alarm will go off 30 sec from
now, it should be capped to 1min instead. But that shouldn't override the
min_granularity if it's already set to go off in, say, 10 sec.

TLDR:

base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now();
if (cur_alarm_delta < l_it->minimum_granularity)
  cur_alarm_delta = l_it->minimum_granularity;
if (cur_alarm_delta < min_granularity)
  min_granularity = cur_alarm_delta;

[Yoyo Zhou, 2014/01/16 23:34:32]

This lg to me.

[iclelland, 2014/01/17 14:54:32]

I don't think it's reversed -- it should _increase_ min_granularity if it's
below an alarm's minimum_granularity.

However, it makes the result sensitive to the order of the alarms iterated over.
If an early alarm has a large minimum_granularity, then min_granularity would be
set to that, but a later alarm with a small granularity (not
minimum_granularity) would lower it again.

Example:
alarm1: granularity=1min, minumum_granularity=1min
alarm2: granularity=10min, minimum_granularity=1min
alarm3: granularity=1000min, minimum_granularity=100min

Given (alarm1, alarm2, alarm3), min_granularity ends up at 100min.
Given (alarm2, alarm3, alarm1), min_granularity ends up at 1min.
Given (alarm1, alarm3, alarm2), min_granularity ends up at 10min.


I think what was intended here was:
   min_granularity = max ( min(all granularity members), max(all
minimum_granularity members) )

But that's not what's being computed.

[Matt Perry, 2014/01/17 18:43:52]

That's not the intended calculation. Each alarm's minimum_granularity should
only affect that alarm. So for your example, our desired min_granularity is
1min, due to alarm 1.

What we want is:
min_granularity = min( for each alarm, max(alarm granularity, alarm
minimum_granularity) )

+        min_granularity = l_it->minimum_granularity;
     }
   }
 
@@ -426,6 +431,7 @@ Alarm::Alarm(const std::string& name,
              base::Time now)
     : js_alarm(new api::alarms::Alarm()) {
   js_alarm->name = name;
+  minimum_granularity = min_granularity;
 
   if (create_info.when.get()) {
     // Absolute scheduling.