Adding more functionality to BudgetDatabase.

Adding more functionality to BudgetDatabase.

The BudgetDatabase now has a clock, which it uses to timestamp budget chunks
with expiration times when an AddBudget is called.  (The clock from
BackgroundBudgetService will be removed when it switches to the BudgetDatabase.)

The unit tests were converted to use the new AddBudget instead of SetValue, and
SetValue and GetValue (which did not use the new cache scheme) were removed.

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

[harkness, 2016-07-28 16:07:41]

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

[Peter Beverloo, 2016-08-01 17:33:09]

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:26: constexpr double
kBudgetDurationInHours = 72.0;
nit: no ".0"

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:73: // having first queried for
the existing budget.
nit: try to avoid "we"

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:77: base::Time now =
now + 72 hours != |now| :-)

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:80: std::make_pair(amount,
now.ToInternalValue()));
"info_iter->second.second" is rather unreadable.

How about the following:

  (1) swap the DCHECK with DCHECK_GT(budget_map_.count(origin.spec()), 0);
  (2) Just access budget_map_[origin.spec()] directly here.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:101:
keys_to_remove->push_back(origin.spec());
Let's remove this case until there is a ClearBudget() method? It's unreachable
code right now.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:140: double debt =
budget_proto->has_debt() ? budget_proto->debt() : 0;
DCHECK(budget_proto->has_debt())?

When wouldn't it have debt? (You always set it on line 106.)

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:178: // Override the default
clock with the specified clock. Only used for testing.
nit: redundant with header file

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.h:56: void AddBudget(const GURL&
origin,
+docs

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.h:67: using BudgetChunks =
std::list<std::pair<double, double>>;
Merge error or deliberate?

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database_unittest.cc:22: const double
kDefaultExpirationInHours = 72.0;
nit: no ".0"

[harkness, 2016-08-03 11:17:48]

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:26: constexpr double
kBudgetDurationInHours = 72.0;
On 2016/08/01 17:33:08, Peter Beverloo wrote:
> nit: no ".0"

Done.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:73: // having first queried for
the existing budget.
On 2016/08/01 17:33:08, Peter Beverloo wrote:
> nit: try to avoid "we"

Done.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:77: base::Time now =
On 2016/08/01 17:33:09, Peter Beverloo wrote:
> now + 72 hours != |now| :-)

tsk tsk.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:80: std::make_pair(amount,
now.ToInternalValue()));
On 2016/08/01 17:33:09, Peter Beverloo wrote:
> "info_iter->second.second" is rather unreadable.
> 
> How about the following:
> 
>   (1) swap the DCHECK with DCHECK_GT(budget_map_.count(origin.spec()), 0);
>   (2) Just access budget_map_[origin.spec()] directly here.

Good idea, updated.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:101:
keys_to_remove->push_back(origin.spec());
On 2016/08/01 17:33:08, Peter Beverloo wrote:
> Let's remove this case until there is a ClearBudget() method? It's unreachable
> code right now.

This case is possible once expiration is around, which is just the next CL. I'd
rather leave it in unless you feel strongly about it.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:140: double debt =
budget_proto->has_debt() ? budget_proto->debt() : 0;
On 2016/08/01 17:33:08, Peter Beverloo wrote:
> DCHECK(budget_proto->has_debt())?
> 
> When wouldn't it have debt? (You always set it on line 106.)

That's true, maybe I was just being overly cautious. Technically in the proto,
debt is an optional, so I was being defensive.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:178: // Override the default
clock with the specified clock. Only used for testing.
On 2016/08/01 17:33:09, Peter Beverloo wrote:
> nit: redundant with header file

Generally when a method has a gotcha like "SHould only be used for testing", I
prefer extra commenting. Do you really feel that having the comment there hurts
the code?

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.h:56: void AddBudget(const GURL&
origin,
On 2016/08/01 17:33:09, Peter Beverloo wrote:
> +docs

Done.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.h:67: using BudgetChunks =
std::list<std::pair<double, double>>;
On 2016/08/01 17:33:09, Peter Beverloo wrote:
> Merge error or deliberate?

oops, merge error.

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database_unittest.cc:22: const double
kDefaultExpirationInHours = 72.0;
On 2016/08/01 17:33:09, Peter Beverloo wrote:
> nit: no ".0"

Done.

[Peter Beverloo, 2016-08-03 13:40:12]

lgtm % conditional to DCHECK

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:101:
keys_to_remove->push_back(origin.spec());
On 2016/08/03 11:17:48, harkness wrote:
> On 2016/08/01 17:33:08, Peter Beverloo wrote:
> > Let's remove this case until there is a ClearBudget() method? It's
unreachable
> > code right now.
> 
> This case is possible once expiration is around, which is just the next CL.
I'd
> rather leave it in unless you feel strongly about it.

Ok. In general, try to minimize unused code :).

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:140: double debt =
budget_proto->has_debt() ? budget_proto->debt() : 0;
On 2016/08/03 11:17:47, harkness wrote:
> On 2016/08/01 17:33:08, Peter Beverloo wrote:
> > DCHECK(budget_proto->has_debt())?
> > 
> > When wouldn't it have debt? (You always set it on line 106.)
> 
> That's true, maybe I was just being overly cautious. Technically in the proto,
> debt is an optional, so I was being defensive.

So let's have a DCHECK() and continue on that assumption?

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:178: // Override the default
clock with the specified clock. Only used for testing.
On 2016/08/03 11:17:47, harkness wrote:
> On 2016/08/01 17:33:09, Peter Beverloo wrote:
> > nit: redundant with header file
> 
> Generally when a method has a gotcha like "SHould only be used for testing", I
> prefer extra commenting. Do you really feel that having the comment there
hurts
> the code?

It's doubly redundant with the comment in the header file and the *ForTesting
suffix in the method name. I don't think it's adding any value. Up to you.

[harkness, 2016-08-08 12:59:35]

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

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:140: double debt =
budget_proto->has_debt() ? budget_proto->debt() : 0;
On 2016/08/03 13:40:11, Peter Beverloo wrote:
> On 2016/08/03 11:17:47, harkness wrote:
> > On 2016/08/01 17:33:08, Peter Beverloo wrote:
> > > DCHECK(budget_proto->has_debt())?
> > > 
> > > When wouldn't it have debt? (You always set it on line 106.)
> > 
> > That's true, maybe I was just being overly cautious. Technically in the
proto,
> > debt is an optional, so I was being defensive.
> 
> So let's have a DCHECK() and continue on that assumption?

Done.

https://codereview.chromium.org/2188953004/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:178: // Override the default
clock with the specified clock. Only used for testing.
On 2016/08/03 13:40:11, Peter Beverloo wrote:
> On 2016/08/03 11:17:47, harkness wrote:
> > On 2016/08/01 17:33:09, Peter Beverloo wrote:
> > > nit: redundant with header file
> > 
> > Generally when a method has a gotcha like "SHould only be used for testing",
I
> > prefer extra commenting. Do you really feel that having the comment there
> hurts
> > the code?
> 
> It's doubly redundant with the comment in the header file and the *ForTesting
> suffix in the method name. I don't think it's adding any value. Up to you.

Done.

[harkness, 2016-08-08 12:59:44]

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

[commit-bot: I haz the power, 2016-08-08 13:00:03]

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-08 13:45:50]

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

[commit-bot: I haz the power, 2016-08-08 13:45:51]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-08-08 15:37:58]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-08-08 15:37:58]

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/2188953004/#ps40001
(title: "Added DCHECK and removed comment.")

[commit-bot: I haz the power, 2016-08-08 15:38:19]

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-08 15:44:20]

Description was changed from

==========
Adding more functionality to BudgetDatabase.

The BudgetDatabase now has a clock, which it uses to timestamp budget chunks
with expiration times when an AddBudget is called.  (The clock from
BackgroundBudgetService will be removed when it switches to the BudgetDatabase.)

The unit tests were converted to use the new AddBudget instead of SetValue, and
SetValue and GetValue (which did not use the new cache scheme) were removed.

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

to

==========
Adding more functionality to BudgetDatabase.

The BudgetDatabase now has a clock, which it uses to timestamp budget chunks
with expiration times when an AddBudget is called.  (The clock from
BackgroundBudgetService will be removed when it switches to the BudgetDatabase.)

The unit tests were converted to use the new AddBudget instead of SetValue, and
SetValue and GetValue (which did not use the new cache scheme) were removed.

BUG=617971

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

[commit-bot: I haz the power, 2016-08-08 15:44:21]

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