Implement delaying resource requests until privacy blacklists are ready.

Implement delaying resource requests until privacy blacklists are ready.

Associate a BlacklistRequestInfo with each URLRequest started by ResourceDispatcherHost so that in various places we get access to the right BlacklistManager (each Profile has its own), and lazily cache a Blacklist::Match.

BlacklistListener controls delaying requests until the privacy blacklist is ready for the request.

BlacklistInterceptor handles substituting real response with a blocking page or blocking image. I've temporarily removed support for unblocking things. It was too hacky.

This change also removes a large block of blacklist-related code from RDH to more focused classes. Should make it a little more readable.

This should also make BlacklistManagerBrowserTest not flaky.

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

[Paweł Hajdan Jr., 2009-12-17 18:49:50]

Sorry for a big patch for review. I had to rip out many things in order to make
it more modular.

I didn't manage to write a unit test for BlacklistListener yet. It's kind of
covered by the BlacklistManagerBrowserTest. The difficulty here is that there
are several unmockable objects involved. I think I'll write a browser test for
it after all, will be simpler and more readable.

[Paweł Hajdan Jr., 2009-12-18 06:50:05]

ping. I noticed the ui_tests failures, going to fix them. But please review the
code anyway, so I can fix other issues in batch.

[Paweł Hajdan Jr., 2009-12-18 15:45:21]

Patch updated to fix ui_tests failures. Please review.

[Erik does not do reviews, 2009-12-18 23:58:01]

I only looked at browser/privacy_blacklist/*

http://codereview.chromium.org/501082/diff/4001/2007
File chrome/browser/privacy_blacklist/blacklist_interceptor.cc (right):

http://codereview.chromium.org/501082/diff/4001/2007#newcode31
chrome/browser/privacy_blacklist/blacklist_interceptor.cc:31: if
(request_info_.is_main_frame()) {
does this mean that blocked iframes get served as images?

http://codereview.chromium.org/501082/diff/4001/2010
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/4001/2010#newcode21
chrome/browser/privacy_blacklist/blacklist_listener.cc:21:
NotificationType::BLACKLIST_MANAGER_BLACKLIST_READ_FINISHED,
don't you need to also handle the error notifications?  otherwise, the queue
will be stuck forever, right?

http://codereview.chromium.org/501082/diff/4001/2010#newcode33
chrome/browser/privacy_blacklist/blacklist_listener.cc:33: 
remove newline (maybe add one after the next DCHECK)

http://codereview.chromium.org/501082/diff/4001/2010#newcode48
chrome/browser/privacy_blacklist/blacklist_listener.cc:48:
request->GetUserData(&BlacklistRequestInfo::kURLRequestDataKey);
could you add a comment above this test?  I'm not sure exactly what condition
this represents.

http://codereview.chromium.org/501082/diff/4001/2010#newcode51
chrome/browser/privacy_blacklist/blacklist_listener.cc:51: BlacklistRequestInfo*
blacklist_request_info =
add newline above

http://codereview.chromium.org/501082/diff/4001/2010#newcode59
chrome/browser/privacy_blacklist/blacklist_listener.cc:59:
delayed_requests_[blacklist_manager].push_back(request_id);
I must be misreading something here, but it seems like this is saying that all
requests that don't match the blacklist get delayed, even after the blacklist
has finished loading.  Maybe this part (or the whole method) just needs more
explanation about when this method is called, and what the tests are.

http://codereview.chromium.org/501082/diff/4001/2011
File chrome/browser/privacy_blacklist/blacklist_listener.h (right):

http://codereview.chromium.org/501082/diff/4001/2011#newcode50
chrome/browser/privacy_blacklist/blacklist_listener.h:50: typedef std::map<const
BlacklistManager*, RequestList> DelayedRequestMap;
current prevalent style seems to be to put the typedef next to the variable
unless it's needed in a method signature.

http://codereview.chromium.org/501082/diff/4001/2011#newcode52
chrome/browser/privacy_blacklist/blacklist_listener.h:52: void
StartDelayedRequests(const BlacklistManager* blacklist_manager);
use & rather than * for const args.

http://codereview.chromium.org/501082/diff/4001/2011#newcode54
chrome/browser/privacy_blacklist/blacklist_listener.h:54: ResourceQueue*
resource_queue_;
comments for instance variables.

http://codereview.chromium.org/501082/diff/4001/2016
File chrome/browser/privacy_blacklist/blacklist_request_info.cc (right):

http://codereview.chromium.org/501082/diff/4001/2016#newcode34
chrome/browser/privacy_blacklist/blacklist_request_info.cc:34:
blacklist_match_.reset(blacklist->findMatch(url_));
why not do this in the constructor?

http://codereview.chromium.org/501082/diff/4001/2017
File chrome/browser/privacy_blacklist/blacklist_request_info.h (right):

http://codereview.chromium.org/501082/diff/4001/2017#newcode31
chrome/browser/privacy_blacklist/blacklist_request_info.h:31: bool main_frame_;
Add comment explaining.

http://codereview.chromium.org/501082/diff/4001/2017#newcode35
chrome/browser/privacy_blacklist/blacklist_request_info.h:35: mutable
scoped_ptr<Blacklist::Match> blacklist_match_;
use of mutable is discouraged.  If you have a need for it, add a comment to
defend its use.

http://codereview.chromium.org/501082/diff/4001/2018
File chrome/browser/privacy_blacklist/blacklist_test_util.cc (right):

http://codereview.chromium.org/501082/diff/4001/2018#newcode66
chrome/browser/privacy_blacklist/blacklist_test_util.cc:66: Extension
extension(path);
add a comment explaining the empty path is a dummy.  Or maybe create a new
static factory for extension that does this for you?  static Extension*
Extension::CreateTestExtension(const DictionaryValue& manifest);

http://codereview.chromium.org/501082/diff/4001/2019
File chrome/browser/privacy_blacklist/blacklist_test_util.h (right):

http://codereview.chromium.org/501082/diff/4001/2019#newcode16
chrome/browser/privacy_blacklist/blacklist_test_util.h:16: class
BlacklistTestingProfile : public TestingProfile {
all classes need a comment

http://codereview.chromium.org/501082/diff/4001/2019#newcode37
chrome/browser/privacy_blacklist/blacklist_test_util.h:37: void clear();
clear what?  more comments please.

http://codereview.chromium.org/501082/diff/4001/2019#newcode44
chrome/browser/privacy_blacklist/blacklist_test_util.h:44: std::vector<FilePath>
persistent_paths_;
add comment for these variables

http://codereview.chromium.org/501082/diff/4001/2020
File chrome/browser/privacy_blacklist/blacklist_ui.cc (right):

http://codereview.chromium.org/501082/diff/4001/2020#newcode44
chrome/browser/privacy_blacklist/blacklist_ui.cc:44: RenderViewHost* view =
RenderViewHost::FromID(child_id_, route_id_);
can the view change for these ids?  If not, why not look this up in the
constructor and not store the ids?

http://codereview.chromium.org/501082/diff/4001/2020#newcode53
chrome/browser/privacy_blacklist/blacklist_ui.cc:53: string16 reason_;
comments for member variables.

[Paweł Hajdan Jr., 2009-12-29 08:47:10]

I think things are at least a bit better than in the previous version. Thanks
for pointing out some problems, and please take another look.

http://codereview.chromium.org/501082/diff/4001/2007
File chrome/browser/privacy_blacklist/blacklist_interceptor.cc (right):

http://codereview.chromium.org/501082/diff/4001/2007#newcode31
chrome/browser/privacy_blacklist/blacklist_interceptor.cc:31: if
(request_info_.is_main_frame()) {
On 2009/12/18 23:58:01, Erik Kay wrote:
> does this mean that blocked iframes get served as images?

Yes. I think I've improved the situation a little bit, but this will need more
improvements. I plan to do it later, so that this patch doesn't get even larger.

http://codereview.chromium.org/501082/diff/4001/2010
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/4001/2010#newcode21
chrome/browser/privacy_blacklist/blacklist_listener.cc:21:
NotificationType::BLACKLIST_MANAGER_BLACKLIST_READ_FINISHED,
On 2009/12/18 23:58:01, Erik Kay wrote:
> don't you need to also handle the error notifications?  otherwise, the queue
> will be stuck forever, right?

Right. The problem is, that then an error effectively disables all blacklists. I
was thinking about adding a UI for that, like "an error occurred, do you want to
retry or disable the whole thing", or retrying automatically, etc. Obviously
this issue blocks the release of the feature, which for now is behind a
command-line flag.

http://codereview.chromium.org/501082/diff/4001/2010#newcode33
chrome/browser/privacy_blacklist/blacklist_listener.cc:33: 
On 2009/12/18 23:58:01, Erik Kay wrote:
> remove newline (maybe add one after the next DCHECK)

Done.

http://codereview.chromium.org/501082/diff/4001/2010#newcode48
chrome/browser/privacy_blacklist/blacklist_listener.cc:48:
request->GetUserData(&BlacklistRequestInfo::kURLRequestDataKey);
On 2009/12/18 23:58:01, Erik Kay wrote:
> could you add a comment above this test?  I'm not sure exactly what condition
> this represents.

Done.

http://codereview.chromium.org/501082/diff/4001/2010#newcode51
chrome/browser/privacy_blacklist/blacklist_listener.cc:51: BlacklistRequestInfo*
blacklist_request_info =
On 2009/12/18 23:58:01, Erik Kay wrote:
> add newline above

Done.

http://codereview.chromium.org/501082/diff/4001/2010#newcode59
chrome/browser/privacy_blacklist/blacklist_listener.cc:59:
delayed_requests_[blacklist_manager].push_back(request_id);
On 2009/12/18 23:58:01, Erik Kay wrote:
> I must be misreading something here, but it seems like this is saying that all
> requests that don't match the blacklist get delayed, even after the blacklist
> has finished loading.  Maybe this part (or the whole method) just needs more
> explanation about when this method is called, and what the tests are.

Good catch! It was an actual bug in the code. I think I should write a test for
that, because it isn't really covered. I think I will need some more calls just
for test.

http://codereview.chromium.org/501082/diff/4001/2011
File chrome/browser/privacy_blacklist/blacklist_listener.h (right):

http://codereview.chromium.org/501082/diff/4001/2011#newcode50
chrome/browser/privacy_blacklist/blacklist_listener.h:50: typedef std::map<const
BlacklistManager*, RequestList> DelayedRequestMap;
On 2009/12/18 23:58:01, Erik Kay wrote:
> current prevalent style seems to be to put the typedef next to the variable
> unless it's needed in a method signature.

Done.

http://codereview.chromium.org/501082/diff/4001/2011#newcode52
chrome/browser/privacy_blacklist/blacklist_listener.h:52: void
StartDelayedRequests(const BlacklistManager* blacklist_manager);
On 2009/12/18 23:58:01, Erik Kay wrote:
> use & rather than * for const args.

Not so easy. I'm going to call that method with NewRunnableMethod, which with &
would want to copy the BlacklistManager. To avoid confusion, I removed the
const. Or maybe we have a better way to handle such cases?

http://codereview.chromium.org/501082/diff/4001/2011#newcode54
chrome/browser/privacy_blacklist/blacklist_listener.h:54: ResourceQueue*
resource_queue_;
On 2009/12/18 23:58:01, Erik Kay wrote:
> comments for instance variables.

Done.

http://codereview.chromium.org/501082/diff/4001/2016
File chrome/browser/privacy_blacklist/blacklist_request_info.cc (right):

http://codereview.chromium.org/501082/diff/4001/2016#newcode34
chrome/browser/privacy_blacklist/blacklist_request_info.cc:34:
blacklist_match_.reset(blacklist->findMatch(url_));
On 2009/12/18 23:58:01, Erik Kay wrote:
> why not do this in the constructor?

That code was broken. We couldn't distinguish between a situation when we had a
blacklist, but it didn't match an URL, or we didn't have a blacklist ready yet.
I did it in a different way.

http://codereview.chromium.org/501082/diff/4001/2017
File chrome/browser/privacy_blacklist/blacklist_request_info.h (right):

http://codereview.chromium.org/501082/diff/4001/2017#newcode31
chrome/browser/privacy_blacklist/blacklist_request_info.h:31: bool main_frame_;
On 2009/12/18 23:58:01, Erik Kay wrote:
> Add comment explaining.

Done.

http://codereview.chromium.org/501082/diff/4001/2017#newcode35
chrome/browser/privacy_blacklist/blacklist_request_info.h:35: mutable
scoped_ptr<Blacklist::Match> blacklist_match_;
On 2009/12/18 23:58:01, Erik Kay wrote:
> use of mutable is discouraged.  If you have a need for it, add a comment to
> defend its use.

After needed changes here mutable disappeared. :)

http://codereview.chromium.org/501082/diff/4001/2018
File chrome/browser/privacy_blacklist/blacklist_test_util.cc (right):

http://codereview.chromium.org/501082/diff/4001/2018#newcode66
chrome/browser/privacy_blacklist/blacklist_test_util.cc:66: Extension
extension(path);
On 2009/12/18 23:58:01, Erik Kay wrote:
> add a comment explaining the empty path is a dummy.  Or maybe create a new
> static factory for extension that does this for you?  static Extension*
> Extension::CreateTestExtension(const DictionaryValue& manifest);

I'd finally fix this dummy path problem in a separate patch (some extensions
unit tests also do that). Or, if you think that would be better, I can do it for
this patch (but it may accumulate too many different things then).

http://codereview.chromium.org/501082/diff/4001/2019
File chrome/browser/privacy_blacklist/blacklist_test_util.h (right):

http://codereview.chromium.org/501082/diff/4001/2019#newcode16
chrome/browser/privacy_blacklist/blacklist_test_util.h:16: class
BlacklistTestingProfile : public TestingProfile {
On 2009/12/18 23:58:01, Erik Kay wrote:
> all classes need a comment

Done.

http://codereview.chromium.org/501082/diff/4001/2019#newcode37
chrome/browser/privacy_blacklist/blacklist_test_util.h:37: void clear();
On 2009/12/18 23:58:01, Erik Kay wrote:
> clear what?  more comments please.

Done.

http://codereview.chromium.org/501082/diff/4001/2019#newcode44
chrome/browser/privacy_blacklist/blacklist_test_util.h:44: std::vector<FilePath>
persistent_paths_;
On 2009/12/18 23:58:01, Erik Kay wrote:
> add comment for these variables

Done.

http://codereview.chromium.org/501082/diff/4001/2020
File chrome/browser/privacy_blacklist/blacklist_ui.cc (right):

http://codereview.chromium.org/501082/diff/4001/2020#newcode44
chrome/browser/privacy_blacklist/blacklist_ui.cc:44: RenderViewHost* view =
RenderViewHost::FromID(child_id_, route_id_);
On 2009/12/18 23:58:01, Erik Kay wrote:
> can the view change for these ids?  If not, why not look this up in the
> constructor and not store the ids?

I think it may change. There is some time window in which for example a tab may
get closed. But I'm not sure about the exact behavior. I think Darin or John
know for sure.

http://codereview.chromium.org/501082/diff/4001/2020#newcode53
chrome/browser/privacy_blacklist/blacklist_ui.cc:53: string16 reason_;
On 2009/12/18 23:58:01, Erik Kay wrote:
> comments for member variables.

Done. Also changed the variable name for better.

[Paweł Hajdan Jr., 2010-01-04 21:19:20]

I've added a test for BlacklistListener. Please take a look.

I'm sorry that this review became so large. :(

[darin (slow to review), 2010-01-04 23:01:11]

LGTM for everything outside of the privacy_blacklist directory.

http://codereview.chromium.org/501082/diff/9003/11023
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/501082/diff/9003/11023#newcode1102
chrome/browser/renderer_host/resource_dispatcher_host.cc:1102:
request_info->GetBlacklistManager();
nit: it might be nice to create a BlacklistRequestInfo::FromURLRequest() method
that hides this boilerplate stuff.

[Erik does not do reviews, 2010-01-05 00:59:16]

http://codereview.chromium.org/501082/diff/4001/2010
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/4001/2010#newcode21
chrome/browser/privacy_blacklist/blacklist_listener.cc:21:
NotificationType::BLACKLIST_MANAGER_BLACKLIST_READ_FINISHED,
On 2009/12/29 08:47:10, Paweł Hajdan Jr. wrote:
> On 2009/12/18 23:58:01, Erik Kay wrote:
> > don't you need to also handle the error notifications?  otherwise, the queue
> > will be stuck forever, right?
> 
> Right. The problem is, that then an error effectively disables all blacklists.
I
> was thinking about adding a UI for that, like "an error occurred, do you want
to
> retry or disable the whole thing", or retrying automatically, etc. Obviously
> this issue blocks the release of the feature, which for now is behind a
> command-line flag.

So you'd rather have all browsing hang than allow the user to surf without a
blacklist?  Even given that this is an experimental feature, I think it's not
the right way to go.  This seems easy to let the queue flow without adding any
UI for now.

http://codereview.chromium.org/501082/diff/4001/2011
File chrome/browser/privacy_blacklist/blacklist_listener.h (right):

http://codereview.chromium.org/501082/diff/4001/2011#newcode52
chrome/browser/privacy_blacklist/blacklist_listener.h:52: void
StartDelayedRequests(const BlacklistManager* blacklist_manager);
On 2009/12/29 08:47:10, Paweł Hajdan Jr. wrote:
> On 2009/12/18 23:58:01, Erik Kay wrote:
> > use & rather than * for const args.
> 
> Not so easy. I'm going to call that method with NewRunnableMethod, which with
&
> would want to copy the BlacklistManager. To avoid confusion, I removed the
> const. Or maybe we have a better way to handle such cases?

I see.  I guess I'd keep the const and add a short comment explaining.  The
method should have a comment anyway.

http://codereview.chromium.org/501082/diff/4001/2020
File chrome/browser/privacy_blacklist/blacklist_ui.cc (right):

http://codereview.chromium.org/501082/diff/4001/2020#newcode44
chrome/browser/privacy_blacklist/blacklist_ui.cc:44: RenderViewHost* view =
RenderViewHost::FromID(child_id_, route_id_);
On 2009/12/29 08:47:10, Paweł Hajdan Jr. wrote:
> On 2009/12/18 23:58:01, Erik Kay wrote:
> > can the view change for these ids?  If not, why not look this up in the
> > constructor and not store the ids?
> 
> I think it may change. There is some time window in which for example a tab
may
> get closed. But I'm not sure about the exact behavior. I think Darin or John
> know for sure.

What would it mean for it to change?  Could you get a concrete answer here?

http://codereview.chromium.org/501082/diff/9003/11009
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/9003/11009#newcode54
chrome/browser/privacy_blacklist/blacklist_listener.cc:54: const Blacklist*
blacklist = blacklist_manager->GetCompiledBlacklist();
given that this variable is only used in the if test, I'd just drop it and put
the expression directly in the if test.

http://codereview.chromium.org/501082/diff/9003/11011
File chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc (right):

http://codereview.chromium.org/501082/diff/9003/11011#newcode26
chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc:26: class
DummyResourceHandler : public ResourceHandler {
comment each class

http://codereview.chromium.org/501082/diff/9003/11011#newcode160
chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc:160:
ASSERT_FALSE(request->is_pending());
It seems weird to have different before/after tests.  It seems like you should
test data_received before and is_pending after as well.  (this also goes for the
NoDelay version)

[Paweł Hajdan Jr., 2010-01-05 08:27:14]

http://codereview.chromium.org/501082/diff/4001/2010
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/4001/2010#newcode21
chrome/browser/privacy_blacklist/blacklist_listener.cc:21:
NotificationType::BLACKLIST_MANAGER_BLACKLIST_READ_FINISHED,
On 2010/01/05 00:59:16, Erik Kay wrote:
> So you'd rather have all browsing hang than allow the user to surf without a
> blacklist?  Even given that this is an experimental feature, I think it's not
> the right way to go.  This seems easy to let the queue flow without adding any
> UI for now.

Well, the first thing is that an I/O error that'd prevent the blacklist from
loading is going to be rare, so I think we can implement that when it's known
exactly how it should behave (I'm discussing it by e-mail with Nick Baum). If
you prefer I can just add listening for blacklist manager error here with a
TODO. This way surfing will work after such an error.

http://codereview.chromium.org/501082/diff/4001/2011
File chrome/browser/privacy_blacklist/blacklist_listener.h (right):

http://codereview.chromium.org/501082/diff/4001/2011#newcode52
chrome/browser/privacy_blacklist/blacklist_listener.h:52: void
StartDelayedRequests(const BlacklistManager* blacklist_manager);
On 2010/01/05 00:59:16, Erik Kay wrote:
> On 2009/12/29 08:47:10, Paweł Hajdan Jr. wrote:
> > On 2009/12/18 23:58:01, Erik Kay wrote:
> > > use & rather than * for const args.
> > 
> > Not so easy. I'm going to call that method with NewRunnableMethod, which
with
> &
> > would want to copy the BlacklistManager. To avoid confusion, I removed the
> > const. Or maybe we have a better way to handle such cases?
> 
> I see.  I guess I'd keep the const and add a short comment explaining.  The
> method should have a comment anyway.
> 

Done.

http://codereview.chromium.org/501082/diff/4001/2020
File chrome/browser/privacy_blacklist/blacklist_ui.cc (right):

http://codereview.chromium.org/501082/diff/4001/2020#newcode44
chrome/browser/privacy_blacklist/blacklist_ui.cc:44: RenderViewHost* view =
RenderViewHost::FromID(child_id_, route_id_);
On 2010/01/05 00:59:16, Erik Kay wrote:
> What would it mean for it to change?  Could you get a concrete answer here?

I'm going to ask Darin about that.

http://codereview.chromium.org/501082/diff/9003/11009
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/9003/11009#newcode54
chrome/browser/privacy_blacklist/blacklist_listener.cc:54: const Blacklist*
blacklist = blacklist_manager->GetCompiledBlacklist();
On 2010/01/05 00:59:17, Erik Kay wrote:
> given that this variable is only used in the if test, I'd just drop it and put
> the expression directly in the if test.

Done.

http://codereview.chromium.org/501082/diff/9003/11011
File chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc (right):

http://codereview.chromium.org/501082/diff/9003/11011#newcode26
chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc:26: class
DummyResourceHandler : public ResourceHandler {
On 2010/01/05 00:59:17, Erik Kay wrote:
> comment each class

Done.

http://codereview.chromium.org/501082/diff/9003/11011#newcode160
chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc:160:
ASSERT_FALSE(request->is_pending());
On 2010/01/05 00:59:17, Erik Kay wrote:
> It seems weird to have different before/after tests.  It seems like you should
> test data_received before and is_pending after as well.  (this also goes for
the
> NoDelay version)

Hmm... doing that would seem weird to me. That's because we're not going to
receive any data before the request is started and the message loop was run (so
I see no point in testing for that), and the request is not going to be pending
when it has finished...

By the way, this test follows a general pattern from
net/url_request/url_request_unittest.cc (checking for is_pending before running
the loop, and testing the results after that).

On the other hand, I wouldn't want this test to be non-obvious or odd-looking.
Could you explain more this comment?

http://codereview.chromium.org/501082/diff/9003/11023
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/501082/diff/9003/11023#newcode1102
chrome/browser/renderer_host/resource_dispatcher_host.cc:1102:
request_info->GetBlacklistManager();
On 2010/01/04 23:01:11, darin wrote:
> nit: it might be nice to create a BlacklistRequestInfo::FromURLRequest()
method
> that hides this boilerplate stuff.

Done.

[Erik does not do reviews, 2010-01-05 15:29:49]

LGTM, assuming no other significant changes from Darin's response to the other
question

http://codereview.chromium.org/501082/diff/4001/2010
File chrome/browser/privacy_blacklist/blacklist_listener.cc (right):

http://codereview.chromium.org/501082/diff/4001/2010#newcode21
chrome/browser/privacy_blacklist/blacklist_listener.cc:21:
NotificationType::BLACKLIST_MANAGER_BLACKLIST_READ_FINISHED,
On 2010/01/05 08:27:15, Paweł Hajdan Jr. wrote:
> On 2010/01/05 00:59:16, Erik Kay wrote:
> > So you'd rather have all browsing hang than allow the user to surf without a
> > blacklist?  Even given that this is an experimental feature, I think it's
not
> > the right way to go.  This seems easy to let the queue flow without adding
any
> > UI for now.
> 
> Well, the first thing is that an I/O error that'd prevent the blacklist from
> loading is going to be rare, so I think we can implement that when it's known
> exactly how it should behave (I'm discussing it by e-mail with Nick Baum). If
> you prefer I can just add listening for blacklist manager error here with a
> TODO. This way surfing will work after such an error.

that would be great

http://codereview.chromium.org/501082/diff/9003/11011
File chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc (right):

http://codereview.chromium.org/501082/diff/9003/11011#newcode160
chrome/browser/privacy_blacklist/blacklist_listener_unittest.cc:160:
ASSERT_FALSE(request->is_pending());
On 2010/01/05 08:27:15, Paweł Hajdan Jr. wrote:
> On 2010/01/05 00:59:17, Erik Kay wrote:
> > It seems weird to have different before/after tests.  It seems like you
should
> > test data_received before and is_pending after as well.  (this also goes for
> the
> > NoDelay version)
> 
> Hmm... doing that would seem weird to me. That's because we're not going to
> receive any data before the request is started and the message loop was run
(so
> I see no point in testing for that), and the request is not going to be
pending
> when it has finished...
> 
> By the way, this test follows a general pattern from
> net/url_request/url_request_unittest.cc (checking for is_pending before
running
> the loop, and testing the results after that).
> 
> On the other hand, I wouldn't want this test to be non-obvious or odd-looking.
> Could you explain more this comment?

If this follows an established pattern, I'm OK with it.

[darin (slow to review), 2010-01-05 17:20:52]

http://codereview.chromium.org/501082/diff/4001/2020
File chrome/browser/privacy_blacklist/blacklist_ui.cc (right):

http://codereview.chromium.org/501082/diff/4001/2020#newcode44
chrome/browser/privacy_blacklist/blacklist_ui.cc:44: RenderViewHost* view =
RenderViewHost::FromID(child_id_, route_id_);
Storing a raw pointer to RenderViewHost seems risky because we then have to
ensure that it remains valid, which could be a problem when the tab is closing. 
The current solution seems good to me.

Also note that it is not possible to lookup the RenderViewHost from the IO
thread, and the constructor of this task runs on the IO thread.

The current solution matches what we do elsewhere when sending notifications
from the IO thread to a particular RVH on the UI thread.