Extension alarms now persist in Preferences.

Extension alarms now persist in Preferences.

BUG=122815
TEST=no

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133543

[Aaron Boodman, 2012-04-20 17:58:22]

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:48: base::Time
scheduled_run_time;
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.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:174: TimerInfo* timer =
new TimerInfo(alarm->repeating);
Nit: Maybe name this timer_info so you don't have timer-timer-> ?

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:185:
ExtensionSystem::Get(profile_)->extension_service();
Consider passing in ExtensionPrefs to make this more easily testable.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:191: AlarmMap::iterator
list = alarms_.find(extension_id);
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?

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:954: base::DictionaryValue*
alarm_dict;
= NULL

[Matt Perry, 2012-04-20 19:38:49]

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:48: base::Time
scheduled_run_time;
On 2012/04/20 17:58:22, Aaron Boodman wrote:
> 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.

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.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:185:
ExtensionSystem::Get(profile_)->extension_service();
On 2012/04/20 17:58:22, Aaron Boodman wrote:
> Consider passing in ExtensionPrefs to make this more easily testable.

I don't think that's necessary. TestExtensionSystem already has a facility for
creating a mock ExtensionService/ExtensionPrefs duo.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:191: AlarmMap::iterator
list = alarms_.find(extension_id);
On 2012/04/20 17:58:22, Aaron Boodman wrote:
> 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?

Are you worried about malicious extensions carpet-bombing users with alarms?

[Aaron Boodman, 2012-04-20 23:15:24]

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:48: base::Time
scheduled_run_time;
On 2012/04/20 19:38:49, Matt Perry wrote:
> On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > 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.
> 
> 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.

I see.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:185:
ExtensionSystem::Get(profile_)->extension_service();
On 2012/04/20 19:38:49, Matt Perry wrote:
> On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > Consider passing in ExtensionPrefs to make this more easily testable.
> 
> I don't think that's necessary. TestExtensionSystem already has a facility for
> creating a mock ExtensionService/ExtensionPrefs duo.

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.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:191: AlarmMap::iterator
list = alarms_.find(extension_id);
On 2012/04/20 19:38:49, Matt Perry wrote:
> On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > 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?
> 
> Are you worried about malicious extensions carpet-bombing users with alarms?

I'm was thinking about buggy extensions filling up the preferences file with
tons of alarm registrations.

[Matt Perry, 2012-04-20 23:39:41]

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:185:
ExtensionSystem::Get(profile_)->extension_service();
On 2012/04/20 23:15:24, Aaron Boodman wrote:
> On 2012/04/20 19:38:49, Matt Perry wrote:
> > On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > > Consider passing in ExtensionPrefs to make this more easily testable.
> > 
> > I don't think that's necessary. TestExtensionSystem already has a facility
for
> > creating a mock ExtensionService/ExtensionPrefs duo.
> 
> 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.

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.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:191: AlarmMap::iterator
list = alarms_.find(extension_id);
On 2012/04/20 23:15:24, Aaron Boodman wrote:
> On 2012/04/20 19:38:49, Matt Perry wrote:
> > On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > > 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?
> > 
> > Are you worried about malicious extensions carpet-bombing users with alarms?
> 
> I'm was thinking about buggy extensions filling up the preferences file with
> tons of alarm registrations.

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]

I recommend changing base::Timer to expose its delivery time. I don't see why
there'd be a problem with that.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:185:
ExtensionSystem::Get(profile_)->extension_service();
On 2012/04/20 23:39:42, Matt Perry wrote:
> On 2012/04/20 23:15:24, Aaron Boodman wrote:
> > On 2012/04/20 19:38:49, Matt Perry wrote:
> > > On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > > > Consider passing in ExtensionPrefs to make this more easily testable.
> > > 
> > > I don't think that's necessary. TestExtensionSystem already has a facility
> for
> > > creating a mock ExtensionService/ExtensionPrefs duo.
> > 
> > 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.
> 
> 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.

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.

http://codereview.chromium.org/10152008/diff/10001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:191: AlarmMap::iterator
list = alarms_.find(extension_id);
On 2012/04/20 23:39:42, Matt Perry wrote:
> On 2012/04/20 23:15:24, Aaron Boodman wrote:
> > On 2012/04/20 19:38:49, Matt Perry wrote:
> > > On 2012/04/20 17:58:22, Aaron Boodman wrote:
> > > > 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?
> > > 
> > > Are you worried about malicious extensions carpet-bombing users with
alarms?
> > 
> > I'm was thinking about buggy extensions filling up the preferences file with
> > tons of alarm registrations.
> 
> 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).

Hm, ok. Let's talk about this more next week. Different bug no matter what.

[Matt Perry, 2012-04-21 00:23:36]

+jar for base/timer.h

Jim, do you have any objections to exposing the next desired run time of a
Timer? Right now I'm keeping track of it externally, but I could get rid of a
bunch of bookkeeping code (in alarm_manager.cc) if I could just get that info
from Timer.

[jar (doing other things), 2012-04-21 15:55:51]

I'm a tad surprised that it is useful.  My two (minimal) fears would be
that (a) we'd some-day want to have an implementation that used some
underlying library or OS code that didn't allow such access <oops... but
not likely>, or (b) that the value is not what you want.  For instance,
what does the value mean when it is in less than Now()?  It could be that
the timer will go off RSN, or it could mean that the time is running... and
you are part of its execution.  Perchance that kind of confusion was
already present (what happens if you reset() the the time, while you are
actually running as part of the task?)

If your associated code is compatible with the issues I raised, then I'm
willing to L G T M, but I wanted to be sure you'd at least thought about
these issues.

Jim


On Fri, Apr 20, 2012 at 5:23 PM, <mpcomplete@chromium.org> wrote:

> +jar for base/timer.h
>
> Jim, do you have any objections to exposing the next desired run time of a
> Timer? Right now I'm keeping track of it externally, but I could get rid
> of a
> bunch of bookkeeping code (in alarm_manager.cc) if I could just get that
> info
> from Timer.
>
>
http://codereview.chromium.**org/10152008/<http://codereview.chromium.org/101...
>

[Matt Perry, 2012-04-23 19:39:36]

(a) was what I expected you to be worried about, and why I asked you to review.
As for (b), I think it's fine for my purposes. One of the following invariants
should hold:
- Timer::IsRunning() is true, OR
- desired_run_time_ indicates the next time the timer will fire; a value less
than Now() means the timer will fire RSN.

On 2012/04/21 15:55:51, jar wrote:
> I'm a tad surprised that it is useful.  My two (minimal) fears would be
> that (a) we'd some-day want to have an implementation that used some
> underlying library or OS code that didn't allow such access <oops... but
> not likely>, or (b) that the value is not what you want.  For instance,
> what does the value mean when it is in less than Now()?  It could be that
> the timer will go off RSN, or it could mean that the time is running... and
> you are part of its execution.  Perchance that kind of confusion was
> already present (what happens if you reset() the the time, while you are
> actually running as part of the task?)
> 
> If your associated code is compatible with the issues I raised, then I'm
> willing to L G T M, but I wanted to be sure you'd at least thought about
> these issues.
> 
> Jim
> 
> 
> On Fri, Apr 20, 2012 at 5:23 PM, <mailto:mpcomplete@chromium.org> wrote:
> 
> > +jar for base/timer.h
> >
> > Jim, do you have any objections to exposing the next desired run time of a
> > Timer? Right now I'm keeping track of it externally, but I could get rid
> > of a
> > bunch of bookkeeping code (in alarm_manager.cc) if I could just get that
> > info
> > from Timer.
> >
> >
>
http://codereview.chromium.**org/10152008/%3Chttp://codereview.chromium.org/1...>
> >

[jar (doing other things), 2012-04-23 19:43:21]

lgtm

[Matt Perry, 2012-04-23 21:31:58]

OK aa, got rid of TimerInfo in favor of base::Timer::desired_run_time

[Aaron Boodman, 2012-04-23 21:36:23]

lgtm