Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255813002:
AddEngagementBudget and SpendBudget added to BudgetDatabase. (Closed)
DescriptionAddEngagementBudget and SpendBudget added to BudgetDatabase.
The BudgetDatabase already had a basic AddBudget method which would add a fixed
amount of budget. This CL adds an engagement based budget increment which takes the
SES score and adds a percentage of the score to the budget based on when the last
award grant was added. For instance, if the budget was last added a day ago, then
AddEngagementBudget will add 1/3 of the SES score to the budget. This functionality
required adding another parameter to the database, which is the last time the
budget was incremented with engagement-based budget.
This CL also adds SpendBudget, which will reduce the budget by the amount requested
if it is available, and will invoke the callback with false otherwise.
BUG=617971
Committed: https://crrev.com/e0e2609b94ad995b18286f1e82cb7cb41c926708
Cr-Commit-Position: refs/heads/master@{#413995}
Patch Set 1 #Patch Set 2 : Added expiration check in SpendBudget #
Total comments: 52
Patch Set 3 : Code review comments #
Total comments: 20
Patch Set 4 : Cleanup #
Messages
Total messages: 17 (8 generated)
harkness@chromium.org changed reviewers: + johnme@chromium.org, peter@chromium.org
https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:19: optional int64 last_updated = 2; Nit: rename this to clarify that only engagement awards update this value; other budget grants do not. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:29: optional int64 expiration = 2; Optional: instead of storing expiration times for each BudgetChunk and a per-origin last_updated, you could just store grant times for each BudgetChunk. I'm not sure if that makes things harder or easier. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:34: chunks = std::move(other.chunks); Doesn't make sense to std::move in a copy constructor, since that leaves other.chunks in an invalid state. Did you mean for this to be a move constructor (const BudgetInfo&& other) instead? If so, you probably also want to use the DISALLOW_COPY_AND_ASSIGN macro to mark it move-only. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:91: double sesScore, Nit: parameter name is different in .h file; it's slightly clearer if they're the same. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:101: if (IsCached(origin)) { Nit: each call to AddEngagementBudget causes origin to searched for in budget_map_ 8 times (including string hashing each time). I can't see obvious ways of optimising that without decreasing legibility, since it's spread across various methods, but perhaps something to be aware of in future. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: int elapsedHours = elapsed.InHours(); If AddEngagementBudget gets called more than once per hour, the origin will never get any budget, because elapsedHours will always be zero (since it is rounded down). Use floating point maths for this instead, e.g. |InSecondsF() / 3600.0|? https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:109: // Update the last_engagement_award to the current time. Nit: please reword this comment to clarify that this line sometimes constructs a new BudgetInfo entry in the map, rather than merely updating an existing one https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:131: for (const auto& chunk : budget_map_[origin.spec()].chunks) Nit: You can probably s/auto/BudgetChunk/ here since the type name is short https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:140: auto& info = budget_map_[origin.spec()]; Please move this up so that the for loop above can use it (again, you may as well use the full type name since it's short). https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:144: while (bill > 0 && chunkIter != info.chunks.end()) { Nit: perhaps simpler as a for loop; more efficient anyway since you only rewrite the array (move assign all the elements) once: int chunksToErase = 0; for (BudgetChunk& chunk : info.chunks) { if (chunk.amount > bill) { chunk.amount -= bill break; } bill -= chunk.amount; chunksToErase++; } info.chunks.erase(info.chunks.begin(), info.chunks.begin() + chunksToErase); Or you could make BudgetChunks a std::list instead, then removing elements wouldn't require the array to be rewritten (and there seem to be no downsides, since you never index into it). https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:190: budget_map_[origin.spec()].chunks = std::move(chunks); You could just write the chunks directly into budget_map_[origin.spec()].chunks in the first place. Obviously you'd need to cache a reference to the BudgetInfo rather than doing a string hash lookup every loop iteration, but that's probably not a bad idea anyway. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:256: const BudgetChunks& chunks = budget_map_[origin.spec()].chunks; Nit: store a reference to the BudgetInfo instead. Then you can re-use the reference for set_last_updated. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:48: using BudgetExpectation = std::list<BudgetStatus>; Nit: Can we rename this to BudgetPrediction (in a separate patch if necessary)? Expectation sounds like it's for testing only. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:73: void AddBudget(const GURL& origin, What's the use case for adding non-engagement budget? https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:86: // Spend a particular amount of budget for an origin. The callback specifies Nit: s/specifies/takes/ https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:89: // to the database. This should only be called after the budget has been read. It seems a slightly awkward constraint that "This should only be called after the budget has been read". I'm not even sure what that means - do you have to call GetBudgetDetails and wait for its callback? Could you avoid that using a queue of pending tasks to perform asynchronously once loaded (in the style of GCMDelayedTaskController)? https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:96: const BudgetDatabase::BudgetExpectation& expectation() { Nit: newline before please https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:198: Nit: remove newline, since the ASSERTs below are tied to this call to GetBudgetDetails. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:201: const auto& expected_value = expectation(); Nit: Don't bother caching this reference; the compiler can just inline the expectation() getter. Or just make expectation_ protected and access it directly, like success_? https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:204: ASSERT_EQ(kDefaultEngagement, expectation().begin()->budget_at); s/expectation()/expected_value/ since you've cached a reference to it https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:209: GetBudgetDetails(); Add newline before getBudgetDetails() https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:210: Ditto remove newline https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:214: ASSERT_DOUBLE_EQ(kDefaultEngagement * 4 / 3, Might be nice to re-test the other 2 values too, to make sure they haven't changed. Ditto above, with testing the 2nd value. To make this easier, consider making expectation_ be a std::vector (initialised as a copy of the std::list in GetBudgetDetailsComplete). https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:238: ASSERT_DOUBLE_EQ(kDefaultBudget1 * 3 - 1, expectation().begin()->budget_at); Ditto with checking the other values in expectation()? https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:249: EXPECT_FALSE(SpendBudget(origin, kDefaultBudget1)); After this, check that the budget hasn't changed?
https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget.proto (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:19: optional int64 last_updated = 2; On 2016/08/19 18:48:35, johnme wrote: > Nit: rename this to clarify that only engagement awards update this value; other > budget grants do not. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget.proto:29: optional int64 expiration = 2; On 2016/08/19 18:48:35, johnme wrote: > Optional: instead of storing expiration times for each BudgetChunk and a > per-origin last_updated, you could just store grant times for each BudgetChunk. > I'm not sure if that makes things harder or easier. That wouldn't actually work, since even if a site currently has no budget, you need to keep track of when the next chunk can be awarded. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:34: chunks = std::move(other.chunks); On 2016/08/19 18:48:35, johnme wrote: > Doesn't make sense to std::move in a copy constructor, since that leaves > other.chunks in an invalid state. Did you mean for this to be a move constructor > (const BudgetInfo&& other) instead? If so, you probably also want to use the > DISALLOW_COPY_AND_ASSIGN macro to mark it move-only. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:91: double sesScore, On 2016/08/19 18:48:35, johnme wrote: > Nit: parameter name is different in .h file; it's slightly clearer if they're > the same. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:101: if (IsCached(origin)) { On 2016/08/19 18:48:35, johnme wrote: > Nit: each call to AddEngagementBudget causes origin to searched for in > budget_map_ 8 times (including string hashing each time). I can't see obvious > ways of optimising that without decreasing legibility, since it's spread across > various methods, but perhaps something to be aware of in future. I'll definitely keep it in mind, and see if there are some ways to optimize in the future. I have one idea, but it's out of scope for this review. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: int elapsedHours = elapsed.InHours(); On 2016/08/19 18:48:36, johnme wrote: > If AddEngagementBudget gets called more than once per hour, the origin will > never get any budget, because elapsedHours will always be zero (since it is > rounded down). Use floating point maths for this instead, e.g. |InSecondsF() / > 3600.0|? Actually, I always meant to just return if the time was less than an hour to prevent proliferation of small chunks, I just didn't add the code. I've added it now. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:109: // Update the last_engagement_award to the current time. On 2016/08/19 18:48:35, johnme wrote: > Nit: please reword this comment to clarify that this line sometimes constructs a > new BudgetInfo entry in the map, rather than merely updating an existing one Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:131: for (const auto& chunk : budget_map_[origin.spec()].chunks) On 2016/08/19 18:48:35, johnme wrote: > Nit: You can probably s/auto/BudgetChunk/ here since the type name is short Peter has been pushing for everything to be auto, so I'd rather keep it auto for consistency. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:140: auto& info = budget_map_[origin.spec()]; On 2016/08/19 18:48:36, johnme wrote: > Please move this up so that the for loop above can use it (again, you may as > well use the full type name since it's short). Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:144: while (bill > 0 && chunkIter != info.chunks.end()) { On 2016/08/19 18:48:35, johnme wrote: > Nit: perhaps simpler as a for loop; more efficient anyway since you only rewrite > the array (move assign all the elements) once: > > int chunksToErase = 0; > for (BudgetChunk& chunk : info.chunks) { > if (chunk.amount > bill) { > chunk.amount -= bill > break; > } > bill -= chunk.amount; > chunksToErase++; > } > info.chunks.erase(info.chunks.begin(), > info.chunks.begin() + chunksToErase); > > Or you could make BudgetChunks a std::list instead, then removing elements > wouldn't require the array to be rewritten (and there seem to be no downsides, > since you never index into it). You're right that this use means we should change the vector to a list. I updated the iteration to a for loop, but continued to use iterators rather than directly accessing the chunks. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:190: budget_map_[origin.spec()].chunks = std::move(chunks); On 2016/08/19 18:48:36, johnme wrote: > You could just write the chunks directly into budget_map_[origin.spec()].chunks > in the first place. Obviously you'd need to cache a reference to the BudgetInfo > rather than doing a string hash lookup every loop iteration, but that's probably > not a bad idea anyway. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:256: const BudgetChunks& chunks = budget_map_[origin.spec()].chunks; On 2016/08/19 18:48:36, johnme wrote: > Nit: store a reference to the BudgetInfo instead. Then you can re-use the > reference for set_last_updated. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:48: using BudgetExpectation = std::list<BudgetStatus>; On 2016/08/19 18:48:36, johnme wrote: > Nit: Can we rename this to BudgetPrediction (in a separate patch if necessary)? > Expectation sounds like it's for testing only. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:73: void AddBudget(const GURL& origin, On 2016/08/19 18:48:36, johnme wrote: > What's the use case for adding non-engagement budget? There are a few of these. One would be a bootstrapping amount of budget for new origins. Another would be refunding budget if a scheduled timer didn't use it. In the short term, it gives a very useful way to test the infrastructure and doesn't add much extra code. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:86: // Spend a particular amount of budget for an origin. The callback specifies On 2016/08/19 18:48:36, johnme wrote: > Nit: s/specifies/takes/ Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:89: // to the database. This should only be called after the budget has been read. On 2016/08/19 18:48:36, johnme wrote: > It seems a slightly awkward constraint that "This should only be called after > the budget has been read". I'm not even sure what that means - do you have to > call GetBudgetDetails and wait for its callback? Could you avoid that using a > queue of pending tasks to perform asynchronously once loaded (in the style of > GCMDelayedTaskController)? Peter and I had a long discussion about how much work we needed to do to make this code safe, and came to the conclusion that as the only consumer was the code in BudgetManager, we could rely on the caller being well behaved, rather than building in a lot of structure to enforce that. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:96: const BudgetDatabase::BudgetExpectation& expectation() { On 2016/08/19 18:48:36, johnme wrote: > Nit: newline before please Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:198: On 2016/08/19 18:48:36, johnme wrote: > Nit: remove newline, since the ASSERTs below are tied to this call to > GetBudgetDetails. Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:201: const auto& expected_value = expectation(); On 2016/08/19 18:48:36, johnme wrote: > Nit: Don't bother caching this reference; the compiler can just inline the > expectation() getter. Or just make expectation_ protected and access it > directly, like success_? Made prediction_ protected, it just makes the code much simpler. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:204: ASSERT_EQ(kDefaultEngagement, expectation().begin()->budget_at); On 2016/08/19 18:48:36, johnme wrote: > s/expectation()/expected_value/ since you've cached a reference to it Directly accesses prediction_ now. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:209: GetBudgetDetails(); On 2016/08/19 18:48:36, johnme wrote: > Add newline before getBudgetDetails() Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:210: On 2016/08/19 18:48:36, johnme wrote: > Ditto remove newline Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:214: ASSERT_DOUBLE_EQ(kDefaultEngagement * 4 / 3, On 2016/08/19 18:48:36, johnme wrote: > Might be nice to re-test the other 2 values too, to make sure they haven't > changed. Ditto above, with testing the 2nd value. To make this easier, consider > making expectation_ be a std::vector (initialised as a copy of the std::list in > GetBudgetDetailsComplete). Good idea on converting the member to be a vector. I've done that and cleaned up the code to use it. I've added some checks on the remaining values in this case, although it ends up duplicating the logic checking that was done in the AddBudget case. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:238: ASSERT_DOUBLE_EQ(kDefaultBudget1 * 3 - 1, expectation().begin()->budget_at); On 2016/08/19 18:48:36, johnme wrote: > Ditto with checking the other values in expectation()? Done. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:249: EXPECT_FALSE(SpendBudget(origin, kDefaultBudget1)); On 2016/08/19 18:48:36, johnme wrote: > After this, check that the budget hasn't changed? Done.
lgtm with nits https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: int elapsedHours = elapsed.InHours(); On 2016/08/22 13:46:26, harkness wrote: > On 2016/08/19 18:48:36, johnme wrote: > > If AddEngagementBudget gets called more than once per hour, the origin will > > never get any budget, because elapsedHours will always be zero (since it is > > rounded down). Use floating point maths for this instead, e.g. |InSecondsF() / > > 3600.0|? > > Actually, I always meant to just return if the time was less than an hour to > prevent proliferation of small chunks, I just didn't add the code. I've added it > now. I see. Even so, `awardRatio = elapsedHours / kBudgetDurationInHours;` still sometimes awards less budget than the site deserves, which seems odd. https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2255813002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:89: // to the database. This should only be called after the budget has been read. On 2016/08/22 13:46:26, harkness wrote: > On 2016/08/19 18:48:36, johnme wrote: > > It seems a slightly awkward constraint that "This should only be called after > > the budget has been read". I'm not even sure what that means - do you have to > > call GetBudgetDetails and wait for its callback? Could you avoid that using a > > queue of pending tasks to perform asynchronously once loaded (in the style of > > GCMDelayedTaskController)? > > Peter and I had a long discussion about how much work we needed to do to make > this code safe, and came to the conclusion that as the only consumer was the > code in BudgetManager, we could rely on the caller being well behaved, rather > than building in a lot of structure to enforce that. Doesn't that just mean you'll have to add equivalent complexity to BudgetManager? If you keep it this way, please make the comments more explicit anyway. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:137: auto& info = budget_map_[origin.spec()]; > Peter has been pushing for everything to be auto, so I'd rather keep it auto for consistency. https://google.github.io/styleguide/cppguide.html#auto says (paraphrasing): Use auto to avoid long/cluttery type names, especially when they involve templates or namespaces or ::iterator, and only for local variables. But continue to use manifest type declarations when it helps readability, for example if the type is not otherwise spelled out nearby. `auto& info` here seems clearly less legible than `BudgetInfo& info` since otherwise you have to lookup the declaration of the map to know what type info is. Ditto with the `const auto& chunk` below. I checked with Peter and he has no strong opinion on these cases. However conversely, we'd both suggest using `auto chunkIter` instead of `BudgetChunks::iterator` below ;-) https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:150: chunkIter++) { Always ++it not it++ with iterators to avoid copying them. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:158: info.chunks.erase(info.chunks.begin(), chunkIter); Now that you're using a std::list, and since you kept iterators rather than range-based for, I'd lean towards erasing as you go rather than than all at the end, since otherwise someone reading the code has to think carefully about the possible final values of chunkIter at the end of the for loop. Then you can also inline the declaration of chunkIter into the for loop (and rename it to `it` or `iter` so it fits on one line). https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:194: BudgetChunks& chunks = info.chunks; Micro-nit: you only use this variable once - inline it? https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:82: prediction_.clear(); You can probably just do prediction_.assign(prediction.begin(), prediction.end())
lgtm % nits https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:91: double sesScore, nit: s/sesScore/score/ https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:97: double awardRatio = 1.0; nit: s/awardRatio/ratio/ https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: int elapsedHours = elapsed.InHours(); nit: s/elapsedHours/elapsed_hours/ It'd be amazing if clang-format could tell you about this. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:82: double sesScore, nit: s/sesScore/score/ https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:183: base::Time expirationTime = nit: s/expirationTime/expiration_time/
https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:91: double sesScore, On 2016/08/22 18:04:27, Peter Beverloo wrote: > nit: s/sesScore/score/ Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:97: double awardRatio = 1.0; On 2016/08/22 18:04:27, Peter Beverloo wrote: > nit: s/awardRatio/ratio/ Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:104: int elapsedHours = elapsed.InHours(); On 2016/08/22 18:04:27, Peter Beverloo wrote: > nit: s/elapsedHours/elapsed_hours/ > > It'd be amazing if clang-format could tell you about this. I'm not sure whether you or I would appreciate that more at this point! https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:137: auto& info = budget_map_[origin.spec()]; On 2016/08/22 14:50:31, johnme wrote: > > Peter has been pushing for everything to be auto, so I'd rather keep it auto > for > consistency. > > https://google.github.io/styleguide/cppguide.html#auto says (paraphrasing): Use > auto to avoid long/cluttery type names, especially when they involve templates > or namespaces or ::iterator, and only for local variables. But continue to use > manifest type declarations when it helps readability, for example if the type is > not otherwise spelled out nearby. > > `auto& info` here seems clearly less legible than `BudgetInfo& info` since > otherwise you have to lookup the declaration of the map to know what type info > is. Ditto with the `const auto& chunk` below. I checked with Peter and he has no > strong opinion on these cases. However conversely, we'd both suggest using `auto > chunkIter` instead of `BudgetChunks::iterator` below ;-) Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:150: chunkIter++) { On 2016/08/22 14:50:31, johnme wrote: > Always ++it not it++ with iterators to avoid copying them. Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:158: info.chunks.erase(info.chunks.begin(), chunkIter); On 2016/08/22 14:50:31, johnme wrote: > Now that you're using a std::list, and since you kept iterators rather than > range-based for, I'd lean towards erasing as you go rather than than all at the > end, since otherwise someone reading the code has to think carefully about the > possible final values of chunkIter at the end of the for loop. Then you can also > inline the declaration of chunkIter into the for loop (and rename it to `it` or > `iter` so it fits on one line). Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:194: BudgetChunks& chunks = info.chunks; On 2016/08/22 14:50:31, johnme wrote: > Micro-nit: you only use this variable once - inline it? Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.h (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.h:82: double sesScore, On 2016/08/22 18:04:27, Peter Beverloo wrote: > nit: s/sesScore/score/ Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:82: prediction_.clear(); On 2016/08/22 14:50:31, johnme wrote: > You can probably just do prediction_.assign(prediction.begin(), > prediction.end()) Good point. Done. https://codereview.chromium.org/2255813002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database_unittest.cc:183: base::Time expirationTime = On 2016/08/22 18:04:27, Peter Beverloo wrote: > nit: s/expirationTime/expiration_time/ Done.
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2255813002/#ps60001 (title: "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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== AddEngagementBudget and SpendBudget added to BudgetDatabase. The BudgetDatabase already had a basic AddBudget method which would add a fixed amount of budget. This CL adds an engagement based budget increment which takes the SES score and adds a percentage of the score to the budget based on when the last award grant was added. For instance, if the budget was last added a day ago, then AddEngagementBudget will add 1/3 of the SES score to the budget. This functionality required adding another parameter to the database, which is the last time the budget was incremented with engagement-based budget. This CL also adds SpendBudget, which will reduce the budget by the amount requested if it is available, and will invoke the callback with false otherwise. BUG=617971 ========== to ========== AddEngagementBudget and SpendBudget added to BudgetDatabase. The BudgetDatabase already had a basic AddBudget method which would add a fixed amount of budget. This CL adds an engagement based budget increment which takes the SES score and adds a percentage of the score to the budget based on when the last award grant was added. For instance, if the budget was last added a day ago, then AddEngagementBudget will add 1/3 of the SES score to the budget. This functionality required adding another parameter to the database, which is the last time the budget was incremented with engagement-based budget. This CL also adds SpendBudget, which will reduce the budget by the amount requested if it is available, and will invoke the callback with false otherwise. BUG=617971 Committed: https://crrev.com/e0e2609b94ad995b18286f1e82cb7cb41c926708 Cr-Commit-Position: refs/heads/master@{#413995} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e0e2609b94ad995b18286f1e82cb7cb41c926708 Cr-Commit-Position: refs/heads/master@{#413995} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
