Make SQLitePersistentCookieStore run on injected SequencedTaskRunners vs named BrowserThreads.

Make SQLitePersistentCookieStore run on injected SequencedTaskRunners vs named BrowserThreads.

In a follow-up, I will make the production code run on the worker pool, which will hopefully speed up browser startup by eliminating delays due to other long-running DB thread tasks.

R=rdsmith@chromium.org
TBR=jar@chromium.org,mmenke@chromium.org
BUG=175415
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184868

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184879

[mmenke, 2013-02-20 20:58:30]

I suggest you have someone who knows this code better than I do take a look at
this (rdsmith?), but this looks good to me - just nits.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:25: #include
"base/threading/thread.h"
Don't think this is needed.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:45: // task to the
background thread.  This task calls Backend::ChainLoadCookies(),
SequencedTaskRunners aren't guaranteed to be threads, should we change
terminology here?

Could just replace thread with "runner", and it wouldn't sound too bad.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:172: const base::Time&
posted_at);
These should be renamed to *OnBGThread.  Actually, SequencedTaskRunners aren't
even guaranteed to be threads.  Maybe InBackground?  OnBGRunner?

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:188: bool load_success);
These should also be renamed to *OnClientThread.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:462: bool load_success) {
Know it wasn't here before, but maybe a
"DCHECK(background_task_runner_->RunsTasksOnCurrentThread());"

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc (right):

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc:53: virtual void
SetUp() {
While you're here, this should be OVERRIDE

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc:93: virtual void
TearDown() {
OVERRIDE

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:127:
pool_owner_->pool()->Shutdown();
Is this needed?  DestroyStore() already calls shutdown.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:135:
base::WaitableEvent db_thread_event_;
There's no longer a db_thread.

[erikwright (departed), 2013-02-21 20:11:43]

rdsmith: PTAL.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:25: #include
"base/threading/thread.h"
On 2013/02/20 20:58:30, mmenke wrote:
> Don't think this is needed.

Done.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:45: // task to the
background thread.  This task calls Backend::ChainLoadCookies(),
On 2013/02/20 20:58:30, mmenke wrote:
> SequencedTaskRunners aren't guaranteed to be threads, should we change
> terminology here?
> 
> Could just replace thread with "runner", and it wouldn't sound too bad.

Done.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:172: const base::Time&
posted_at);
On 2013/02/20 20:58:30, mmenke wrote:
> These should be renamed to *OnBGThread.  Actually, SequencedTaskRunners aren't
> even guaranteed to be threads.  Maybe InBackground?  OnBGRunner?

Done.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:188: bool load_success);
On 2013/02/20 20:58:30, mmenke wrote:
> These should also be renamed to *OnClientThread.

Done.

https://codereview.chromium.org/12314015/diff/10/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:462: bool load_success) {
On 2013/02/20 20:58:30, mmenke wrote:
> Know it wasn't here before, but maybe a
> "DCHECK(background_task_runner_->RunsTasksOnCurrentThread());"

Done.

[Randy Smith (Not in Mondays), 2013-02-22 18:20:16]

As noted below, I think the specification of the relationship between client
thread and class could be a bit crisper, but beyond that, LGTM.  (Note that I
just skimmed the tests to confirm you were doing the obvious search-and-replace,
and figured if there were problems with that, the tests would have picked them
up.  If you want a closer review than that, let me know.)

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:39: // This class is
designed to be shared between any client thread and the
Does the client thread need to be the same as the client runner?  That would
seem to contradict the "any".

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:100: EXPECT_EQ(0U,
cookies.size());
I know this is probably my cluelessness, but why the change?

[erikwright (departed), 2013-02-25 16:25:47]

I'll add some additional comments about the purposes of the two runners.

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:39: // This class is
designed to be shared between any client thread and the
On 2013/02/22 18:20:16, rdsmith wrote:
> Does the client thread need to be the same as the client runner?  That would
> seem to contradict the "any".

No. It can be any, AFAICT.

Insert here, by reference, all of our previous discussions about the threading
model of the cookie store being strange.

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/12314015/diff/3008/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:100: EXPECT_EQ(0U,
cookies.size());
On 2013/02/22 18:20:16, rdsmith wrote:
> I know this is probably my cluelessness, but why the change?

ASSERT is designed to terminate a test. It does so by returning from the test
body (which is defined as void). Thus you cannot use ASSERT in helper methods
with return types.

Most developers (this is from direct observation) assume that ASSERT is
terminating the test, without understanding why. So using it in a void helper
method is misleading. It is unexpected (or, at best, an intended side-effect
that requires detailed knowledge of the implementation of GTest) that it causes
the helper to immediately terminate while allowing the test to continue.

It is more correct to use EXPECT and ADD_FAILURE (vs FAIL) anywhere outside main
test bodies and SetUp/TearDown.

[erikwright (departed), 2013-02-26 19:58:45]

TBR mmenke for browser/{profiles,safe_browsing}
TBR jar for base/base.gyp

[mmenke, 2013-02-26 20:00:18]

On 2013/02/26 19:58:45, erikwright wrote:
> TBR mmenke for browser/{profiles,safe_browsing}
> TBR jar for base/base.gyp

profiles LGTM.

[commit-bot: I haz the power, 2013-02-26 23:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/12314015/1017

[commit-bot: I haz the power, 2013-02-27 01:02:19]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-02-27 02:07:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/12314015/14003

[commit-bot: I haz the power, 2013-02-27 04:15:03]

Change committed as 184868

[commit-bot: I haz the power, 2013-02-27 04:57:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/12314015/14003

[commit-bot: I haz the power, 2013-02-27 05:17:34]

Change committed as 184879