Make BackgroundBudgetService a KeyedService

This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

BUG=602244
Committed: https://crrev.com/0c1cff9f04f8884ff43d6be2ee003618c80a9db5
Cr-Commit-Position: refs/heads/master@{#387587}

[harkness, 2016-04-14 13:29:08]

harkness@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-04-14 13:55:35]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 13:55:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1889513004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1889513004/1

[Michael van Ouwerkerk, 2016-04-14 14:36:31]

Please update the CL description and subject to highlight what _this_ CL is
about. Some of that detail can be written once, in the bug this CL refers to.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service.cc (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service.cc:124: // static
nit: no longer

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_factory.cc (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.cc:18: return
NULL;
nit: s/NULL/nullptr/

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_factory.h (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:17:
content::BrowserContext* profile);
nit: it seems odd to name an argument of type BrowserContext |profile|, I'd
expect to see |context|.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:24:
~BackgroundBudgetServiceFactory() override{};
nit: odd spacing. Did you run format?

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:26:
KeyedService* BuildServiceInstanceFor(
Add this line above:
// BrowserContextKeyedServiceFactory interface.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:27:
content::BrowserContext* profile) const override;
nit: s/profile/context/

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_unittest.cc
(right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_unittest.cc:28:
TestingProfile* profile() { return &profile_; }
It seems a service() method instead of a profile() method would reduce verbosity
a lot.

[commit-bot: I haz the power, 2016-04-14 14:47:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 14:47:22]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-04-14 16:01:18]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2016-04-14 16:01:18]

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service.h (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service.h:25: explicit
BackgroundBudgetService(Profile* profile);
nit: virtual destructor

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_factory.cc (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.cc:33:
DependsOn(SiteEngagementServiceFactory::GetInstance());
PushMessagingServiceFactory should declare a dependency on the
BackgroundBudgetServiceFactory.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_factory.h (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:24:
~BackgroundBudgetServiceFactory() override{};
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> nit: odd spacing. Did you run format?

also no semi-colon please :-)

[harkness, 2016-04-14 17:28:09]

Description was changed from

==========
This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

BUG=602244
==========

to

==========
This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

BUG=602244
==========

[harkness, 2016-04-14 17:28:54]

Description was changed from

==========
This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

BUG=602244
==========

to

==========
This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

BUG=602244
==========

[harkness, 2016-04-14 17:44:19]

I renamed the issue; I should have done that before I sent it out. I don't think
the general info about the higher level goal should only be in the bug, I think
having it multiple places is a good thing, and it should certainly be in mind
for this code review. I did swap the order of the description though, to put the
info about the current CL first, with the info about the overall goal later,
which I think makes it clearer what's actually being done here.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service.cc (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service.cc:124: // static
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> nit: no longer

Done.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service.h (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service.h:25: explicit
BackgroundBudgetService(Profile* profile);
On 2016/04/14 16:01:18, Peter Beverloo wrote:
> nit: virtual destructor

Done.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_factory.cc (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.cc:18: return
NULL;
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.cc:33:
DependsOn(SiteEngagementServiceFactory::GetInstance());
On 2016/04/14 16:01:18, Peter Beverloo wrote:
> PushMessagingServiceFactory should declare a dependency on the
> BackgroundBudgetServiceFactory.

Done.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_factory.h (right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:17:
content::BrowserContext* profile);
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> nit: it seems odd to name an argument of type BrowserContext |profile|, I'd
> expect to see |context|.

I don't really have strong feelings either way, I just figured the caller is
going to be passing in a profile, so it would be more natural. I've updated it.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:24:
~BackgroundBudgetServiceFactory() override{};
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> nit: odd spacing. Did you run format?

I did, and I just ran it again with no change. I went ahead and updated the
spacing, and ran format again, and it didn't correct it back, so I guess this is
just a case format doesn't care about.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:26:
KeyedService* BuildServiceInstanceFor(
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> Add this line above:
> // BrowserContextKeyedServiceFactory interface.

Done.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_factory.h:27:
content::BrowserContext* profile) const override;
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> nit: s/profile/context/

Done.

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/background_budget_service_unittest.cc
(right):

https://codereview.chromium.org/1889513004/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/background_budget_service_unittest.cc:28:
TestingProfile* profile() { return &profile_; }
On 2016/04/14 14:36:31, Michael van Ouwerkerk wrote:
> It seems a service() method instead of a profile() method would reduce
verbosity
> a lot.

Done.

[harkness, 2016-04-15 11:53:27]

I created a new branch with the KeyedService changes and without the SES
changes, so that I could remove the dependency.

[Michael van Ouwerkerk, 2016-04-15 12:22:00]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 12:22:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1889513004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1889513004/60001

[Peter Beverloo, 2016-04-15 12:22:24]

\o/

lgtm % comment, but please get Michael's stamp too

https://codereview.chromium.org/1889513004/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/background_budget_service_factory.cc (right):

https://codereview.chromium.org/1889513004/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service_factory.cc:17: if
(profile->IsOffTheRecord())
Why not? The dependency (SiteEngagementServiceFactory) is available in
incognito, so I see no reason to derive here?

[Michael van Ouwerkerk, 2016-04-15 12:34:02]

nice, lgtm

[Michael van Ouwerkerk, 2016-04-15 12:34:04]

nice, lgtm

[commit-bot: I haz the power, 2016-04-15 13:17:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 13:17:14]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-04-15 13:19:06]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2016-04-15 13:19:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1889513004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1889513004/60001

[commit-bot: I haz the power, 2016-04-15 13:24:37]

Description was changed from

==========
This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

BUG=602244
==========

to

==========
This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

BUG=602244
==========

[commit-bot: I haz the power, 2016-04-15 13:24:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-15 13:26:06]

Description was changed from

==========
This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

BUG=602244
==========

to

==========
This code converts the previously static class of BackgroundBudgetService to a
KeyedService, which is tied to the lifetime of the Profile.

This immediately gives dependency tracking via the factory dependency, and
allows better test writing going forward.

This is part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.

BUG=602244

Committed: https://crrev.com/0c1cff9f04f8884ff43d6be2ee003618c80a9db5
Cr-Commit-Position: refs/heads/master@{#387587}
==========

[commit-bot: I haz the power, 2016-04-15 13:26:07]

Patchset 4 (id:??) landed as
https://crrev.com/0c1cff9f04f8884ff43d6be2ee003618c80a9db5
Cr-Commit-Position: refs/heads/master@{#387587}