Start plumbing connections from the BudgetManager to the BudgetDatabase

Start plumbing connections from the BudgetManager to the BudgetDatabase

This adds the BudgetDatabase as a member of the manager, paving the way
for the switch from using the prefs to store budget to using the proto
database.

This also adds Reserve and Consume entrypoints on the BudgetManager.
Reserve is the entrypoint for the Mojo service to enable Javascript to
purchase reservations with budget. Consume is the entrypoint for clients
of the Manager to determine if an origin should be allowed an action,
either because it has a reservation or because it has enough budget to
cover the cost.

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

[harkness, 2016-08-24 21:56:32]

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

[harkness, 2016-08-24 21:56:33]

First, sorry that there are a lot of lines of diffs. By the time I was done, I
ended up moving a lot of code in budget_database so that functions made more
sense to me.

There is now a SyncCache call, similar to the LazyInitialize we discussed.

Tests in budget_database_unittests.cc still need to be updated, but I wanted to
get the code to you so you could start taking a look at the overall structure.

[Peter Beverloo, 2016-08-25 15:31:19]

lgtm % comments

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

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:63: // First, synchronize the
cache.
nit: drop. This is implied by the fact that you call "SyncCache" that take a
callback named "GetBudgetAfterSync". (Dito on line 73.)

Should that be called GetBudgetDetailsAfterSync by the way?

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:67: SyncCache(origin,
cache_callback);
nit: just inline the callback, no need to store it in a local first. (Dito on
line 77.)

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:127: bool success) {
nit: can this be `const` now?

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:161: callback.Run(false);
nit (here & lines 172, 256): false -> false /* success */

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:182: ++iter;
nit: delete, this doesn't do anything since you `break`

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

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:155:
clock->SetNow(starting_time);
This is a bit of a weird paradigm— by adding values in the future and then
resetting the clock "back to the past", this test is now in a state which is
unlikely for production code to reach. (Unless the user messages with their
clock.)

What is significant about resetting the clock?

[harkness, 2016-08-25 16:36:13]

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

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:63: // First, synchronize the
cache.
On 2016/08/25 15:31:19, Peter Beverloo wrote:
> nit: drop. This is implied by the fact that you call "SyncCache" that take a
> callback named "GetBudgetAfterSync". (Dito on line 73.)
> 
> Should that be called GetBudgetDetailsAfterSync by the way?

Done.

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:67: SyncCache(origin,
cache_callback);
On 2016/08/25 15:31:19, Peter Beverloo wrote:
> nit: just inline the callback, no need to store it in a local first. (Dito on
> line 77.)

Done.

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:127: bool success) {
On 2016/08/25 15:31:19, Peter Beverloo wrote:
> nit: can this be `const` now?

It can't because we use the [] operator on the map, which could insert an
element. It won't because there should be an entry in the cache after
SyncCache(), but it theoretically could.

Actually, this made me realize that there WAS a way it could still not be in the
cache, if it had a score of 0, because of that last little change I made to not
give sites a 0 sized chunk. I've removed that now, it was just going to
complicate things, and if a site has 0 engagement, it shouldn't get budget.

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:161: callback.Run(false);
On 2016/08/25 15:31:19, Peter Beverloo wrote:
> nit (here & lines 172, 256): false -> false /* success */

Done.

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database.cc:182: ++iter;
On 2016/08/25 15:31:19, Peter Beverloo wrote:
> nit: delete, this doesn't do anything since you `break`

Done.

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

https://codereview.chromium.org/2272563005/diff/60001/chrome/browser/budget_s...
chrome/browser/budget_service/budget_database_unittest.cc:155:
clock->SetNow(starting_time);
On 2016/08/25 15:31:19, Peter Beverloo wrote:
> This is a bit of a weird paradigm— by adding values in the future and then
> resetting the clock "back to the past", this test is now in a state which is
> unlikely for production code to reach. (Unless the user messages with their
> clock.)
> 
> What is significant about resetting the clock?

That's actually an artifact from when chunks partially decayed over time. The
only times we could be sure of values for testing was at the start. Now that we
have discrete chunks that expire entirely at a fixed time, I can remove this.

[harkness, 2016-08-25 17:40:05]

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

[commit-bot: I haz the power, 2016-08-25 17:40:33]

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-25 19:11:55]

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

[commit-bot: I haz the power, 2016-08-25 19:11:56]

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...)

[harkness, 2016-09-02 12:57:21]

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

[commit-bot: I haz the power, 2016-09-02 12:57:34]

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 14:02:15]

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

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

Dry run: This issue passed the CQ dry run.

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

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-09-02 14:05:20]

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/2272563005/#ps100001
(title: "Fix the task runner")

[commit-bot: I haz the power, 2016-09-02 14:05:35]

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 14:09:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-02 14:10:37]

Description was changed from

==========
Start plumbing connections from the BudgetManager to the BudgetDatabase

This adds the BudgetDatabase as a member of the manager, paving the way
for the switch from using the prefs to store budget to using the proto
database.

This also adds Reserve and Consume entrypoints on the BudgetManager.
Reserve is the entrypoint for the Mojo service to enable Javascript to
purchase reservations with budget. Consume is the entrypoint for clients
of the Manager to determine if an origin should be allowed an action,
either because it has a reservation or because it has enough budget to
cover the cost.

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

to

==========
Start plumbing connections from the BudgetManager to the BudgetDatabase

This adds the BudgetDatabase as a member of the manager, paving the way
for the switch from using the prefs to store budget to using the proto
database.

This also adds Reserve and Consume entrypoints on the BudgetManager.
Reserve is the entrypoint for the Mojo service to enable Javascript to
purchase reservations with budget. Consume is the entrypoint for clients
of the Manager to determine if an origin should be allowed an action,
either because it has a reservation or because it has enough budget to
cover the cost.

BUG=617971

Committed: https://crrev.com/c3fb045f4ff19a65873bbcce47ffca0dd1a3b570
Cr-Commit-Position: refs/heads/master@{#416260}
==========

[commit-bot: I haz the power, 2016-09-02 14:10:38]

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