Enable database logging on history thread.

Enable database logging on history thread.

BUG=16591
TEST=History UI test

[darin (slow to review), 2009-09-04 19:32:43]

http://codereview.chromium.org/198011/diff/1003/1005
File base/thread.h (right):

http://codereview.chromium.org/198011/diff/1003/1005#newcode128
Line 128: virtual void Run(MessageLoop& MessageLoop);
google style says that reference parameters must only be const.  for this, i
think you want to use a pointer.  also, please put this above between Init and
CleanUp in the protected section to indicate that it is intended to be
overriden.

http://codereview.chromium.org/198011/diff/1003/1007
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/198011/diff/1003/1007#newcode101
Line 101: void RunHistoryThread(MessageLoop& MessageLoop);
nit: "MessageLoop* message_loop" for both of these

I think you should instead create a new class that extends from ChromeThread for
use by the history system.  This way I don't think you should have any changes
to ChromeThread.

http://codereview.chromium.org/198011/diff/1003/1008
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/198011/diff/1003/1008#newcode101
Line 101: : thread_(new ChromeThread(ChromeThread::HISTORY)),
it is strange that we were not using ChromeThread before.  was
ChromeThread::HISTORY just not hooked up to anything before?

[huanr, 2009-09-05 00:07:03]

http://codereview.chromium.org/198011/diff/1003/1005
File base/thread.h (right):

http://codereview.chromium.org/198011/diff/1003/1005#newcode128
Line 128: virtual void Run(MessageLoop& MessageLoop);
On 2009/09/04 19:32:43, darin wrote:
> google style says that reference parameters must only be const.  for this, i
> think you want to use a pointer.  also, please put this above between Init and
> CleanUp in the protected section to indicate that it is intended to be
> overriden.

Done.

http://codereview.chromium.org/198011/diff/1003/1007
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/198011/diff/1003/1007#newcode101
Line 101: void RunHistoryThread(MessageLoop& MessageLoop);
On 2009/09/04 19:32:43, darin wrote:
> nit: "MessageLoop* message_loop" for both of these
> 
> I think you should instead create a new class that extends from ChromeThread
for
> use by the history system.  This way I don't think you should have any changes
> to ChromeThread.

Done.

http://codereview.chromium.org/198011/diff/1003/1008
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/198011/diff/1003/1008#newcode101
Line 101: : thread_(new ChromeThread(ChromeThread::HISTORY)),
Right. ChromeThread::HISTORY is only used here.
 On 2009/09/04 19:32:43, darin wrote:
> it is strange that we were not using ChromeThread before.  was
> ChromeThread::HISTORY just not hooked up to anything before?

[darin (slow to review), 2009-09-08 16:48:54]

http://codereview.chromium.org/198011/diff/6001/7003
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/198011/diff/6001/7003#newcode101
Line 101: namespace {
nit: move this anonymous namespace section above the HistoryService:: stuff. 
That way the HistoryService implementation remains a single group.

http://codereview.chromium.org/198011/diff/6001/7003#newcode106
Line 106: virtual ~ChromeHistoryThread() {}
whenever you subclass Thread, you unfortunately have to remember to call Stop()
from your destructor.  otherwise, there can be problems.  for example, your
CleanUp method may not run.  this doesn't matter to you, but it could matter if
someone later on added a CleanUp method.

(that you have to do this means that Thread is poorly designed--blame me!)

[huanr, 2009-09-08 17:35:43]

http://codereview.chromium.org/198011/diff/6001/7003
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/198011/diff/6001/7003#newcode101
Line 101: namespace {
On 2009/09/08 16:48:54, darin wrote:
> nit: move this anonymous namespace section above the HistoryService:: stuff. 
> That way the HistoryService implementation remains a single group.

Done.

http://codereview.chromium.org/198011/diff/6001/7003#newcode106
Line 106: virtual ~ChromeHistoryThread() {}
On 2009/09/08 16:48:54, darin wrote:
> whenever you subclass Thread, you unfortunately have to remember to call
Stop()
> from your destructor.  otherwise, there can be problems.  for example, your
> CleanUp method may not run.  this doesn't matter to you, but it could matter
if
> someone later on added a CleanUp method.
> 
> (that you have to do this means that Thread is poorly designed--blame me!)

Done.

[darin (slow to review), 2009-09-09 17:56:30]

LGTM

[tim (not reviewing), 2009-09-17 22:05:36]

This cl broke the sync integration tests :(  A long time ago I landed
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/histor...,
which removed the notion of a well known history thread so that we can run with
multiple profiles at once, and created a ProfileManager test for this, but it
has been disabled due to memory leaks caused by (we think) the MessageLoop not
destroying tasks properly at shutdown.  It turns out my change was quickly
undone back then (a patch immediately following mine re-added HISTORY to
ChromeThread), and now this one seems to reinstate it to it's former self, so
when we try and run the integration tests with multiple profiles, we hit a
DCHECK because ChromeThread expects only one to ever exist.

[huanr, 2009-09-18 00:10:11]

This change is to log history activities. It will be in dev channel for some
time, probably two weeks, and get reverted. I will think of some way to fix it
if this timeline does not work for sync tests.

Regarding the change, Darin was asking about why ChromeThread is not used for
history during the code review, and Brett was surprised about it too. So you may
want to have good comment somewhere to prevent ChromeThread::HISTORY from being
added back.

[tim (not reviewing), 2009-09-18 14:07:29]

On Thu, Sep 17, 2009 at 5:10 PM, <huanr@chromium.org> wrote:

> This change is to log history activities. It will be in dev channel for
> some
> time, probably two weeks, and get reverted. I will think of some way to fix
> it
> if this timeline does not work for sync tests.
>
> Regarding the change, Darin was asking about why ChromeThread is not used
> for
> history during the code review, and Brett was surprised about it too. So
> you may
> want to have good comment somewhere to prevent ChromeThread::HISTORY from
> being
> added back.


Yeah, that and/or somehow re-enable the ProfileManager test.  We
run our sync tests regularly on a buildbot, but we had a bit of a gap
recently due to all our code shuffling around and changing
repositories.     We have a pretty substantial update coming for next
week's dev channel release so it would be nice to have our tests
running
:\  At the very least it seems like if I just inherit from Thread vs
ChromeThread for ChromeHistoryThread I could squeak by and run the tests
locally.  Not sure how to verify that the logging still works.  There's a
thread on chromium-dev discussing the change I made, if you're interested
search for 'question on profiles & ChromeThread'.


>
> http://codereview.chromium.org/198011
>