Refactor chrome.alarms interface to support absolute alarm deadlines.

Refactor chrome.alarms interface to support absolute alarm deadlines.

BUG=122817
TEST=Created an extension with absolute delays under 5 minutes away,
and watched that they didn't fire for 5 minutes.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142037

[Aaron Boodman, 2012-06-09 04:21:10]

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/alarms.idl (right):

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
chrome/common/extensions/api/alarms.idl:20: double? repeatAfterMinutes;
What is the purpose of this property?

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
chrome/common/extensions/api/alarms.idl:31: // This option may not be set if
delayInMinutes is set or
How about just creating two methods: createOneShot() and createRepeating().

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
chrome/common/extensions/api/alarms.idl:33: double? approxTimeToFire;
I don't think the 'approx' qualifier is needed. That is implied by the
conceptual purpose of the API and can be explained in the comments.

[Jeffrey Yasskin, 2012-06-11 19:28:36]

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/alarms.idl (right):

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
chrome/common/extensions/api/alarms.idl:20: double? repeatAfterMinutes;
On 2012/06/09 04:21:10, Aaron Boodman wrote:
> What is the purpose of this property?

Checking whether it's set at all replaces the 'repeating' property. Its value
used to be in the 'delayInMinutes', but since that's now an absolute time, it
doesn't encode the period of the repetition anymore, so I've stuck that
information here.

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
chrome/common/extensions/api/alarms.idl:31: // This option may not be set if
delayInMinutes is set or
On 2012/06/09 04:21:10, Aaron Boodman wrote:
> How about just creating two methods: createOneShot() and createRepeating().

Sure. How's this look?

https://chromiumcodereview.appspot.com/10545104/diff/1/chrome/common/extensio...
chrome/common/extensions/api/alarms.idl:33: double? approxTimeToFire;
On 2012/06/09 04:21:10, Aaron Boodman wrote:
> I don't think the 'approx' qualifier is needed. That is implied by the
> conceptual purpose of the API and can be explained in the comments.

Done.

[Aaron Boodman, 2012-06-11 20:57:28]

API lgtm w/ nit below. Matt should still review the code.

https://chromiumcodereview.appspot.com/10545104/diff/13/chrome/common/extensi...
File chrome/common/extensions/api/alarms.idl (right):

https://chromiumcodereview.appspot.com/10545104/diff/13/chrome/common/extensi...
chrome/common/extensions/api/alarms.idl:32: double? timeToFire;
Just 'time' is fine for the name.

[Jeffrey Yasskin, 2012-06-11 21:05:06]

https://chromiumcodereview.appspot.com/10545104/diff/13/chrome/common/extensi...
File chrome/common/extensions/api/alarms.idl (right):

https://chromiumcodereview.appspot.com/10545104/diff/13/chrome/common/extensi...
chrome/common/extensions/api/alarms.idl:32: double? timeToFire;
On 2012/06/11 20:57:28, Aaron Boodman wrote:
> Just 'time' is fine for the name.

Done.

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

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:152: if (double*
period_in_minutes =
I always thought our style guide banned this, but I now see it's explicitly
allowed. Cool.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:193:
service->extension_prefs()->SetRegisteredAlarms(extension_id, alarm_prefs);
I have a CL that changes this a bit. The slower one of us is going to have to
merge to it.

If you're the slower one, I'd just pass alarms_[extension_id] here. No need to
copy it to a temp list.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:228: l_it =
m_it->second.begin(), l_end = m_it->second.end();
nit: getting rid of l_end would actually require less lines, since you wouldn't
need to wrap here.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:265: ++m_it;
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.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.h:31: const
extensions::api::alarms::AlarmOneShotCreateInfo& create_info,
extensions prefix is unnecessary

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarms_api.cc:25: "Cannot set both
approxTimeToFire and delayInMinutes.";
error messages are out of date

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarms_api.cc:106: ? kDevDelayMinimum :
kReleaseDelayMinimum),
chrome style is to put operators on the line preceeding the linewrap

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/extension_prefs.cc:767: void Save(DictionaryValue*
dictionary,
I would still call this SaveTime since it only works on the time classes.

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

Matt and I talked about this on IRC, and now that repeating alarms also have an
initial time, we think having two entry points is no longer the right interface,
so I've merged createOneShot and createRepeating back together.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:193:
service->extension_prefs()->SetRegisteredAlarms(extension_id, alarm_prefs);
On 2012/06/11 22:32:44, Matt Perry wrote:
> I have a CL that changes this a bit. The slower one of us is going to have to
> merge to it.
> 
> If you're the slower one, I'd just pass alarms_[extension_id] here. No need to
> copy it to a temp list.

I already don't need to copy it to a temp list. I just forgot to fix that when I
removed the linked_ptr layer in AlarmList. Thanks!

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:228: l_it =
m_it->second.begin(), l_end = m_it->second.end();
On 2012/06/11 22:32:44, Matt Perry wrote:
> nit: getting rid of l_end would actually require less lines, since you
wouldn't
> need to wrap here.

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.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.cc:265: ++m_it;
On 2012/06/11 22:32:44, Matt Perry wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarm_manager.h:31: const
extensions::api::alarms::AlarmOneShotCreateInfo& create_info,
On 2012/06/11 22:32:44, Matt Perry wrote:
> extensions prefix is unnecessary

Whoops, removed everywhere.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarms_api.cc:25: "Cannot set both
approxTimeToFire and delayInMinutes.";
On 2012/06/11 22:32:44, Matt Perry wrote:
> error messages are out of date

Oops, fixed.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarms_api.cc:106: ? kDevDelayMinimum :
kReleaseDelayMinimum),
On 2012/06/11 22:32:44, Matt Perry wrote:
> chrome style is to put operators on the line preceeding the linewrap

Yay for style comments. :) Done.

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/1014/chrome/browser/exte...
chrome/browser/extensions/extension_prefs.cc:767: void Save(DictionaryValue*
dictionary,
On 2012/06/11 22:32:44, Matt Perry wrote:
> I would still call this SaveTime since it only works on the time classes.

Done.

[Matt Perry, 2012-06-12 20:54:29]

lgtm

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarms_api_unittest.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarms_api_unittest.cc:127:
&base::MockTimeProvider::StaticNow),
nit: looks like this doesn't need to wrap?

[Aaron Boodman, 2012-06-12 23:02:38]

lgtm

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/common/exten...
File chrome/common/extensions/api/alarms.idl (right):

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/common/exten...
chrome/common/extensions/api/alarms.idl:31: // Cannot be specified with
|delayInMinutes|, and one of |when|,
Maybe it would be simpler to just describe the allowed calls in the structure's
comments, rather than repeating for each parameter.

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/common/exten...
chrome/common/extensions/api/alarms.idl:70: 
extra line

[Jeffrey Yasskin, 2012-06-12 23:37:28]

I'll re-test in the browser, and then commit. Thanks for the review.

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/browser/exte...
File chrome/browser/extensions/api/alarms/alarms_api_unittest.cc (right):

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/browser/exte...
chrome/browser/extensions/api/alarms/alarms_api_unittest.cc:127:
&base::MockTimeProvider::StaticNow),
On 2012/06/12 20:54:29, Matt Perry wrote:
> nit: looks like this doesn't need to wrap?

Oops, fixed.

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/common/exten...
File chrome/common/extensions/api/alarms.idl (right):

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/common/exten...
chrome/common/extensions/api/alarms.idl:31: // Cannot be specified with
|delayInMinutes|, and one of |when|,
On 2012/06/12 23:02:38, Aaron Boodman wrote:
> Maybe it would be simpler to just describe the allowed calls in the
structure's
> comments, rather than repeating for each parameter.

Done.

https://chromiumcodereview.appspot.com/10545104/diff/4005/chrome/common/exten...
chrome/common/extensions/api/alarms.idl:70: 
On 2012/06/12 23:02:38, Aaron Boodman wrote:
> extra line

Oops, fixed.

[commit-bot: I haz the power, 2012-06-12 23:46:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10545104/12004

[commit-bot: I haz the power, 2012-06-12 23:46:09]

Failed to apply patch for chrome/browser/extensions/api/alarms/alarm_manager.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/api/alarms/alarm_manager.cc
Hunk #1 succeeded at 37 (offset 5 lines).
Hunk #2 succeeded at 100 (offset 45 lines).
Hunk #3 FAILED at 68.
Hunk #4 succeeded at 147 (offset 49 lines).
Hunk #5 FAILED at 131.
Hunk #6 FAILED at 187.
Hunk #7 FAILED at 209.
Hunk #8 succeeded at 281 (offset 47 lines).
Hunk #9 succeeded at 304 (offset 47 lines).
Hunk #10 succeeded at 312 (offset 47 lines).
Hunk #11 FAILED at 320.
5 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/alarms/alarm_manager.cc.rej

[Jeffrey Yasskin, 2012-06-13 18:50:48]

Brett, could you review the change to base/value_conversions? I changed the new
functions to work on TimeDeltas instead of Times.

[brettw, 2012-06-13 21:50:49]

lgtm

[commit-bot: I haz the power, 2012-06-13 21:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10545104/28

[commit-bot: I haz the power, 2012-06-14 00:31:01]

Change committed as 142037