Fix threading issues in BlacklistManager, using new ChromeThread.

Fix threading issues in BlacklistManager, using new ChromeThread.

TEST=Covered by unit_tests and browser_tests.
BUG=21541
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31386

[jam, 2009-11-05 19:21:44]

http://codereview.chromium.org/361030/diff/1/2
File chrome/browser/privacy_blacklist/blacklist_manager.cc (left):

http://codereview.chromium.org/361030/diff/1/2#oldcode22
Line 22: // a custom Task instead of just NewRunnableMethod.
I think it'll simplify the code to stop using tasks and switch to using
NewRunnableMethod.  wdyt?

http://codereview.chromium.org/361030/diff/1/5
File chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc (right):

http://codereview.chromium.org/361030/diff/1/5#newcode145
Line 145: ChromeThread mock_file_thread(ChromeThread::FILE,
MessageLoop::current());
nit: it's better to put these just once in the BlackListManagerTest base class.

[Paweł Hajdan Jr., 2009-11-06 13:21:40]

On 2009/11/05 19:21:44, John Abd-El-Malek wrote:
> http://codereview.chromium.org/361030/diff/1/2#oldcode22
> Line 22: // a custom Task instead of just NewRunnableMethod.
> I think it'll simplify the code to stop using tasks and switch to using
> NewRunnableMethod.  wdyt?

Done. Indeed, seems better now.

> http://codereview.chromium.org/361030/diff/1/5#newcode145
> Line 145: ChromeThread mock_file_thread(ChromeThread::FILE,
> MessageLoop::current());
> nit: it's better to put these just once in the BlackListManagerTest base
class.

I was thinking about it, but I'm not sure what's the best way to do that. In one
case, I need a file thread which is a real thread (the RealThread test case).
But in all cases "ui" thread is just MessageLoop::current(). Maybe I could move
the "ui" thread to the base class... What do you think?

[jam, 2009-11-06 18:29:01]

On 2009/11/06 13:21:40, Paweł Hajdan Jr. wrote:
> I was thinking about it, but I'm not sure what's the best way to do that. In
one
> case, I need a file thread which is a real thread (the RealThread test case).
> But in all cases "ui" thread is just MessageLoop::current(). Maybe I could
move
> the "ui" thread to the base class... What do you think?

I think you can make all of them have a "real" file thread which is a different
thread.  Since you just end up calling WaitForBlacklistUpdate(); everywhere
things should still work since the task will get posted to a file thread, and
the response will come back to the UI thread.

[Paweł Hajdan Jr., 2009-11-06 20:38:28]

Good idea, done.

[jam, 2009-11-06 22:22:09]

lgtm