Re-work the thread restrictions on the DOM distiller database

Re-work the thread restrictions on the DOM distiller database

In the current implementation, DomDistillerDatabase cannot be
deleted on the main thread, instead the user must call ::Destroy(),
and then let the database delete itself on its task runner. This is
awkward and sort of leaks implementation details out of that class.

Second, currently the parts of DomDistillerDatabase on each thread
are doing some ad hoc method of checking their thread restrictions.
Using ThreadChecker is better, though it loses some flexibility
from checking TaskRunner::RunsTasksOnCurrentThread (maybe there
should be something like a TaskRunnerChecker for that).

Now:
1. no functions are called on DomDistillerDatabase off of the main
thread
2. DomDistillerDatabase::LevelDB enforces that function calls and
destructor all run on the same thread
3. DomDistillerDatabase can be deleted on the main thread

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

[cjhopman, 2013-11-01 21:51:24]

thakis: *

[Nico, 2013-11-03 23:29:52]

Thanks, this looks simpler.

Update the CL description with why you're doing this ("simpler", I guess). CL
descriptions should always answer "why?", not just "what?".

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
File components/dom_distiller/core/dom_distiller_database.cc (right):

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:30:
DomDistillerDatabase::LevelDB::~LevelDB() {}
should the destructor check too? (the class has a scoped_ptr, so it seems to
matter)

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:33:
thread_checker_.CalledOnValidThread();
This does nothing as is. The function just returns a bool. You want to DCHECK()
the result.

(I suppose the function should be WARN_UNUSED_RESULT. Filed
http://crbug.com/314505 for that – maybe you're looking for commons work to
do… ;-) )

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:122: VLOG(1) << "Opening
" << database_dir.value();
Is this useful? In general, stuff shouldn't log stuff unless necessary. Chrome
outputs way too much logspam.

(also for the other vlogs. also for the log(warning))

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:161: InitCallback
callback) {
Does this intentionally not use thread_checker_ ?

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
File components/dom_distiller/core/dom_distiller_database_unittest.cc (right):

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database_unittest.cc:270: new
DomDistillerDatabase::LevelDB());
Can this be stack-allocated?

[cjhopman, 2013-11-04 18:40:59]

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
File components/dom_distiller/core/dom_distiller_database.cc (right):

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:30:
DomDistillerDatabase::LevelDB::~LevelDB() {}
On 2013/11/03 23:29:52, Nico wrote:
> should the destructor check too? (the class has a scoped_ptr, so it seems to
> matter)

Done.

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:33:
thread_checker_.CalledOnValidThread();
On 2013/11/03 23:29:52, Nico wrote:
> This does nothing as is. The function just returns a bool. You want to
DCHECK()
> the result.
> 
> (I suppose the function should be WARN_UNUSED_RESULT. Filed
> http://crbug.com/314505 for that – maybe you're looking for commons work to
> do… ;-) )

Ha, that's amusing. Done.

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:122: VLOG(1) << "Opening
" << database_dir.value();
On 2013/11/03 23:29:52, Nico wrote:
> Is this useful? In general, stuff shouldn't log stuff unless necessary. Chrome
> outputs way too much logspam.
> 
> (also for the other vlogs. also for the log(warning))

I've gone over the LOG/VLOG and removed/quieted them a bit.

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database.cc:161: InitCallback
callback) {
On 2013/11/03 23:29:52, Nico wrote:
> Does this intentionally not use thread_checker_ ?

Hm. I was using thread_checker_ to enforce that users are using this correctly.
Since this isn't part of the public interface, it doesn't need to be checked.
That being said, it looks inconsistent being the only thing that doesn't check
it.

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
File components/dom_distiller/core/dom_distiller_database_unittest.cc (right):

https://codereview.chromium.org/56193004/diff/1/components/dom_distiller/core...
components/dom_distiller/core/dom_distiller_database_unittest.cc:270: new
DomDistillerDatabase::LevelDB());
On 2013/11/03 23:29:52, Nico wrote:
> Can this be stack-allocated?

This isn't even needed :/

[Nico, 2013-11-04 19:31:56]

lgtm!

https://codereview.chromium.org/56193004/diff/190001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_database_unittest.cc (right):

https://codereview.chromium.org/56193004/diff/190001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_database_unittest.cc:273: new
DomDistillerDatabase(db_thread.message_loop_proxy()));
Can this be on the stack too? (In a local block, since you want it to go away
before pumping the runloop)

[cjhopman, 2013-11-05 19:08:29]

https://codereview.chromium.org/56193004/diff/190001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_database_unittest.cc (right):

https://codereview.chromium.org/56193004/diff/190001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_database_unittest.cc:273: new
DomDistillerDatabase(db_thread.message_loop_proxy()));
On 2013/11/04 19:31:56, Nico wrote:
> Can this be on the stack too? (In a local block, since you want it to go away
> before pumping the runloop)

It could be. I feel like that is subtle enough that I would want to add a
comment about why we need the local block, and then at that point it's simpler
to do the more explicit scoped_ptr w/ .reset().

[commit-bot: I haz the power, 2013-11-05 23:16:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/56193004/190001

[commit-bot: I haz the power, 2013-11-06 03:27:50]

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

[commit-bot: I haz the power, 2013-11-06 18:09:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/56193004/190001

[commit-bot: I haz the power, 2013-11-06 21:59:30]

Change committed as 233386