Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2173833002:
Expand the functionality of the BudgetDatabase. (Closed)
|
Created:
4 years, 5 months ago by harkness Modified:
4 years, 4 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpand the functionality of the BudgetDatabase.
Adds a GetBudgetDetails call which returns full information about the
expected budget at future times.
Also adds a cache layer so that multiple GetBudgetDetails calls can't
hammer the filesystem I/O.
BUG=617971
Committed: https://crrev.com/804b612a628958aef2f1f21fdda3c56c827cb2cb
Cr-Commit-Position: refs/heads/master@{#408104}
Patch Set 1 #Patch Set 2 : Reverted change I decided against. #
Total comments: 33
Patch Set 3 : Code review comments #
Total comments: 28
Patch Set 4 : More code review cleanup #
Total comments: 4
Patch Set 5 : Final cleanup #
Messages
Total messages: 14 (4 generated)
harkness@chromium.org changed reviewers: + peter@chromium.org
To what extend will BudgetDatabase have to follow the API, i.e. how will BudgetService be using these values? https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:34: // TODO(harkness): Either remove this method or add caching. Based on what will you make this decision? Dito re: the new TODO on line 46. Together with the TODO on line 81, it seems to me like the three options you're considering are: (1) Never cache, always cause file I/O (current impl) (2) Lazily cache, cause file I/O on the first request (3) Eagerly cache, cause file I/O on database load and writes Right now there's a mixture of (1) and (2), which is not great. In respect to (2), we're not expiring the cached entries either, which means memory usage may approximate (3) over time. This is probably a discussion worth having in person :-). https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:69: ReturnBudgetDetails(origin, callback, true); This is a risk of prefixing your method names with "Return" - control flow in the method will continue after this, so |callback| will actually be invoked twice. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:91: callback.Run(success); Control flow still continues after this check, so we may theoretically be working with corrupt information in the |budget_proto|, access a null pointer or...? https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: } Since you're clearing the |info| after getting it, wouldn't this code (:93 until :104) be more elegant when written as follows? ===== BudgetChunks chunks; for (const auto& chunk : budget_proto->budget()) chunks.push_back(std::make_pair(chunk.amount(), chunk.expiration())); budget_map_[origin.spec()] = std::make_pair(budget_proto->debt(), std::move(chunks)); ===== https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:108: void BudgetDatabase::ReturnBudgetDetails( suggestion: DidGetBudgetChunks? https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:115: callback.Run(success, 0.0, BudgetExpectation()); dito - control flow still continues... https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:115: callback.Run(success, 0.0, BudgetExpectation()); nit: s/0.0/0/, elsewhere too https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:129: expectation.push_front(std::pair<double, double>(total, iter->second)); std::make_pair https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:135: expectation.push_front(std::pair<double, double>(total, 0)); std::make_pair https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:32: typedef std::list<std::pair<double, double>> BudgetChunks; std::vector? I don't think any of the list-over-vector benefits applies. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:33: typedef std::pair<double, BudgetChunks> BudgetInfo; It looks like both BudgetChunks and BudgetInfo can be private? https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:36: typedef std::list<std::pair<double, double>> BudgetExpectation; All three of these should be using `using`, as you do on lines 39, 44, 47 and 50. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:50: using AddToCacheCallback = base::Callback<void(bool success)>; private https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:86: base::hash_map<std::string, BudgetInfo> budget_map_; base::hash_map<> is deprecated, please use std::unordered_map<> instead.
https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:34: // TODO(harkness): Either remove this method or add caching. On 2016/07/22 16:14:51, Peter Beverloo wrote: > Based on what will you make this decision? Dito re: the new TODO on line 46. > > Together with the TODO on line 81, it seems to me like the three options you're > considering are: > > (1) Never cache, always cause file I/O (current impl) > (2) Lazily cache, cause file I/O on the first request > (3) Eagerly cache, cause file I/O on database load and writes > > Right now there's a mixture of (1) and (2), which is not great. In respect to > (2), we're not expiring the cached entries either, which means memory usage may > approximate (3) over time. > > This is probably a discussion worth having in person :-). Discussed in person, this will be a lazy system. The methods which are still using the direct I/O will be removed once their replacement is available. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:69: ReturnBudgetDetails(origin, callback, true); On 2016/07/22 16:14:51, Peter Beverloo wrote: > This is a risk of prefixing your method names with "Return" - control flow in > the method will continue after this, so |callback| will actually be invoked > twice. Nope, this is just me being stupid because I have to keep reminding myself that we're working with callbacks. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:91: callback.Run(success); On 2016/07/22 16:14:51, Peter Beverloo wrote: > Control flow still continues after this check, so we may theoretically be > working with corrupt information in the |budget_proto|, access a null pointer > or...? or just being stupid. :) https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: } On 2016/07/22 16:14:52, Peter Beverloo wrote: > Since you're clearing the |info| after getting it, wouldn't this code (:93 until > :104) be more elegant when written as follows? > > ===== > BudgetChunks chunks; > for (const auto& chunk : budget_proto->budget()) > chunks.push_back(std::make_pair(chunk.amount(), chunk.expiration())); > > budget_map_[origin.spec()] = > std::make_pair(budget_proto->debt(), std::move(chunks)); > ===== Yes, the two have equivalent effects. In terms of elegance, I have a preference for seeing things broken out in a more explicit style, but I recognize that's a more old-fashioned approach, so I'm willing to change it if you prefer. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:108: void BudgetDatabase::ReturnBudgetDetails( On 2016/07/22 16:14:51, Peter Beverloo wrote: > suggestion: DidGetBudgetChunks? I went with DidGetBudget https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:115: callback.Run(success, 0.0, BudgetExpectation()); On 2016/07/22 16:14:51, Peter Beverloo wrote: > dito - control flow still continues... Acknowledged. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:115: callback.Run(success, 0.0, BudgetExpectation()); On 2016/07/22 16:14:51, Peter Beverloo wrote: > nit: s/0.0/0/, elsewhere too Done. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:129: expectation.push_front(std::pair<double, double>(total, iter->second)); On 2016/07/22 16:14:51, Peter Beverloo wrote: > std::make_pair Done. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:135: expectation.push_front(std::pair<double, double>(total, 0)); On 2016/07/22 16:14:51, Peter Beverloo wrote: > std::make_pair Done. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:32: typedef std::list<std::pair<double, double>> BudgetChunks; On 2016/07/22 16:14:52, Peter Beverloo wrote: > std::vector? I don't think any of the list-over-vector benefits applies. It doesn't really matter for my purposes. Access will only be sequential, but I can change it. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:33: typedef std::pair<double, BudgetChunks> BudgetInfo; On 2016/07/22 16:14:52, Peter Beverloo wrote: > It looks like both BudgetChunks and BudgetInfo can be private? Ah yes, I'd planned to talk to you about this. You mentioned in a previous review that all typedefs needed to be at the top of the class, which makes sense, but I thought code style was also to try to have a single public block and a single private block. Here I went ahead and added an extra block - is that what I should be using for chromium style? https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:36: typedef std::list<std::pair<double, double>> BudgetExpectation; On 2016/07/22 16:14:52, Peter Beverloo wrote: > All three of these should be using `using`, as you do on lines 39, 44, 47 and > 50. Done. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:50: using AddToCacheCallback = base::Callback<void(bool success)>; On 2016/07/22 16:14:52, Peter Beverloo wrote: > private Done. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:86: base::hash_map<std::string, BudgetInfo> budget_map_; On 2016/07/22 16:14:52, Peter Beverloo wrote: > base::hash_map<> is deprecated, please use std::unordered_map<> instead. Done.
This looks fine, modulo minor nits. Sorry that we care about these things— let's get it in first thing tomorrow :) https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: } On 2016/07/26 16:42:07, harkness wrote: > On 2016/07/22 16:14:52, Peter Beverloo wrote: > > Since you're clearing the |info| after getting it, wouldn't this code (:93 > until > > :104) be more elegant when written as follows? > > > > ===== > > BudgetChunks chunks; > > for (const auto& chunk : budget_proto->budget()) > > chunks.push_back(std::make_pair(chunk.amount(), chunk.expiration())); > > > > budget_map_[origin.spec()] = > > std::make_pair(budget_proto->debt(), std::move(chunks)); > > ===== > > Yes, the two have equivalent effects. > > In terms of elegance, I have a preference for seeing things broken out in a more > explicit style, but I recognize that's a more old-fashioned approach, so I'm > willing to change it if you prefer. Semantically you're replacing the entry in |budget_map_|. In your code, you do this manually by overriding the values (leading to the {first/second} obscurity, also because |info| is a non-const reference which is less obvious than a pointer). In my code I do this by creating a new value entirely, then overriding the previous one. I personally find my suggestion much clearer, but up to you. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:32: typedef std::list<std::pair<double, double>> BudgetChunks; On 2016/07/26 16:42:07, harkness wrote: > On 2016/07/22 16:14:52, Peter Beverloo wrote: > > std::vector? I don't think any of the list-over-vector benefits applies. > > It doesn't really matter for my purposes. Access will only be sequential, but I > can change it. std::vector is more popular in Chromium code by an order of magnitude, so let's prefer consistency? https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:33: typedef std::pair<double, BudgetChunks> BudgetInfo; On 2016/07/26 16:42:07, harkness wrote: > On 2016/07/22 16:14:52, Peter Beverloo wrote: > > It looks like both BudgetChunks and BudgetInfo can be private? > > Ah yes, I'd planned to talk to you about this. You mentioned in a previous > review that all typedefs needed to be at the top of the class, which makes > sense, but I thought code style was also to try to have a single public block > and a single private block. Here I went ahead and added an extra block - is that > what I should be using for chromium style? No, there are at most three blocks, one of each {public, protected, private}, in that order. Within these blocks, however, declarations still have a common declaration order (e.g. type defs on top). These typedefs can simply be the first things in the private block below. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:49: // the cache. I think we decided on this one, so remove the ambiguity from the language? https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:74: weak_ptr_factory_.GetWeakPtr(), origin, callback, true)); true -> true /* success */ https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:138: for (BudgetChunks::const_reverse_iterator iter = info.second.rbegin(); const auto& https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:12: #include "base/containers/hash_tables.h" delete https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:96: return; nit: drop (control flow ends after this anyway) https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:112: double debt_; init (double debt_ = 0;) https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:132: const BudgetDatabase::BudgetExpectation exp = expectation(); nit 1: expectation() returns a reference, so no need to make a copy. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:132: const BudgetDatabase::BudgetExpectation exp = expectation(); nit 2: perfectly fine to use const auto& here. Try to avoid shorter names such as "exp" where possible. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:136: BudgetDatabase::BudgetExpectation::const_iterator iter = exp.begin(); nit: const auto& iter = ...? https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:137: ASSERT_EQ(exp.size(), (const unsigned long)3); nit 1: This ASSERT should be under line 133, it's unrelated to line 136. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:137: ASSERT_EQ(exp.size(), (const unsigned long)3); nit 3: We disallow c-style casts in Chromium. In this case you can use the "U" suffix: 3U https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:137: ASSERT_EQ(exp.size(), (const unsigned long)3); nit 2: Canonical use of the {ASSERT,EXPECT}_EQ macros is (expected, actual), which affects the error messages GTest prints. Dito further down. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:143: EXPECT_EQ(iter->second, 0.0); nit: s/0.0/0/ (dito on line 152)
https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:32: typedef std::list<std::pair<double, double>> BudgetChunks; On 2016/07/26 17:26:20, Peter Beverloo wrote: > On 2016/07/26 16:42:07, harkness wrote: > > On 2016/07/22 16:14:52, Peter Beverloo wrote: > > > std::vector? I don't think any of the list-over-vector benefits applies. > > > > It doesn't really matter for my purposes. Access will only be sequential, but > I > > can change it. > > std::vector is more popular in Chromium code by an order of magnitude, so let's > prefer consistency? I changed BudgetChunks to be a vector, but because we're adding entries onto the front of BudgetExpectation, that needed to stay a list. https://codereview.chromium.org/2173833002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:33: typedef std::pair<double, BudgetChunks> BudgetInfo; On 2016/07/26 17:26:20, Peter Beverloo wrote: > On 2016/07/26 16:42:07, harkness wrote: > > On 2016/07/22 16:14:52, Peter Beverloo wrote: > > > It looks like both BudgetChunks and BudgetInfo can be private? > > > > Ah yes, I'd planned to talk to you about this. You mentioned in a previous > > review that all typedefs needed to be at the top of the class, which makes > > sense, but I thought code style was also to try to have a single public block > > and a single private block. Here I went ahead and added an extra block - is > that > > what I should be using for chromium style? > > No, there are at most three blocks, one of each {public, protected, private}, in > that order. Within these blocks, however, declarations still have a common > declaration order (e.g. type defs on top). > > These typedefs can simply be the first things in the private block below. Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:49: // the cache. On 2016/07/26 17:26:21, Peter Beverloo wrote: > I think we decided on this one, so remove the ambiguity from the language? Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:74: weak_ptr_factory_.GetWeakPtr(), origin, callback, true)); On 2016/07/26 17:26:20, Peter Beverloo wrote: > true -> true /* success */ Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:138: for (BudgetChunks::const_reverse_iterator iter = info.second.rbegin(); On 2016/07/26 17:26:20, Peter Beverloo wrote: > const auto& I believe there are issues with const auto& in this case. I can auto or leave it as is. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:12: #include "base/containers/hash_tables.h" On 2016/07/26 17:26:21, Peter Beverloo wrote: > delete Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:96: return; On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit: drop (control flow ends after this anyway) Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:112: double debt_; On 2016/07/26 17:26:21, Peter Beverloo wrote: > init (double debt_ = 0;) Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:132: const BudgetDatabase::BudgetExpectation exp = expectation(); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit 2: perfectly fine to use const auto& here. Try to avoid shorter names such > as "exp" where possible. Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:132: const BudgetDatabase::BudgetExpectation exp = expectation(); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit 1: expectation() returns a reference, so no need to make a copy. Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:136: BudgetDatabase::BudgetExpectation::const_iterator iter = exp.begin(); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit: const auto& iter = ...? I did some reading on const_iterator and auto, and it doesn't seem like there's a good way to do this. Let's talk about it in person. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:137: ASSERT_EQ(exp.size(), (const unsigned long)3); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit 1: This ASSERT should be under line 133, it's unrelated to line 136. Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:137: ASSERT_EQ(exp.size(), (const unsigned long)3); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit 2: Canonical use of the {ASSERT,EXPECT}_EQ macros is (expected, actual), > which affects the error messages GTest prints. Dito further down. Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:137: ASSERT_EQ(exp.size(), (const unsigned long)3); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit 3: We disallow c-style casts in Chromium. In this case you can use the "U" > suffix: 3U Done. https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:143: EXPECT_EQ(iter->second, 0.0); On 2016/07/26 17:26:21, Peter Beverloo wrote: > nit: s/0.0/0/ (dito on line 152) Done.
lgtm https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:138: for (BudgetChunks::const_reverse_iterator iter = info.second.rbegin(); On 2016/07/27 10:59:02, harkness wrote: > On 2016/07/26 17:26:20, Peter Beverloo wrote: > > const auto& > > I believe there are issues with const auto& in this case. I can auto or leave it > as is. You're right re: const, because we're incrementing. Using `auto` would use a T::reverse_iterator. Sorry. But! It looks like we have //base/containers/adapters.h, so you should be able to write: for (const auto& chunk : base::Reversed(info.second)) { } \o/ https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2173833002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:136: BudgetDatabase::BudgetExpectation::const_iterator iter = exp.begin(); On 2016/07/27 10:59:03, harkness wrote: > On 2016/07/26 17:26:21, Peter Beverloo wrote: > > nit: const auto& iter = ...? > > I did some reading on const_iterator and auto, and it doesn't seem like there's > a good way to do this. Let's talk about it in person. Ugh ok. Could still use `auto` (const-ness doesn't really matter here), or leave as-is. Up to you. https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:143: callback.Run(success, info.first, expectation); nit: success -> true /* success */ Might be a wee bit clearer for the reader (now you have to backtrace to assert that it must be true since we'd have bailed out around line 118). Up to you. https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:131: const auto& expectedValue = expectation(); expected_value
https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:143: callback.Run(success, info.first, expectation); On 2016/07/27 11:21:40, Peter Beverloo wrote: > nit: success -> true /* success */ > > Might be a wee bit clearer for the reader (now you have to backtrace to assert > that it must be true since we'd have bailed out around line 118). Up to you. Done. https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2173833002/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:131: const auto& expectedValue = expectation(); On 2016/07/27 11:21:40, Peter Beverloo wrote: > expected_value Done.
The CQ bit was checked by harkness@chromium.org
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/2173833002/#ps80001 (title: "Final cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Expand the functionality of the BudgetDatabase. Adds a GetBudgetDetails call which returns full information about the expected budget at future times. Also adds a cache layer so that multiple GetBudgetDetails calls can't hammer the filesystem I/O. BUG=617971 ========== to ========== Expand the functionality of the BudgetDatabase. Adds a GetBudgetDetails call which returns full information about the expected budget at future times. Also adds a cache layer so that multiple GetBudgetDetails calls can't hammer the filesystem I/O. BUG=617971 Committed: https://crrev.com/804b612a628958aef2f1f21fdda3c56c827cb2cb Cr-Commit-Position: refs/heads/master@{#408104} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/804b612a628958aef2f1f21fdda3c56c827cb2cb Cr-Commit-Position: refs/heads/master@{#408104} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
