Introduced the URLBlacklistManager, and wired it to various places.

Introduced the URLBlacklistManager, and wired it to various places.

This is part of a work in progress (http://codereview.chromium.org/7716003).

This patch is meant to introduce the blacklist manager, which listens to prefs
changes on the UI thread (later to be managed by policies) and filters
URLRequests on the IO thread.

Subsequent patches will build the blacklist based on the preferences, and
block URLRequests that match the list. The policy will be introduced once
everything else is ready.

BUG=49612
TEST=URLBlacklistManagerTest; everything still works as before


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99608

[Joao da Silva, 2011-08-25 14:50:36]

Please review, thanks!

@mnissler: main review

@willchan: owner review for browser/net/, browser/profiles/

@phajdan.jr: owner review for test/base/

[Paweł Hajdan Jr., 2011-08-25 17:19:51]

chrome/test LGTM

I also took a look at other test files, below are my comments.

http://codereview.chromium.org/7747018/diff/1/chrome/browser/policy/host_blac...
File chrome/browser/policy/host_blacklist_manager_unittest.cc (right):

http://codereview.chromium.org/7747018/diff/1/chrome/browser/policy/host_blac...
chrome/browser/policy/host_blacklist_manager_unittest.cc:79:
scoped_ptr<NotificationService> notification_service_;
nit: Why scoped_ptr?

http://codereview.chromium.org/7747018/diff/1/chrome/browser/policy/host_blac...
chrome/browser/policy/host_blacklist_manager_unittest.cc:80:
scoped_ptr<TestingProfile> profile_;
nit: Why scoped_ptr?

[Joao da Silva, 2011-08-26 09:28:30]

@phajdan.jr: removed the unnecessary scoped_ptrs. Thanks for reviewing!

http://codereview.chromium.org/7747018/diff/1/chrome/browser/policy/host_blac...
File chrome/browser/policy/host_blacklist_manager_unittest.cc (right):

http://codereview.chromium.org/7747018/diff/1/chrome/browser/policy/host_blac...
chrome/browser/policy/host_blacklist_manager_unittest.cc:79:
scoped_ptr<NotificationService> notification_service_;
On 2011/08/25 17:19:52, Paweł Hajdan Jr. wrote:
> nit: Why scoped_ptr?

No reason for it, removed.

http://codereview.chromium.org/7747018/diff/1/chrome/browser/policy/host_blac...
chrome/browser/policy/host_blacklist_manager_unittest.cc:80:
scoped_ptr<TestingProfile> profile_;
On 2011/08/25 17:19:52, Paweł Hajdan Jr. wrote:
> nit: Why scoped_ptr?

Same.

[willchan no longer on Chromium, 2011-08-26 11:40:20]

The overall structure is ok but the lifetime management needs some work. I
haven't read through in detail, but the ownership seems like it should be held
by ProfileIOData or ProfileImplIOData. And maybe ProfileImpl/OffTheRecordProfile
should hold raw pointers which are guaranteed to be safe since
ProfileImpl/OffTheRecordProfile are destroyed on the UI thread before
ProfileIOData gets destroyed on the IO thread, since
ProfileImpl/OffTheRecordProfile destruction initiates the ProfileIOData
destruction.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:77: // TODO(joaodasilva): this
error code isn't quite correct
This error code is wrong. Maybe ERR_ACCESS_DENIED would be better. Also, does
this break the WebRequest API? What's the right way to let the WebRequest API
know that it's getting denied? I suggest you talk to mpcomplete/battre.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
File chrome/browser/policy/host_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
chrome/browser/policy/host_blacklist_manager.h:32: // Tracks the blacklist
policies for a given profile, and updates it on changes.
The ownership of this object should be explained in this header file. I think
it's weird that it's refcounted. I suggest you make it owned by the
ProfileIOData. Instantiate it on the UI thread and then pass ownership to the
ProfileIOData. The only weird thing about this then would be the FILE thread
details. You construct the updated blacklist on the FILE thread, and then post
the update to the IO thread, but it may have disappeared by that point. The
solution is to either use PostTaskAndReply() or roll your own tasks. I suggest
you use PostTaskAndReply(). Use weak pointers, so when you bounce back to the IO
thread, then if HostBlacklistManager is already deleted, then the update is
ignored.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
chrome/browser/policy/host_blacklist_manager.h:60: friend class
BuildBlacklistTask;
Why are these friends? Why don't you just make the member functions public? The
ChromeNetworkDelegate should only have const access to the blacklist. And the
Profile should be updating it via non-const member functions.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
File chrome/browser/policy/host_blacklist_manager_unittest.cc (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
chrome/browser/policy/host_blacklist_manager_unittest.cc:20: namespace policy {
Can you put everything inside here within an anonymous namespace?
policy::(anonymous)::Foo.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:480: virtual policy::HostBlacklistManager*
GetHostBlacklistManager() = 0;
Please kill this. You should pass in the HostBlacklistManager via
ProfileImplOData::Handle::Init().

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:281: mutable
scoped_refptr<policy::HostBlacklistManager> host_blacklist_manager_;
Where does ResourceContext use this? I don't see it. Why is it in the "Pointed
to by ResourceContext" section?

[Joao da Silva, 2011-08-29 11:24:25]

@willchan: thanks for the great review! Please have another look.

Nit: the class has been renamed URLBlacklistManager.

The instance is now owned by ProfileIOData. The task that builds the blacklist
on the FILE thread has a weak ptr to the URLBlacklistManager. The weak ptr lives
on the IO thread, where it is checked and, if valid, is used to set the
blacklist. However, the "builder" task is fired from the UI thread, which must
acquire the (IO) weak ptr. That is the reason for the WeakPtrFactory and the
2-step initialization. See comments inline and in the source.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:77: // TODO(joaodasilva): this
error code isn't quite correct
On 2011/08/26 11:40:21, willchan wrote:
> This error code is wrong. Maybe ERR_ACCESS_DENIED would be better. Also, does
> this break the WebRequest API? What's the right way to let the WebRequest API
> know that it's getting denied? I suggest you talk to mpcomplete/battre.

This error was chosen because it's the same that is used by the WebRequest API
when an extension cancels a request. I thought of adding a net error code like
ERR_BLOCKED_BY_POLICY, but that can be considered a layering violation.

There's both ACCESS_DENIED (documented as "Permission to access a resource,
other than the network, was denied."), and NETWORK_ACCESS_DENIED ("Permission to
access the network was denied. This is used to distinguish errors that were most
likely caused by a firewall from other access denied errors.").
NETWORK_ACCESS_DENIED seems more appropriate in this case, WDYT?

I've pinged mpcomplete and battre on the issue too.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
File chrome/browser/policy/host_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
chrome/browser/policy/host_blacklist_manager.h:32: // Tracks the blacklist
policies for a given profile, and updates it on changes.
On 2011/08/26 11:40:21, willchan wrote:
> The ownership of this object should be explained in this header file. I think
> it's weird that it's refcounted. I suggest you make it owned by the
> ProfileIOData. Instantiate it on the UI thread and then pass ownership to the
> ProfileIOData. The only weird thing about this then would be the FILE thread
> details. You construct the updated blacklist on the FILE thread, and then post
> the update to the IO thread, but it may have disappeared by that point. The
> solution is to either use PostTaskAndReply() or roll your own tasks. I suggest
> you use PostTaskAndReply(). Use weak pointers, so when you bounce back to the
IO
> thread, then if HostBlacklistManager is already deleted, then the update is
> ignored.

Done. It took a while to figure how to do this, and I think the current
iteration makes sense. The instance is created and destroyed from profile IO
data, first through the UI thread and then through the IO thread. It's using
WeakPtrFactories to handle the FILE thread issue; a weak ptr (valid on the IO
thread) is passed around while rebuilding the blacklist, and is verified before
calling SetBlacklist. I'm using custom tasks to handle ownership of the vectors
and the Blacklist.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
chrome/browser/policy/host_blacklist_manager.h:60: friend class
BuildBlacklistTask;
On 2011/08/26 11:40:21, willchan wrote:
> Why are these friends? Why don't you just make the member functions public?
The
> ChromeNetworkDelegate should only have const access to the blacklist. And the
> Profile should be updating it via non-const member functions.

Done.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
File chrome/browser/policy/host_blacklist_manager_unittest.cc (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...
chrome/browser/policy/host_blacklist_manager_unittest.cc:20: namespace policy {
On 2011/08/26 11:40:21, willchan wrote:
> Can you put everything inside here within an anonymous namespace?
> policy::(anonymous)::Foo.

Done.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:480: virtual policy::HostBlacklistManager*
GetHostBlacklistManager() = 0;
On 2011/08/26 11:40:21, willchan wrote:
> Please kill this. You should pass in the HostBlacklistManager via
> ProfileImplOData::Handle::Init().

Done.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:281: mutable
scoped_refptr<policy::HostBlacklistManager> host_blacklist_manager_;
On 2011/08/26 11:40:21, willchan wrote:
> Where does ResourceContext use this? I don't see it. Why is it in the "Pointed
> to by ResourceContext" section?

My bad, it's actually pointed to be the network delegate. I've moved it before
the network_delegate_, so that it is destroyed after the delegate is destroyed
too.

[willchan no longer on Chromium, 2011-09-01 00:49:33]

Sorry for the delay, I just got back from vacation. I've emailed you offline
about the ownership issue.

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:77: // TODO(joaodasilva): this
error code isn't quite correct
On 2011/08/29 11:24:25, Joao da Silva wrote:
> On 2011/08/26 11:40:21, willchan wrote:
> > This error code is wrong. Maybe ERR_ACCESS_DENIED would be better. Also,
does
> > this break the WebRequest API? What's the right way to let the WebRequest
API
> > know that it's getting denied? I suggest you talk to mpcomplete/battre.
> 
> This error was chosen because it's the same that is used by the WebRequest API
> when an extension cancels a request. I thought of adding a net error code like
> ERR_BLOCKED_BY_POLICY, but that can be considered a layering violation.
> 
> There's both ACCESS_DENIED (documented as "Permission to access a resource,
> other than the network, was denied."), and NETWORK_ACCESS_DENIED ("Permission
to
> access the network was denied. This is used to distinguish errors that were
most
> likely caused by a firewall from other access denied errors.").
> NETWORK_ACCESS_DENIED seems more appropriate in this case, WDYT?

I think the correct way to handle this is to use ERR_NETWORK_ACCESS_DENIED and
log the more specific reason to the NetLog.

> 
> I've pinged mpcomplete and battre on the issue too.

This is quite important, so please talk to them and report back.

[willchan no longer on Chromium, 2011-09-01 17:05:04]

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:63: class SetBlacklistTask :
public Task {
Rather than write a bunch of Tasks, you should probably use
NewRunnableFunction().

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:171:
io_weak_ptr_factory_.InvalidateWeakPtrs();
Unnecessary, the destructor for the factory will do this automagically.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:210: BrowserThread::PostTask(
Rather than going directly to the FILE thread, please bounce through the IO
thread. As I noted previously, UI=>IO thread is safe due to
Profile=>ProfileIOData destruction ordering, so you don't need a weak pointer
for this.

On the IO thread, post to the FILE thread. You can acquire the WeakPtr there.
This eliminates the need to cache |weak_ptr_| which would get initialized on the
IO thread and used on the UI thread. This helps reduce the cognitive overhead
for figuring out which threads use a member variable.

For the BuildBlacklistTask and SetBlacklistTask, I recommend you use
BrowserThread::PostTaskAndReply(). Replace BuildBlacklistTask and
SetBlacklistTask with BuildBlacklist (same implementation as
BuildBlacklistTask::Run()) and SetBlacklist (same implementation as
SetBlacklistTask::Run(), except you need to remember to delete the data if the
weak pointer is invalid).

This helps eliminate much of the boilerplate of BuildBlacklistTask and
SetBlacklistTask.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:223: if (!blacklist_.get())
I prefer not to let variables be NULL if reasonable/possible. It reduces the
possible code path combinations when you eliminate NULL checks. Can you just
create an empty Blacklist in the constructor?

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:64: explicit
URLBlacklistManager(Profile* profile);
Please pass in PrefService instead. It's a bit lame to have to create a whole
Profile for the test, when you only use the PrefService. Dependency reduction is
good!

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:87: virtual void
PostUpdateTask(Task* task);  // Virtual for testing.
Can you move this to private? I don't think it ever gets called in the test.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:101: // Used to get |weak_ptr_| to
self on the IO thread.
For clarity's sake, please organize these member variables by UI and IO thread
ownership. Use banner comments like

// --------
// UI thread
// --------
...

// --------
// IO thread
// --------
...

I have some suggestions to restrict member variables to use on certain threads
as much as possible.

[Joao da Silva, 2011-09-02 14:31:02]

@willchan: thanks for another great review! Please have another look.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:63: class SetBlacklistTask :
public Task {
On 2011/09/01 17:05:04, willchan wrote:
> Rather than write a bunch of Tasks, you should probably use
> NewRunnableFunction().

The purpose of the tasks was to hold ownership of the stuff passed to them
(blacklist, or the string vectors). I looked a bit more around after our
discussion, and am now using PostTaskAndReply for this, and linked_ptrs to
manage the memory. See below.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:171:
io_weak_ptr_factory_.InvalidateWeakPtrs();
On 2011/09/01 17:05:04, willchan wrote:
> Unnecessary, the destructor for the factory will do this automagically.

Done.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:210: BrowserThread::PostTask(
On 2011/09/01 17:05:04, willchan wrote:
> Rather than going directly to the FILE thread, please bounce through the IO
> thread. As I noted previously, UI=>IO thread is safe due to
> Profile=>ProfileIOData destruction ordering, so you don't need a weak pointer
> for this.
> 
> On the IO thread, post to the FILE thread. You can acquire the WeakPtr there.
> This eliminates the need to cache |weak_ptr_| which would get initialized on
the
> IO thread and used on the UI thread. This helps reduce the cognitive overhead
> for figuring out which threads use a member variable.
> 
> For the BuildBlacklistTask and SetBlacklistTask, I recommend you use
> BrowserThread::PostTaskAndReply(). Replace BuildBlacklistTask and
> SetBlacklistTask with BuildBlacklist (same implementation as
> BuildBlacklistTask::Run()) and SetBlacklist (same implementation as
> SetBlacklistTask::Run(), except you need to remember to delete the data if the
> weak pointer is invalid).
> 
> This helps eliminate much of the boilerplate of BuildBlacklistTask and
> SetBlacklistTask.

PostTaskAndReply turned out to be very cool for this indeed, and the custom
tasks aren't needed anymore, nor the |weak_ptr_|. The only issue left in my mind
was the ownership of the StringVectors* and the Blacklist* passed with the
callbacks, which could potentially leak during shutdown. A hint from Mattias
pointed to linked_ptr, which solves the problem nicely. That's the only addition
to your proposals, and prevents the leaks too.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:223: if (!blacklist_.get())
On 2011/09/01 17:05:04, willchan wrote:
> I prefer not to let variables be NULL if reasonable/possible. It reduces the
> possible code path combinations when you eliminate NULL checks. Can you just
> create an empty Blacklist in the constructor?

Done.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:64: explicit
URLBlacklistManager(Profile* profile);
On 2011/09/01 17:05:04, willchan wrote:
> Please pass in PrefService instead. It's a bit lame to have to create a whole
> Profile for the test, when you only use the PrefService. Dependency reduction
is
> good!

Done.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:87: virtual void
PostUpdateTask(Task* task);  // Virtual for testing.
On 2011/09/01 17:05:04, willchan wrote:
> Can you move this to private? I don't think it ever gets called in the test.

The test just overrides this and posts the task, but ignores the delay. It will
actually be called when the preferences are changed in the test, and is used to
call Update immediately without having to wait.

http://codereview.chromium.org/7747018/diff/20001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:101: // Used to get |weak_ptr_| to
self on the IO thread.
On 2011/09/01 17:05:04, willchan wrote:
> For clarity's sake, please organize these member variables by UI and IO thread
> ownership. Use banner comments like
> 
> // --------
> // UI thread
> // --------
> ...
> 
> // --------
> // IO thread
> // --------
> ...
> 
> I have some suggestions to restrict member variables to use on certain threads
> as much as possible.

Great idea, done.

[Joao da Silva, 2011-09-02 15:07:01]

Adding @battre to review interaction with extensions.

[battre, 2011-09-02 15:42:13]

On 2011/09/02 15:07:01, Joao da Silva wrote:
> Adding @battre to review interaction with extensions.

As discussed, the approach of blocking the request by the blacklist before an
extension can redirects LGTM. I think in the future, it would be nice, if
extensions could intervene before the blacklist is queried.

[willchan no longer on Chromium, 2011-09-02 16:39:26]

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:92:
URLBlacklistManager::~URLBlacklistManager() {
Can you add a DCHECK here for BrowserThread::IO?

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:118: // This is overriden in
tests to post the task without the delay.
overridden

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:119:
MessageLoop::current()->PostDelayedTask(FROM_HERE, task, kUpdateDelayMs);
I had toyed earlier with replacing this with base::Timer, which would get rid of
the ui_method_factory_ use for the most part as well. But it doesn't let you
override for tests like you've done here.

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:128: linked_ptr<StringVector>
block(
linked_ptr doesn't work because it isn't threadsafe. The linked_ptr on the stack
here and the linked_ptr bound into the closure returned by base::Bind may be
destroyed at the same time on different threads. Therefore, we have a race on
the circular linked list in linked_ptr.

If you really want to do this, we'd need shared_ptr or you'd have to wrap these
data structures a container class inheriting from RefCountedThreadSafe.

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:34: Blacklist() {
This is totally fine now and you were probably already planning on moving the
definition to the .cc file in the future, but just in case, I wanted to say that
you should do this. In fact, I think the clang builder will force you to do
this. Inline constructors and destructors can lead to code bloat.

[Joao da Silva, 2011-09-02 17:35:20]

@willchan: reviewed, please have yet another look. Thanks :-)

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:92:
URLBlacklistManager::~URLBlacklistManager() {
On 2011/09/02 16:39:26, willchan wrote:
> Can you add a DCHECK here for BrowserThread::IO?

Done.

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:118: // This is overriden in
tests to post the task without the delay.
On 2011/09/02 16:39:26, willchan wrote:
> overridden

Done.

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:128: linked_ptr<StringVector>
block(
On 2011/09/02 16:39:26, willchan wrote:
> linked_ptr doesn't work because it isn't threadsafe. The linked_ptr on the
stack
> here and the linked_ptr bound into the closure returned by base::Bind may be
> destroyed at the same time on different threads. Therefore, we have a race on
> the circular linked list in linked_ptr.
> 
> If you really want to do this, we'd need shared_ptr or you'd have to wrap
these
> data structures a container class inheriting from RefCountedThreadSafe.

I'd really like to make sure there is always an owner of the memory. For the
current CL I'll let it leak in the (very unlikely) case of a policy update
during shutdown. I'll cc you later on a proposal for these cases.

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/26001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:34: Blacklist() {
On 2011/09/02 16:39:26, willchan wrote:
> This is totally fine now and you were probably already planning on moving the
> definition to the .cc file in the future, but just in case, I wanted to say
that
> you should do this. In fact, I think the clang builder will force you to do
> this. Inline constructors and destructors can lead to code bloat.

I've moved it to the .cc. Thanks for the info.

[willchan no longer on Chromium, 2011-09-03 20:28:04]

LGTM

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:105:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
OIC, this is causing you crashes on the linux_chromeos tests due to the test
being lame. That was really hard to work around, so I'm fine with you disabling
this for now.

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:147:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
Please format like the style guide says:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....
Arguments on new lines must align with previous arguments.

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:159:
BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
Ditto

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:32: class Blacklist {
I think this should be named URLBlacklist because a Blacklist is too generic.

[Joao da Silva, 2011-09-04 17:35:52]

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:105:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2011/09/03 20:28:04, willchan wrote:
> OIC, this is causing you crashes on the linux_chromeos tests due to the test
> being lame. That was really hard to work around, so I'm fine with you
disabling
> this for now.

Done.

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:147:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
On 2011/09/03 20:28:04, willchan wrote:
> Please format like the style guide says:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....
> Arguments on new lines must align with previous arguments.

Done.

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:159:
BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
On 2011/09/03 20:28:04, willchan wrote:
> Ditto

Done.

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7747018/diff/27017/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:32: class Blacklist {
On 2011/09/03 20:28:04, willchan wrote:
> I think this should be named URLBlacklist because a Blacklist is too generic.

Done.

[commit-bot: I haz the power, 2011-09-04 19:20:02]

Change committed as 99608