Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(98)

Issue 2281673002: Full hookup of BudgetManager interfaces to BudgetDatabase. (Closed)

Created:
4 years, 3 months ago by harkness
Modified:
4 years, 3 months ago
Reviewers:
Peter Beverloo
CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, harkness+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@manager
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 19

Patch Set 2 : Added prefs cleanup and code review nits #

Patch Set 3 : Merge branch 'manager' into connections #

Patch Set 4 : Fixed hang and cleaned up browsertests #

Total comments: 16

Patch Set 5 : Moved UMA tests to budget_database_unittest and fixed code review nits. #

Total comments: 1

Patch Set 6 : Rebase and integrate with Reserve plumbing and new error type parameter. #

Total comments: 4

Patch Set 7 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -537 lines) Patch
M chrome/browser/budget_service/budget_database.h View 1 2 3 4 5 6 4 chunks +6 lines, -21 lines 0 comments Download
M chrome/browser/budget_service/budget_database.cc View 1 2 3 4 5 6 9 chunks +59 lines, -26 lines 0 comments Download
M chrome/browser/budget_service/budget_database_unittest.cc View 1 2 3 4 5 6 6 chunks +159 lines, -29 lines 0 comments Download
M chrome/browser/budget_service/budget_manager.h View 3 chunks +3 lines, -20 lines 0 comments Download
M chrome/browser/budget_service/budget_manager.cc View 1 2 3 4 5 5 chunks +13 lines, -147 lines 0 comments Download
M chrome/browser/budget_service/budget_manager_unittest.cc View 1 5 chunks +26 lines, -196 lines 0 comments Download
M chrome/browser/budget_service/budget_service_impl.cc View 1 2 3 4 5 1 chunk +12 lines, -3 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 1 2 3 4 3 chunks +0 lines, -35 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_notification_manager.h View 2 chunks +4 lines, -11 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_notification_manager.cc View 1 2 3 4 2 chunks +13 lines, -49 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 39 (28 generated)
harkness
https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_service/budget_database.cc#newcode29 chrome/browser/budget_service/budget_database.cc:29: // This is 10 days = 240 hours. I ...
4 years, 3 months ago (2016-08-25 19:51:19 UTC) #2
Peter Beverloo
https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_service/budget_database.cc#newcode7 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_service/budget_database.cc#newcode29 chrome/browser/budget_service/budget_database.cc:29: // This is ...
4 years, 3 months ago (2016-08-26 14:56:44 UTC) #7
harkness
https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/1/chrome/browser/budget_service/budget_database.cc#newcode7 chrome/browser/budget_service/budget_database.cc:7: #include "base/containers/adapters.h" On 2016/08/26 14:56:43, Peter Beverloo wrote: > ...
4 years, 3 months ago (2016-08-31 13:15:36 UTC) #8
harkness
PTAL at modifications to push_messaging_browsertest.cc.
4 years, 3 months ago (2016-09-05 13:58:57 UTC) #17
Peter Beverloo
lgtm % some nits https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_service/budget_database.cc#newcode88 chrome/browser/budget_service/budget_database.cc:88: double total = 0.0; nit: ...
4 years, 3 months ago (2016-09-05 14:39:03 UTC) #18
harkness
https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/60001/chrome/browser/budget_service/budget_database.cc#newcode88 chrome/browser/budget_service/budget_database.cc:88: double total = 0.0; On 2016/09/05 14:39:03, Peter Beverloo ...
4 years, 3 months ago (2016-09-06 13:28:40 UTC) #19
Peter Beverloo
lgtm % the boolean thing https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_service/budget_database.cc#newcode87 chrome/browser/budget_service/budget_database.cc:87: double BudgetDatabase::GetBudget(const GURL& origin) ...
4 years, 3 months ago (2016-09-07 16:56:35 UTC) #28
harkness
https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_service/budget_database.cc File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2281673002/diff/100001/chrome/browser/budget_service/budget_database.cc#newcode87 chrome/browser/budget_service/budget_database.cc:87: double BudgetDatabase::GetBudget(const GURL& origin) { On 2016/09/07 16:56:35, Peter ...
4 years, 3 months ago (2016-09-07 17:10:29 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2281673002/120001
4 years, 3 months ago (2016-09-08 08:48:45 UTC) #36
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 3 months ago (2016-09-08 09:17:46 UTC) #37
commit-bot: I haz the power
4 years, 3 months ago (2016-09-08 09:20:48 UTC) #39
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/6f6b4143aa82b7876a8a0678e2cabb7074031ee5
Cr-Commit-Position: refs/heads/master@{#417236}

Powered by Google App Engine
This is Rietveld 408576698