Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2058523003:
Make BackgroundBudgetService calls asynchronous. (Closed)
|
Created:
4 years, 6 months ago by harkness Modified:
4 years, 6 months ago CC:
chromium-reviews, Peter Beverloo, mvanouwerkerk+watch_chromium.org, johnme+watch_chromium.org, harkness+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL converts the GetBudget and StoreBudget calls on the
BackgroundBudgetService to be asynchonous. This is a step on the path to
converting BackgroundBudgetService budget storage to use a LevelDB storage
instead of prefs storage, which will require some of the budget service's
methods to run on a thread other than the UI thread.
BUG=617971
Committed: https://crrev.com/8e81c750cf56b698d4a1a04bfc2a2ed425d01d41
Cr-Commit-Position: refs/heads/master@{#400707}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Code review comments integrated. #
Total comments: 2
Patch Set 3 : Explicitly use base::RunLoop everywhere. #
Total comments: 10
Patch Set 4 : Simplified test cases, updated nits #
Total comments: 12
Patch Set 5 : More test code cleanup #
Messages
Total messages: 37 (15 generated)
harkness@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
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/patch-status/2058523003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/background_budget_service.cc (right): https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.cc:128: // If there is no stored data or the data can't be parsed, just return the So this currently does not return, it just keeps running the rest of the method. You might be able to update your test to detect this kind of bug, where the callback would run more than once. Though... not sure how to wait for such a condition that should not happen. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.cc:143: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, nit: use curly braces for this if statement as this now wraps to multiple lines https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/background_budget_service.h (right): https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.h:13: #include "base/memory/ref_counted.h" Why is this include needed in the header? https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.h:15: #include "content/public/browser/browser_thread.h" Why is this include needed in the header? https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.h:58: const base::Closure& closure); I expect in future this will become a callback that is given a status enum for success / failure? For now, this is fine. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service_unittest.cc:62: void StoredBudget(int id) { nit: add a blank line above this one https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service_unittest.cc:94: WaitForCallback(1); I think we should be able to do this without passing around callback ids, and storing last_callback_ and run_loop_: base::RunLoop run_loop; GetService()->GetBudget(origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget, base::Unretained(this), run_loop.QuitClosure())); run_loop.Run(); ...and run the quit closure in GotBudget.
PTAL https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/background_budget_service.cc (right): https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.cc:128: // If there is no stored data or the data can't be parsed, just return the On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > So this currently does not return, it just keeps running the rest of the method. > You might be able to update your test to detect this kind of bug, where the > callback would run more than once. Though... not sure how to wait for such a > condition that should not happen. Ack. The good news is that it did get caught by the browser test. I missed this when I was pulling out the extra async layer I had added that I decided to defer for the level DB patch. I've updated it now. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.cc:143: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > nit: use curly braces for this if statement as this now wraps to multiple lines Done. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/background_budget_service.h (right): https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.h:13: #include "base/memory/ref_counted.h" On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > Why is this include needed in the header? Removed. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.h:15: #include "content/public/browser/browser_thread.h" On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > Why is this include needed in the header? This was a bigger review, including some of the internal async calls to put the GetBudget call explicitly on the IO thread, and then I needed this, but I removed it now. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service.h:58: const base::Closure& closure); On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > I expect in future this will become a callback that is given a status enum for > success / failure? For now, this is fine. Correct. That will happen when I add in the level DB calls, but there wasn't a success/failure right now. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service_unittest.cc:62: void StoredBudget(int id) { On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > nit: add a blank line above this one Done. https://codereview.chromium.org/2058523003/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/background_budget_service_unittest.cc:94: WaitForCallback(1); On 2016/06/10 12:10:19, Michael van Ouwerkerk wrote: > I think we should be able to do this without passing around callback ids, and > storing last_callback_ and run_loop_: > > base::RunLoop run_loop; > GetService()->GetBudget(origin, > base::Bind(&BackgroundBudgetServiceTest::GotBudget, base::Unretained(this), > run_loop.QuitClosure())); > run_loop.Run(); > > ...and run the quit closure in GotBudget. Updated the code as discussed in person.
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/patch-status/2058523003/20001
lgtm with nit https://codereview.chromium.org/2058523003/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:31: using base::RunLoop; Peter would probably ask you not to do this with things in the base namespace. Personally, I'd just note that there's lots of using statements for things in base, but not for RunLoop, which has a short name, and I don't think it would cause any wrapping in this file.
Oh and please fix the CL title.
Description was changed from ========== This CL converts the GetBudget and StoreBudget calls on the BackgroundBudgetService to be asynchonous. This is a step on the path to converting BackgroundBudgetService budget storage to use a LevelDB storage instead of prefs storage, which will require some of the budget service's methods to run on a thread other than the UI thread. BUG=617971 ========== to ========== This CL converts the GetBudget and StoreBudget calls on the BackgroundBudgetService to be asynchonous. This is a step on the path to converting BackgroundBudgetService budget storage to use a LevelDB storage instead of prefs storage, which will require some of the budget service's methods to run on a thread other than the UI thread. BUG=617971 ==========
https://codereview.chromium.org/2058523003/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:31: using base::RunLoop; On 2016/06/10 15:04:05, Michael van Ouwerkerk wrote: > Peter would probably ask you not to do this with things in the base namespace. > Personally, I'd just note that there's lots of using statements for things in > base, but not for RunLoop, which has a short name, and I don't think it would > cause any wrapping in this file. Done.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2058523003/#ps40001 (title: "Explicitly use base::RunLoop everywhere.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058523003/40001
harkness@chromium.org changed reviewers: + johnme@chromium.org
johnme@chromium.org: Please review changes in chrome/browser/budget_service I'd forgotten that I only added John and Peter as OWNERS in budget_service. John, can you take a look please?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm with comments and nits https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service.h (right): https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service.h:45: using GetBudgetCallback = base::Callback<void(double /* budget */)>; Nit: you don't need to comment out budget. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service.h:56: const base::Closure& closure); Nit: please rename this to done_callback and/or comment that it will be run once the budget has finished being stored. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:61: void StoredBudget(base::Closure run_loop_closure) { run_loop_closure.Run(); } This method is unnecessary. Just pass the QuitClosure directly, rather than wrapping it with this. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:74: std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop()); No need to use unique_ptr, just store it on the stack: base::RunLoop run_loop; Foo(run_loop.QuitClosure()); run_loop.Run(); https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:89: std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop()); It would be simpler to have a helper method on BackgroundBudgetServiceTest that does all the RunLoop faff and synchronously returns a budget. That way the only diff you should need in the tests is replacing `GetService()->GetBudget` with `GetBudget` and `GetService()->StoreBudget` with `StoreBudget`.
https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service.h (right): https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service.h:45: using GetBudgetCallback = base::Callback<void(double /* budget */)>; On 2016/06/15 09:59:06, johnme wrote: > Nit: you don't need to comment out budget. Done. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service.h:56: const base::Closure& closure); On 2016/06/15 09:59:06, johnme wrote: > Nit: please rename this to done_callback and/or comment that it will be run once > the budget has finished being stored. Done. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:61: void StoredBudget(base::Closure run_loop_closure) { run_loop_closure.Run(); } On 2016/06/15 09:59:07, johnme wrote: > This method is unnecessary. Just pass the QuitClosure directly, rather than > wrapping it with this. Done. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:74: std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop()); On 2016/06/15 09:59:07, johnme wrote: > No need to use unique_ptr, just store it on the stack: > > base::RunLoop run_loop; > Foo(run_loop.QuitClosure()); > run_loop.Run(); I ended up hiding this in the helper method. https://codereview.chromium.org/2058523003/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:89: std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop()); On 2016/06/15 09:59:07, johnme wrote: > It would be simpler to have a helper method on BackgroundBudgetServiceTest that > does all the RunLoop faff and synchronously returns a budget. > > That way the only diff you should need in the tests is replacing > `GetService()->GetBudget` with `GetBudget` and `GetService()->StoreBudget` with > `StoreBudget`. I added a helper method for the simple test cases, but any time I needed to modify time, I left the more complicated processing.
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/patch-status/2058523003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:72: const GURL origin(kTestOrigin); Nit: perhaps simpler to just pass GURL(kTestOrigin) as a parameter to StoreBudget. Ditto elsewhere. Up to you. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:74: new BackgroundBudgetService(profile())); 1. No need for unique_ptr here (`BackgroundBudgetService service(profile_);` should be equivalent). 2. Why do you create a new instance of the service here, rather than using GetService to fetch the singleton (per profile anyway) instance from the factory? https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:89: double budget = GetBudget(); Nit: I guess you can just inline this into the EXPECT_. Ditto with other calls to getBudget (you never use the return value more than once). https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:105: EXPECT_NEAR(budget, kTestBudget, kTestBudget); The 3rd parameter to EXPECT_NEAR is the allowable absolute error. `kTestBudget` doesn't seem like a reasonable value for this? Why not just use EXPECT_DOUBLE_EQ here (and many of the other places you use EXPECT_NEAR) for consistency? https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:123: GetService()->GetBudget( It seems like you can use the helper methods here as well. Ditto in GetBudgetConsumedOverTime and GetBudgetNegativeTime. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:146: EXPECT_NEAR(budget_, kTestBudget, kTestSES * 3600.0 / kSecondsToAccumulate); This is quite an elaborate calculation for the allowable absolute error - could you add an explanatory comment?
https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... File chrome/browser/budget_service/background_budget_service_unittest.cc (right): https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:72: const GURL origin(kTestOrigin); On 2016/06/20 13:35:22, johnme wrote: > Nit: perhaps simpler to just pass GURL(kTestOrigin) as a parameter to > StoreBudget. Ditto elsewhere. Up to you. Putting it here allowed me to remove it from a bunch of the tests, so I'm going to leave it here. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:74: new BackgroundBudgetService(profile())); On 2016/06/20 13:35:22, johnme wrote: > 1. No need for unique_ptr here (`BackgroundBudgetService service(profile_);` > should be equivalent). > > 2. Why do you create a new instance of the service here, rather than using > GetService to fetch the singleton (per profile anyway) instance from the > factory? Yeah, I had that in one of my test cases, probably left over from pre-KeyedServiceFactory days, and that happened to be the one I copy/pasted. oops. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:89: double budget = GetBudget(); On 2016/06/20 13:35:22, johnme wrote: > Nit: I guess you can just inline this into the EXPECT_. Ditto with other calls > to getBudget (you never use the return value more than once). I swapped them in calls where it was only used once, but left the assignment in the methods where I did test it multiple times. In those tests, I left all checks to be on budget for consistency. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:105: EXPECT_NEAR(budget, kTestBudget, kTestBudget); On 2016/06/20 13:35:22, johnme wrote: > The 3rd parameter to EXPECT_NEAR is the allowable absolute error. `kTestBudget` > doesn't seem like a reasonable value for this? Why not just use EXPECT_DOUBLE_EQ > here (and many of the other places you use EXPECT_NEAR) for consistency? You're right, this one had a bad bound on it. The other ones actually have reasonable bounds, so I converted this one, but left the others. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:123: GetService()->GetBudget( On 2016/06/20 13:35:22, johnme wrote: > It seems like you can use the helper methods here as well. Ditto in > GetBudgetConsumedOverTime and GetBudgetNegativeTime. Done. https://codereview.chromium.org/2058523003/diff/60001/chrome/browser/budget_s... chrome/browser/budget_service/background_budget_service_unittest.cc:146: EXPECT_NEAR(budget_, kTestBudget, kTestSES * 3600.0 / kSecondsToAccumulate); On 2016/06/20 13:35:22, johnme wrote: > This is quite an elaborate calculation for the allowable absolute error - could > you add an explanatory comment? Updated it to be a more bounds-based calculation.
lgtm - test looks much better thanks!
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2058523003/#ps80001 (title: "More test code cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058523003/80001
Message was sent while issue was closed.
Description was changed from ========== This CL converts the GetBudget and StoreBudget calls on the BackgroundBudgetService to be asynchonous. This is a step on the path to converting BackgroundBudgetService budget storage to use a LevelDB storage instead of prefs storage, which will require some of the budget service's methods to run on a thread other than the UI thread. BUG=617971 ========== to ========== This CL converts the GetBudget and StoreBudget calls on the BackgroundBudgetService to be asynchonous. This is a step on the path to converting BackgroundBudgetService budget storage to use a LevelDB storage instead of prefs storage, which will require some of the budget service's methods to run on a thread other than the UI thread. BUG=617971 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== This CL converts the GetBudget and StoreBudget calls on the BackgroundBudgetService to be asynchonous. This is a step on the path to converting BackgroundBudgetService budget storage to use a LevelDB storage instead of prefs storage, which will require some of the budget service's methods to run on a thread other than the UI thread. BUG=617971 ========== to ========== This CL converts the GetBudget and StoreBudget calls on the BackgroundBudgetService to be asynchonous. This is a step on the path to converting BackgroundBudgetService budget storage to use a LevelDB storage instead of prefs storage, which will require some of the budget service's methods to run on a thread other than the UI thread. BUG=617971 Committed: https://crrev.com/8e81c750cf56b698d4a1a04bfc2a2ed425d01d41 Cr-Commit-Position: refs/heads/master@{#400707} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8e81c750cf56b698d4a1a04bfc2a2ed425d01d41 Cr-Commit-Position: refs/heads/master@{#400707} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
