Connect Mojo budget_service to BudgetManager implementation of Reserve.

Connect Mojo budget_service to BudgetManager implementation of Reserve.

This connects the Mojo service to the pre-existing implementation of
Reserve. It also updates the Reserve implementation to track both whether
the origin had enough budget to spend as well as whether there was an
error in reading or writing the database. Tests were updated to reflect
the new parameters, but no new tests were needed.

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

[harkness, 2016-09-08 15:35:33]

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

[harkness, 2016-09-08 15:35:33]

I did seriously consider changing the Mojo enum to include values for the
Reserve success boolean, but I would still need the extra layer of callbacks for
the WriteCachedValuesToDatabase in order to translate from bool to
DATABASE_ERROR, so it wouldn't save much code clarity. I decided that the code
gains wouldn't be worth the confusion of having an enum for multiple calls with
values which are only for a single call.

[Peter Beverloo, 2016-09-08 17:16:06]

lgtm % comments

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

https://codereview.chromium.org/2326523003/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:104: // If the database read
failed, or there's nothing to add to the cache.
nit: This sentence feels incomplete now?

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

https://codereview.chromium.org/2326523003/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.h:42: using SpendBudgetCallback =
blink::mojom::BudgetService::ReserveCallback;
We should probably just redefine the callback format. A reader interested in
spending budget now suddenly has to look up what reserving budget means (but I
want to spend it!), which lives in a generated file.

That'd also save you from having to explain the arguments in English, as you do
above.

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

https://codereview.chromium.org/2326523003/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.cc:116: if (error !=
blink::mojom::BudgetServiceErrorType::NONE || !success) {
Let's remove "!success" from this conditional, so that it'll solely handle the
"a database error occurred" case. The "database's happy, but no budget" case is
captured by line 120 already.

[harkness, 2016-09-13 13:41:29]

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

https://codereview.chromium.org/2326523003/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.cc:104: // If the database read
failed, or there's nothing to add to the cache.
On 2016/09/08 17:16:06, Peter Beverloo wrote:
> nit: This sentence feels incomplete now?

Done.

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

https://codereview.chromium.org/2326523003/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_database.h:42: using SpendBudgetCallback =
blink::mojom::BudgetService::ReserveCallback;
On 2016/09/08 17:16:06, Peter Beverloo wrote:
> We should probably just redefine the callback format. A reader interested in
> spending budget now suddenly has to look up what reserving budget means (but I
> want to spend it!), which lives in a generated file.
> 
> That'd also save you from having to explain the arguments in English, as you
do
> above.

Done.

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

https://codereview.chromium.org/2326523003/diff/1/chrome/browser/budget_servi...
chrome/browser/budget_service/budget_manager.cc:116: if (error !=
blink::mojom::BudgetServiceErrorType::NONE || !success) {
On 2016/09/08 17:16:06, Peter Beverloo wrote:
> Let's remove "!success" from this conditional, so that it'll solely handle the
> "a database error occurred" case. The "database's happy, but no budget" case
is
> captured by line 120 already.

Good point. Updated.

[harkness, 2016-09-13 13:41:38]

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

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

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

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

[commit-bot: I haz the power, 2016-09-13 13:43:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[harkness, 2016-09-13 14:02:54]

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

[commit-bot: I haz the power, 2016-09-13 14:03:15]

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

[Peter Beverloo, 2016-09-13 14:28:19]

still lgtm, thanks!

[Peter Beverloo, 2016-09-13 14:28:24]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2016-09-13 14:28:44]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2016-09-13 14:29:03]

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-13 14:43:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-13 14:47:17]

Description was changed from

==========
Connect Mojo budget_service to BudgetManager implementation of Reserve.

This connects the Mojo service to the pre-existing implementation of
Reserve. It also updates the Reserve implementation to track both whether
the origin had enough budget to spend as well as whether there was an
error in reading or writing the database. Tests were updated to reflect
the new parameters, but no new tests were needed.

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

to

==========
Connect Mojo budget_service to BudgetManager implementation of Reserve.

This connects the Mojo service to the pre-existing implementation of
Reserve. It also updates the Reserve implementation to track both whether
the origin had enough budget to spend as well as whether there was an
error in reading or writing the database. Tests were updated to reflect
the new parameters, but no new tests were needed.

BUG=617971

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

[commit-bot: I haz the power, 2016-09-13 14:47:18]

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