Full hookup of BudgetManager interfaces to BudgetDatabase.

Full hookup of BudgetManager interfaces to BudgetDatabase.

This includes changing the GetBudgetDetails on BudgetDatabase to use the mojo
types and plumbing the result through the BudgetManager. It pulls out all of
the old implementation in BudetManager that used the prefs to store data
(although cleanup of that isn't quite complete). It also replaces the old
GetBudget/StoreBudget calls in the push messaging system with the new Consume
call.

Tests were added in BudgetManager for the new reserve functionality, and any
tests of GetBudget that were still applicable were moved from BudgetManager
to BudgetDatabase.

BUG=617971
Committed: https://crrev.com/6f6b4143aa82b7876a8a0678e2cabb7074031ee5
Cr-Commit-Position: refs/heads/master@{#417236}

[harkness, 2016-08-25 19:51:18]

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

[harkness, 2016-08-25 19:51:19]

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:29: // This is 10 days = 240
hours.
I went ahead and kept it at 10 days. We can collect numbers and revisit.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:88: double
BudgetDatabase::GetBudget(const GURL& origin) {
We need this because the data structure for mojo is a vector, so we can't do the
nifty emplace_front trick. Also, it's used for the UMA.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:137:
callback.Run(std::move(predictions));
I'll add this when I add Reserve to the mojo service, since it will touch the
same set of files.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.h (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.h:38: using GetBudgetCallback =
blink::mojom::BudgetService::GetBudgetCallback;
Is this allowed?

[harkness, 2016-08-26 05:55:32]

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

[commit-bot: I haz the power, 2016-08-26 05:55:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 06:32:28]

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

[commit-bot: I haz the power, 2016-08-26 06:32:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peter Beverloo, 2016-08-26 14:56:44]

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:7: #include
"base/containers/adapters.h"
nit: unused

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:29: // This is 10 days = 240
hours.
On 2016/08/25 19:51:19, harkness wrote:
> I went ahead and kept it at 10 days. We can collect numbers and revisit.

Acknowledged.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:192:
UMA_HISTOGRAM_COUNTS_100("PushMessaging.SESForLowBudgetOrigin", score);
We tried to hard to keep these out of here :( Let's just accept this and go with
it for now.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.cc:26: // TODO(harkness): Add code
to delete old database info stored in prefs.
Will this happen before landing the CL?

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.h (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.h:38: using GetBudgetCallback =
blink::mojom::BudgetService::GetBudgetCallback;
On 2016/08/25 19:51:19, harkness wrote:
> Is this allowed?

Yeah.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager_unittest.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager_unittest.cc:30: BudgetManagerTest()
{}
nit: no need to define the constructor, the compiler will do this for you

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager_unittest.cc:101:
ASSERT_FALSE(ConsumeBudget(type));
This suite goes down from 8 to 1 unit test. Is that really all that should be
tested here?

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

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:194:
BudgetManager::ConsumeCallback consumeCallback =
nit: I wouldn't use the local here. Up to you.

[harkness, 2016-08-31 13:15:36]

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:7: #include
"base/containers/adapters.h"
On 2016/08/26 14:56:43, Peter Beverloo wrote:
> nit: unused

Done.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:192:
UMA_HISTOGRAM_COUNTS_100("PushMessaging.SESForLowBudgetOrigin", score);
On 2016/08/26 14:56:43, Peter Beverloo wrote:
> We tried to hard to keep these out of here :( Let's just accept this and go
with
> it for now.

Acknowledged.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.cc:26: // TODO(harkness): Add code
to delete old database info stored in prefs.
On 2016/08/26 14:56:44, Peter Beverloo wrote:
> Will this happen before landing the CL?

Done now.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager.h (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.h:38: using GetBudgetCallback =
blink::mojom::BudgetService::GetBudgetCallback;
On 2016/08/26 14:56:44, Peter Beverloo wrote:
> On 2016/08/25 19:51:19, harkness wrote:
> > Is this allowed?
> 
> Yeah.

Acknowledged.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
File chrome/browser/budget_service/budget_manager_unittest.cc (right):

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager_unittest.cc:30: BudgetManagerTest()
{}
On 2016/08/26 14:56:44, Peter Beverloo wrote:
> nit: no need to define the constructor, the compiler will do this for you

Done.

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager_unittest.cc:101:
ASSERT_FALSE(ConsumeBudget(type));
On 2016/08/26 14:56:44, Peter Beverloo wrote:
> This suite goes down from 8 to 1 unit test. Is that really all that should be
> tested here?

A lot of the functionality for this moved into the budget_database, so most of
the tests that were here migrated there. 

The only functionality that's remaining in the Manager is the Reserve/Consume
cache, so that's the one test that needs to be here.

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

https://codereview.chromium.org/2281673002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:194:
BudgetManager::ConsumeCallback consumeCallback =
On 2016/08/26 14:56:44, Peter Beverloo wrote:
> nit: I wouldn't use the local here. Up to you.

Done.

[harkness, 2016-09-02 14:19:38]

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

[commit-bot: I haz the power, 2016-09-02 14:19:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 15:02:01]

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

[commit-bot: I haz the power, 2016-09-02 15:02:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[harkness, 2016-09-05 13:09:07]

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

[commit-bot: I haz the power, 2016-09-05 13:09:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-05 13:52:51]

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

[commit-bot: I haz the power, 2016-09-05 13:52:52]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-09-05 13:58:57]

PTAL at modifications to push_messaging_browsertest.cc.

[Peter Beverloo, 2016-09-05 14:39:03]

lgtm % some nits

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:88: double total = 0.0;
nit: s/0.0/0/

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:162: 
Can we DCHECK_EQ(0, total)?

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:176: // Get the current SES
score, to generate UMA..
nit: .. -> .

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:197:
TEST_F(BudgetDatabaseTest, GetBudgetNegativeTime) {
Could you lead this test's body with a brief comment explaining why we're going
back in time here?

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager.cc:27: // information still there,
clear it.
// TODO(harkness): Remove once Chrome 56 has branched.

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager.cc:67: // Just pass the call into
the database.
Does this comment add sufficient value? It's exactly what the next call does.

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

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:859: EXPECT_EQ(1,
buckets[0].count);
I'm a wee bit hesitant towards testing the implementation details of a UMA
that's recorded in an entirely different directory :-(. Is there some way we
could convert this test to a unit test living in the Budget Service?

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

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:256: // If
the service was allowed to issue a silent push, just return.
service -> |origin|

[harkness, 2016-09-06 13:28:40]

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:88: double total = 0.0;
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> nit: s/0.0/0/

Done.

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:162: 
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> Can we DCHECK_EQ(0, total)?

Done.

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:176: // Get the current SES
score, to generate UMA..
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> nit: .. -> .

Done.

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:197:
TEST_F(BudgetDatabaseTest, GetBudgetNegativeTime) {
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> Could you lead this test's body with a brief comment explaining why we're
going
> back in time here?

Done.

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_manager.cc (right):

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager.cc:27: // information still there,
clear it.
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> // TODO(harkness): Remove once Chrome 56 has branched.

Done.

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_manager.cc:67: // Just pass the call into
the database.
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> Does this comment add sufficient value? It's exactly what the next call does.

Done.

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

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:859: EXPECT_EQ(1,
buckets[0].count);
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> I'm a wee bit hesitant towards testing the implementation details of a UMA
> that's recorded in an entirely different directory :-(. Is there some way we
> could convert this test to a unit test living in the Budget Service?

I added tests in budget_database_unittest.cc and removed these.

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

https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:256: // If
the service was allowed to issue a silent push, just return.
On 2016/09/05 14:39:03, Peter Beverloo wrote:
> service -> |origin|

Done.

https://codereview.chromium.org/2281673002/diff/80001/chrome/browser/budget_s...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/80001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:279: // Now, cleanup any
expired budget chunks for the origin.
After I added the better tests for the UMA, I found that because we were
recording budget before the expiration cleanup, we were getting invalid values.
Switching the order here and adding a check for IsCached in CleanupExpiredBudget
solves this.

[harkness, 2016-09-06 13:32:46]

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

[commit-bot: I haz the power, 2016-09-06 13:32:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 14:13:40]

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

[commit-bot: I haz the power, 2016-09-06 14:13:41]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-09-07 16:07:29]

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

[commit-bot: I haz the power, 2016-09-07 16:08:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 16:52:07]

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

[commit-bot: I haz the power, 2016-09-07 16:52:08]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-09-07 16:56:35]

lgtm % the boolean thing

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
chrome/browser/budget_service/budget_database.cc:87: double
BudgetDatabase::GetBudget(const GURL& origin) {
optional nit: could be const

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
chrome/browser/budget_service/budget_database_unittest.cc:60: (error ==
blink::mojom::BudgetServiceErrorType::NONE) ? true : false;
Why do you do this? The result of a comparison already is a boolean.

[harkness, 2016-09-07 17:10:29]

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
File chrome/browser/budget_service/budget_database.cc (right):

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
chrome/browser/budget_service/budget_database.cc:87: double
BudgetDatabase::GetBudget(const GURL& origin) {
On 2016/09/07 16:56:35, Peter Beverloo wrote:
> optional nit: could be const

Acknowledged.

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
File chrome/browser/budget_service/budget_database_unittest.cc (right):

https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_...
chrome/browser/budget_service/budget_database_unittest.cc:60: (error ==
blink::mojom::BudgetServiceErrorType::NONE) ? true : false;
On 2016/09/07 16:56:35, Peter Beverloo wrote:
> Why do you do this? The result of a comparison already is a boolean.

I was mis-reading a code style rule, updated it.

[harkness, 2016-09-07 17:10:36]

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

[commit-bot: I haz the power, 2016-09-07 17:10:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 18:49:49]

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

[commit-bot: I haz the power, 2016-09-07 18:49:50]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-09-08 08:48:31]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-09-08 08:48:32]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2281673002/#ps120001
(title: "nits")

[commit-bot: I haz the power, 2016-09-08 08:48:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 09:17:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-08 09:20:47]

Description was changed from

==========
Full hookup of BudgetManager interfaces to BudgetDatabase.

This includes changing the GetBudgetDetails on BudgetDatabase to use the mojo
types and plumbing the result through the BudgetManager. It pulls out all of
the old implementation in BudetManager that used the prefs to store data
(although cleanup of that isn't quite complete). It also replaces the old
GetBudget/StoreBudget calls in the push messaging system with the new Consume
call.

Tests were added in BudgetManager for the new reserve functionality, and any
tests of GetBudget that were still applicable were moved from BudgetManager
to BudgetDatabase.

BUG=617971
==========

to

==========
Full hookup of BudgetManager interfaces to BudgetDatabase.

This includes changing the GetBudgetDetails on BudgetDatabase to use the mojo
types and plumbing the result through the BudgetManager. It pulls out all of
the old implementation in BudetManager that used the prefs to store data
(although cleanup of that isn't quite complete). It also replaces the old
GetBudget/StoreBudget calls in the push messaging system with the new Consume
call.

Tests were added in BudgetManager for the new reserve functionality, and any
tests of GetBudget that were still applicable were moved from BudgetManager
to BudgetDatabase.

BUG=617971

Committed: https://crrev.com/6f6b4143aa82b7876a8a0678e2cabb7074031ee5
Cr-Commit-Position: refs/heads/master@{#417236}
==========

[commit-bot: I haz the power, 2016-09-08 09:20:48]

Patchset 7 (id:??) landed as
https://crrev.com/6f6b4143aa82b7876a8a0678e2cabb7074031ee5
Cr-Commit-Position: refs/heads/master@{#417236}