Change AlarmManager to use ProfileKeyedAPI.

Change AlarmManager to use ProfileKeyedAPI.

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

[Devlin, 2013-04-17 01:30:59]

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.h:52: public ProfileKeyedAPI
{
Nit: this is the most important inheritance; put it first.

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.h:159: bool owns_clock_;
This is bad.  What if we instead have AlarmManager own the clock in all cases?

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_system.h:31: const base::FilePath&
install_directory,
Not your code, but please wrap this properly, since you're here :)

[Patrick Riordan, 2013-04-17 03:40:39]

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.h:52: public ProfileKeyedAPI
{
On 2013/04/17 01:30:59, D Cronin wrote:
> Nit: this is the most important inheritance; put it first.

Done.

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.h:159: bool owns_clock_;
On 2013/04/17 01:30:59, D Cronin wrote:
> This is bad.  What if we instead have AlarmManager own the clock in all cases?

Done.

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/14319002/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_system.h:31: const base::FilePath&
install_directory,
On 2013/04/17 01:30:59, D Cronin wrote:
> Not your code, but please wrap this properly, since you're here :)

Done.

[Devlin, 2013-04-22 21:19:28]

lgtm with nits

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/alarms/alarm_manager.cc:171: void
AlarmManager::SetClockForTesting(base::Clock* clock) {
Put methods in the .cc file in approximately the same order as they appear in
the .h file.

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/alarms/alarm_manager.h:89: // Deletes
AlarmManager's owned clock and replaces it with |clock|.
You should note that it takes ownership of |clock| as well.

[Patrick Riordan, 2013-04-23 23:32:04]

+ yoz for c/b/extensions/
+ mirandac for c/b/profiles/

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/alarms/alarm_manager.cc:171: void
AlarmManager::SetClockForTesting(base::Clock* clock) {
On 2013/04/22 21:19:28, D Cronin wrote:
> Put methods in the .cc file in approximately the same order as they appear in
> the .h file.

They do.

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/14319002/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/alarms/alarm_manager.h:89: // Deletes
AlarmManager's owned clock and replaces it with |clock|.
On 2013/04/22 21:19:28, D Cronin wrote:
> You should note that it takes ownership of |clock| as well.

Done.

[Yoyo Zhou, 2013-04-24 00:34:20]

LGTM
The CL description should describe what the CL is doing rather than the state
that the CL is effecting. For instance, "Change AlarmManager to use
ProfileKeyedAPI."

[Miranda Callahan, 2013-04-24 09:54:28]

On 2013/04/24 00:34:20, Yoyo Zhou wrote:
> LGTM
> The CL description should describe what the CL is doing rather than the state
> that the CL is effecting. For instance, "Change AlarmManager to use
> ProfileKeyedAPI."

profiles/* LGTM.

[commit-bot: I haz the power, 2013-04-24 23:22:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/14319002/1...

[commit-bot: I haz the power, 2013-04-25 00:17:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/14319002/4...

[commit-bot: I haz the power, 2013-04-25 01:11:12]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-25 01:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/14319002/5...

[commit-bot: I haz the power, 2013-04-25 01:30:31]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-04-25 22:29:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/14319002/4...

[commit-bot: I haz the power, 2013-04-26 00:40:07]

Change committed as 196555