Make chrome.alarms API accept non-integers and too-short delays, and lower the minimum.

We still don't honor the too-short delays (this CL actually makes us
more aggressive about not honoring them), but I've lowered the limit
to 1 minute, and we emit a warning to the console instead of erroring,
and we emit this warning even in dev mode since people were getting
surprised by the difference in behavior between dev and release.

I've also moved the frequency description into the create()
documentation from the chrome.alarms summary, at joemarini's request.

And I fixed the eventPages example to work with trunk Chrome.

BUG=168519,159879

Review URL: https://chromiumcodereview.appspot.com/11818010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@176004 0039d316-1c4b-4281-b951-d872f2087c98

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

@@ -29,7 +29,7 @@ const char kRegisteredAlarms[] = "alarms";
 const char kAlarmGranularity[] = "granularity";
 
 // The minimum period between polling for alarms to run.
-const base::TimeDelta kDefaultMinPollPeriod = base::TimeDelta::FromMinutes(1);
+const base::TimeDelta kDefaultMinPollPeriod = base::TimeDelta::FromDays(1);
 
 class DefaultAlarmDelegate : public AlarmManager::Delegate {
  public:
@@ -207,8 +207,7 @@ void AlarmManager::AddAlarmImpl(const std::string& extension_id,
 
   alarms_[extension_id].push_back(alarm);
 
-  // TODO(yoz): Is 0 really sane? There could be thrashing.
-  ScheduleNextPoll(base::TimeDelta::FromMinutes(0));
+  ScheduleNextPoll();
 }
 
 void AlarmManager::WriteToStorage(const std::string& extension_id) {
@@ -237,7 +236,7 @@ void AlarmManager::ReadFromStorage(const std::string& extension_id,
   }
 }
 
-void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
+void AlarmManager::ScheduleNextPoll() {
   // 0. If there are no alarms, stop the timer.
   if (alarms_.empty()) {
     timer_.Stop();
@@ -247,12 +246,12 @@ void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
   // TODO(yoz): Try not to reschedule every single time if we're adding
   // a lot of alarms.
 
-  base::Time next_poll(last_poll_time_ + min_period);
-
-  // Find the soonest alarm that is scheduled to run.
+  // Find the soonest alarm that is scheduled to run and the smallest
+  // granularity of any alarm.
   // 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);
+  base::TimeDelta min_granularity = kDefaultMinPollPeriod;
   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();
@@ -261,14 +260,18 @@ void AlarmManager::ScheduleNextPoll(base::TimeDelta min_period) {
           base::Time::FromJsTime(l_it->js_alarm->scheduled_time);
       if (cur_alarm_time < soonest_alarm_time)
         soonest_alarm_time = cur_alarm_time;
+      if (l_it->granularity < min_granularity)
+        min_granularity = l_it->granularity;
     }
   }
 
-  // If the next alarm is more than min_period in the future, wait for it.
-  // Otherwise, only poll as often as min_period.
-  if (last_poll_time_.is_null() || next_poll < soonest_alarm_time) {
+  base::Time next_poll(last_poll_time_ + min_granularity);
+  // If the next alarm is more than min_granularity in the future, wait for it.
+  // Otherwise, only poll as often as min_granularity.
+  // As a special case, if we've never checked for an alarm before
+  // (e.g. during startup), let alarms fire asap.
+  if (last_poll_time_.is_null() || next_poll < soonest_alarm_time)
     next_poll = soonest_alarm_time;
-  }
 
   // Schedule the poll.
   next_poll_time_ = next_poll;
@@ -303,19 +306,7 @@ void AlarmManager::PollAlarms() {
     }
   }
 
-  // Schedule the next poll. The soonest it may happen is after
-  // kDefaultMinPollPeriod or after the shortest granularity of any alarm,
-  // whichever comes sooner.
-  base::TimeDelta min_poll_period = kDefaultMinPollPeriod;
-  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_it != m_it->second.end(); ++l_it) {
-      if (l_it->granularity < min_poll_period)
-        min_poll_period = l_it->granularity;
-    }
-  }
-  ScheduleNextPoll(min_poll_period);
+  ScheduleNextPoll();
 }
 
 void AlarmManager::Observe(

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

@@ -123,8 +123,8 @@ class AlarmManager
                        scoped_ptr<base::Value> value);
 
   // Schedules the next poll of alarms for when the next soonest alarm runs,
-  // but do not more often than min_period.
-  void ScheduleNextPoll(base::TimeDelta min_period);
+  // but not more often than the minimum granularity of all alarms.
+  void ScheduleNextPoll();
 
   // Polls the alarms, running any that have elapsed. After running them and
   // rescheduling repeating alarms, schedule the next poll.

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

@@ -19,47 +19,18 @@ namespace {
 
 const char kDefaultAlarmName[] = "";
 const char kAlarmNotFound[] = "No alarm named '*' exists.";
-const char kDelayLessThanMinimum[] = "* is less than minimum of * minutes.";
-const char kDelayIsNonInteger[] = "* is not an integer value.";
 const char kBothRelativeAndAbsoluteTime[] =
     "Cannot set both when and delayInMinutes.";
 const char kNoScheduledTime[] =
     "Must set at least one of when, delayInMinutes, or periodInMinutes.";
-const int kReleaseDelayMinimum = 5;
+const int kReleaseDelayMinimum = 1;
 const int kDevDelayMinimum = 0;
 
-bool ValidateDelay(double delay_in_minutes,
+bool ValidateAlarmCreateInfo(const std::string& alarm_name,
+                             const alarms::AlarmCreateInfo& create_info,
                              const Extension* extension,
-                   const std::string& delay_or_period,
-                   std::string* error) {
-  double delay_minimum = kDevDelayMinimum;
-  if (extension->location() != Extension::LOAD) {
-    // In release mode we check for integer delay values and a stricter delay
-    // minimum.
-    if (delay_in_minutes != static_cast<int>(delay_in_minutes)) {
-      *error = ErrorUtils::FormatErrorMessage(
-          kDelayIsNonInteger,
-          delay_or_period);
-      return false;
-    }
-    delay_minimum = kReleaseDelayMinimum;
-  }
-
-  // Validate against our found delay minimum.
-  if (delay_in_minutes < delay_minimum) {
-    *error = ErrorUtils::FormatErrorMessage(
-        kDelayLessThanMinimum,
-        delay_or_period,
-        base::DoubleToString(delay_minimum));
-    return false;
-  }
-
-  return true;
-}
-
-bool ValidateAlarmCreateInfo(const alarms::AlarmCreateInfo& create_info,
-                             const Extension* extension,
-                             std::string* error) {
+                             std::string* error,
+                             std::vector<std::string>* warnings) {
   if (create_info.delay_in_minutes.get() &&
       create_info.when.get()) {
     *error = kBothRelativeAndAbsoluteTime;
@@ -79,14 +50,36 @@ bool ValidateAlarmCreateInfo(const alarms::AlarmCreateInfo& create_info,
   // gets delayed past the boundary).  However, it's still worth warning about
   // relative delays that are shorter than we'll honor.
   if (create_info.delay_in_minutes.get()) {
-    if (!ValidateDelay(*create_info.delay_in_minutes,
-                       extension, "Delay", error))
-      return false;
+    if (*create_info.delay_in_minutes < kReleaseDelayMinimum) {
+      COMPILE_ASSERT(kReleaseDelayMinimum == 1, update_warning_message_below);
+      if (extension->location() == Extension::LOAD)
+        warnings->push_back(ErrorUtils::FormatErrorMessage(
+            "Alarm delay is less than minimum of 1 minutes."
+            " In released .crx, alarm \"*\" will fire in approximately"
+            " 1 minutes.",
+            alarm_name));
+      else
+        warnings->push_back(ErrorUtils::FormatErrorMessage(
+            "Alarm delay is less than minimum of 1 minutes."
+            " Alarm \"*\" will fire in approximately 1 minutes.",
+            alarm_name));
+    }
   }
   if (create_info.period_in_minutes.get()) {
-    if (!ValidateDelay(*create_info.period_in_minutes,
-                       extension, "Period", error))
-      return false;
+    if (*create_info.period_in_minutes < kReleaseDelayMinimum) {
+      COMPILE_ASSERT(kReleaseDelayMinimum == 1, update_warning_message_below);
+      if (extension->location() == Extension::LOAD)
+        warnings->push_back(ErrorUtils::FormatErrorMessage(
+            "Alarm period is less than minimum of 1 minutes."
+            " In released .crx, alarm \"*\" will fire approximately"
+            " every 1 minutes.",
+            alarm_name));
+      else
+        warnings->push_back(ErrorUtils::FormatErrorMessage(
+            "Alarm period is less than minimum of 1 minutes."
+            " Alarm \"*\" will fire approximately every 1 minutes.",
+            alarm_name));
+    }
   }
 
   return true;
@@ -102,11 +95,17 @@ bool AlarmsCreateFunction::RunImpl() {
   scoped_ptr<alarms::Create::Params> params(
       alarms::Create::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
+  const std::string& alarm_name =
+      params->name.get() ? *params->name : kDefaultAlarmName;
+  std::vector<std::string> warnings;
   if (!ValidateAlarmCreateInfo(
-          params->alarm_info, GetExtension(), &error_))
+          alarm_name, params->alarm_info, GetExtension(), &error_, &warnings))
     return false;
+  for (std::vector<std::string>::const_iterator it = warnings.begin();
+       it != warnings.end(); ++it)
+    WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING, *it);
 
-  Alarm alarm(params->name.get() ? *params->name : kDefaultAlarmName,
+  Alarm alarm(alarm_name,
               params->alarm_info,
               base::TimeDelta::FromMinutes(
                   GetExtension()->location() == Extension::LOAD ?

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

@@ -4,14 +4,19 @@
 
 // This file tests the chrome.alarms extension API.
 
-#include "base/values.h"
 #include "base/test/mock_time_provider.h"
+#include "base/values.h"
 #include "chrome/browser/extensions/api/alarms/alarm_manager.h"
 #include "chrome/browser/extensions/api/alarms/alarms_api.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/common/extensions/extension_messages.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/mock_render_process_host.h"
+#include "ipc/ipc_test_sink.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -54,6 +59,10 @@ class ExtensionAlarmsTest : public BrowserWithTestWindowTest {
     extension_ = utils::CreateEmptyExtensionWithLocation(
         extensions::Extension::LOAD);
 
+    // Make sure there's a RenderViewHost for alarms to warn into.
+    AddTab(browser(), extension_->GetBackgroundURL());
+    contents_ = chrome::GetActiveWebContents(browser());
+
     current_time_ = base::Time::FromDoubleT(10);
     ON_CALL(mock_time_, Now())
         .WillByDefault(testing::ReturnPointee(&current_time_));
@@ -63,6 +72,7 @@ class ExtensionAlarmsTest : public BrowserWithTestWindowTest {
       UIThreadExtensionFunction* function, const std::string& args) {
     scoped_refptr<UIThreadExtensionFunction> delete_function(function);
     function->set_extension(extension_.get());
+    function->SetRenderViewHost(contents_->GetRenderViewHost());
     return utils::RunFunctionAndReturnSingleResult(function, args, browser());
   }
 
@@ -86,6 +96,7 @@ class ExtensionAlarmsTest : public BrowserWithTestWindowTest {
   std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
                                         const std::string& args) {
     function->set_extension(extension_.get());
+    function->SetRenderViewHost(contents_->GetRenderViewHost());
     return utils::RunFunctionAndReturnError(function, args, browser());
   }
 
@@ -134,6 +145,7 @@ class ExtensionAlarmsTest : public BrowserWithTestWindowTest {
   AlarmManager* alarm_manager_;
   AlarmDelegate* alarm_delegate_;
   scoped_refptr<extensions::Extension> extension_;
+  content::WebContents* contents_;
 };
 
 TEST_F(ExtensionAlarmsTest, Create) {
@@ -184,7 +196,7 @@ TEST_F(ExtensionAlarmsTest, CreateRepeating) {
 
   current_time_ += base::TimeDelta::FromSeconds(1);
   // Wait again, and ensure the alarm fires again.
-  alarm_manager_->ScheduleNextPoll(base::TimeDelta::FromSeconds(0));
+  alarm_manager_->ScheduleNextPoll();
   MessageLoop::current()->Run();
 
   ASSERT_EQ(2u, alarm_delegate_->alarms_seen.size());
@@ -259,10 +271,17 @@ TEST_F(ExtensionAlarmsTest, CreateDupe) {
 
 TEST_F(ExtensionAlarmsTest, CreateDelayBelowMinimum) {
   // Create an alarm with delay below the minimum accepted value.
-  std::string error = RunFunctionAndReturnError(
-      new AlarmsCreateFunction(&base::MockTimeProvider::StaticNow),
-      "[\"negative\", {\"delayInMinutes\": -0.2}]");
-  EXPECT_FALSE(error.empty());
+  CreateAlarm("[\"negative\", {\"delayInMinutes\": -0.2}]");
+  IPC::TestSink& sink = static_cast<content::MockRenderProcessHost*>(
+      contents_->GetRenderViewHost()->GetProcess())->sink();
+  const IPC::Message* warning = sink.GetUniqueMessageMatching(
+      ExtensionMsg_AddMessageToConsole::ID);
+  ASSERT_TRUE(warning);
+  content::ConsoleMessageLevel level;
+  std::string message;
+  ExtensionMsg_AddMessageToConsole::Read(warning, &level, &message);
+  EXPECT_EQ(content::CONSOLE_MESSAGE_LEVEL_WARNING, level);
+  EXPECT_THAT(message, testing::HasSubstr("delay is less than minimum of 1"));
 }
 
 TEST_F(ExtensionAlarmsTest, Get) {
@@ -357,7 +376,7 @@ TEST_F(ExtensionAlarmsTest, Clear) {
 
   // Now wait for the alarms to fire, and ensure the cancelled alarms don't
   // fire.
-  alarm_manager_->ScheduleNextPoll(base::TimeDelta::FromSeconds(0));
+  alarm_manager_->ScheduleNextPoll();
   MessageLoop::current()->Run();
 
   ASSERT_EQ(1u, alarm_delegate_->alarms_seen.size());
@@ -476,15 +495,18 @@ TEST_F(ExtensionAlarmsSchedulingTest, ReleasedExtensionPollsInfrequently) {
   CreateAlarm("[\"a\", {\"when\": 300010}]");
   CreateAlarm("[\"b\", {\"when\": 340000}]");
 
-  // In released extensions, we set the granularity to at least 1
-  // minute, but AddAlarm schedules its next poll precisely.
+  // On startup (when there's no "last poll"), we let alarms fire as
+  // soon as they're scheduled.
   EXPECT_DOUBLE_EQ(300010, alarm_manager_->next_poll_time_.ToJsTime());
 
-  // Run an iteration to see the effect of the granularity.
-  current_time_ = base::Time::FromJsTime(300020);
-  MessageLoop::current()->Run();
-  EXPECT_DOUBLE_EQ(300020, alarm_manager_->last_poll_time_.ToJsTime());
-  EXPECT_DOUBLE_EQ(360020, alarm_manager_->next_poll_time_.ToJsTime());
+  alarm_manager_->last_poll_time_ = base::Time::FromJsTime(290000);
+  // In released extensions, we set the granularity to at least 5
+  // minutes, which makes AddAlarm schedule the next poll after the
+  // extension requested.
+  alarm_manager_->ScheduleNextPoll();
+  EXPECT_DOUBLE_EQ((alarm_manager_->last_poll_time_ +
+                    base::TimeDelta::FromMinutes(1)).ToJsTime(),
+                   alarm_manager_->next_poll_time_.ToJsTime());
 }
 
 TEST_F(ExtensionAlarmsSchedulingTest, TimerRunning) {

chrome/browser/extensions/extension_function.cc

@@ -289,6 +289,13 @@ void UIThreadExtensionFunction::SendResponse(bool success) {
   }
 }
 
+void UIThreadExtensionFunction::WriteToConsole(
+    content::ConsoleMessageLevel level,
+    const std::string& message) {
+  render_view_host_->Send(new ExtensionMsg_AddMessageToConsole(
+      render_view_host_->GetRoutingID(), level, message));
+}
+
 IOThreadExtensionFunction::IOThreadExtensionFunction()
     : routing_id_(-1) {
 }

chrome/browser/extensions/extension_function.h

@@ -20,6 +20,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/render_view_host_observer.h"
+#include "content/public/common/console_message_level.h"
 #include "ipc/ipc_message.h"
 
 class Browser;
@@ -316,6 +317,10 @@ class UIThreadExtensionFunction : public ExtensionFunction {
       const extensions::WindowController* window_controller) const;
 
  protected:
+  // Emits a message to the extension's devtools console.
+  void WriteToConsole(content::ConsoleMessageLevel level,
+                      const std::string& message);
+
   friend struct content::BrowserThread::DeleteOnThread<
       content::BrowserThread::UI>;
   friend class base::DeleteHelper<UIThreadExtensionFunction>;

chrome/common/extensions/api/alarms.idl

@@ -49,9 +49,18 @@ namespace alarms {
     // the same name (or no name if none is specified), it will be cancelled and
     // replaced by this alarm.
     //
-    // Note that granularity is not guaranteed: times are more of a hint to the
-    // browser. For performance reasons, alarms may be delayed an arbitrary
-    // amount of time before firing.
+    // In order to reduce the load on the user's machine, Chrome limits alarms
+    // to at most once every 1 minutes but may delay them an arbitrary amount
+    // more.  That is, setting <code>$ref:[alarms.AlarmCreateInfo.delayInMinutes
+    // delayInMinutes]</code> or
+    // <code>$ref:[alarms.AlarmCreateInfo.periodInMinutes
+    // periodInMinutes]</code> to less than <code>1</code> will not be honored
+    // and will cause a warning.  <code>$ref:[alarms.AlarmCreateInfo.when
+    // when]</code> can be set to less than 1 minutes after "now" without
+    // warning but won't actually cause the alarm to fire for at least 1 minutes.
+    //
+    // To help you debug your app or extension, when you've loaded it unpacked,
+    // there's no limit to how often the alarm can fire.
     //
     // |name|: Optional name to identify this alarm. Defaults to the empty
     // string.

chrome/common/extensions/docs/examples/api/eventPage/basic/background.js

@@ -52,12 +52,14 @@ chrome.browserAction.onClicked.addListener(function() {
   });
 });
 
-chrome.experimental.keybinding.onCommand.addListener(function(command) {
+chrome.commands.onCommand.addListener(function(command) {
   chrome.tabs.create({url: "http://www.google.com/"});
 });
 
 chrome.runtime.onMessage.addListener(function(msg, _, sendResponse) {
   if (msg.setAlarm) {
+    // For testing only.  delayInMinutes will be rounded up to at least 5 in a
+    // packed or released extension.
     chrome.alarms.create({delayInMinutes: 0.1});
   } else if (msg.delayedResponse) {
     // Note: setTimeout itself does NOT keep the page awake. We return true

chrome/common/extensions/docs/examples/api/eventPage/basic/manifest.json

@@ -3,7 +3,7 @@
   "description": "Demonstrates usage and features of the event page",
   "version": "1.0",
   "manifest_version": 2,
-  "permissions": ["alarms", "tabs", "bookmarks", "experimental", "keybinding", "http://*.google.com/*"],
+  "permissions": ["alarms", "tabs", "bookmarks", "declarativeWebRequest", "*://*.google.com/*"],
   "background": {
     "scripts": ["background.js"],
     "persistent": false

chrome/common/extensions/docs/templates/intros/alarms.html

@@ -22,18 +22,3 @@
     <td><a href="event_pages.html">Event Pages</a></td>
   </tr>
 </table>
-
-<h2 id="frequency-limits">Alarm frequency limits</h2>
-<p>In order to reduce the load on the user's machine, Chrome limits
-alarms to at most once every 5 minutes but may delay them an arbitrary
-amount more.  That is, setting
-<code>$ref:[alarms.AlarmCreateInfo.delayInMinutes
-delayInMinutes]</code> or
-<code>$ref:[alarms.AlarmCreateInfo.periodInMinutes
-periodInMinutes]</code> to less than <code>5</code> will cause an
-error.  <code>$ref:[alarms.AlarmCreateInfo.when when]</code> can be
-set to less than 5 minutes after "now" but won't actually cause the
-alarm to run for at least 5 minutes.</p>
-
-<p>To help you debug your app or extension, when you've loaded it
-unpacked, there's no limit to how often the alarm can fire.</p>