Introduced the URLBlacklistManager, and wired it to various places.

chrome/browser/policy/url_blacklist_manager.cc

Index: chrome/browser/policy/url_blacklist_manager.cc
diff --git a/chrome/browser/policy/url_blacklist_manager.cc b/chrome/browser/policy/url_blacklist_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dec0d5e7e5b90b3373a49a8cb04f6d90312a72a3
--- /dev/null
+++ b/chrome/browser/policy/url_blacklist_manager.cc
@@ -0,0 +1,238 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/policy/url_blacklist_manager.h"
+
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/pref_names.h"
+#include "content/browser/browser_thread.h"
+#include "content/common/notification_details.h"
+#include "content/common/notification_source.h"
+#include "googleurl/src/gurl.h"
+
+namespace policy {
+
+namespace {
+
+// Time to wait before starting an update of the blacklist. Scheduling another
+// update during this period will reset the timer.
+const int64 kUpdateDelayMs = 1000;
+
+// Maximum filters per policy. Filters over this index are ignored.
+const size_t kMaxFiltersPerPolicy = 100;
+
+typedef std::vector<std::string> StringVector;
+
+StringVector* ListValueToStringVector(const base::ListValue* list) {
+  StringVector* vector = new StringVector;
+
+  if (!list)
+    return vector;
+
+  vector->reserve(list->GetSize());
+  std::string s;
+  for (base::ListValue::const_iterator it = list->begin();
+       it != list->end() && vector->size() < kMaxFiltersPerPolicy; ++it) {
+    if ((*it)->GetAsString(&s))
+      vector->push_back(s);
+  }
+
+  return vector;
+}
+
+}  // namespace
+
+// TODO(joaodasilva): this is a work in progress. http://crbug.com/49612
+class Blacklist {
+ public:
+  Blacklist() {
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(Blacklist);
+};
+
+namespace {
+
+// A task that owns the Blacklist, and passes it to the URLBlacklistManager
+// on the IO thread, if the URLBlacklistManager still exists.
+class SetBlacklistTask : public Task {

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

Rather than write a bunch of Tasks, you should probably use
NewRunnableFunction().

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

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.

+ public:
+  SetBlacklistTask(
+      base::WeakPtr<URLBlacklistManager> url_blacklist_manager,
+      Blacklist* blacklist)
+      : url_blacklist_manager_(url_blacklist_manager),
+        blacklist_(blacklist) {
+  }
+
+  virtual void Run() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    if (url_blacklist_manager_)
+      url_blacklist_manager_->SetBlacklist(blacklist_.release());
+  }
+
+ private:
+  base::WeakPtr<URLBlacklistManager> url_blacklist_manager_;
+  scoped_ptr<Blacklist> blacklist_;
+  DISALLOW_COPY_AND_ASSIGN(SetBlacklistTask);
+};
+
+// A task that builds the blacklist on the FILE thread, and then posts another
+// task to pass it to the URLBlacklistManager on the IO thread.
+class BuildBlacklistTask : public Task {
+ public:
+  BuildBlacklistTask(
+      base::WeakPtr<URLBlacklistManager> url_blacklist_manager,
+      StringVector* blacklist,
+      StringVector* whitelist)
+      : url_blacklist_manager_(url_blacklist_manager),
+        blacklist_(blacklist),
+        whitelist_(whitelist) {
+  }
+
+  virtual void Run() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+    Blacklist* blacklist = NULL;
+    if (!blacklist_->empty()) {
+      blacklist = new Blacklist;
+      // TODO(joaodasilva): this is a work in progress. http://crbug.com/49612
+    }
+
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        new SetBlacklistTask(url_blacklist_manager_, blacklist));
+  }
+
+ private:
+  base::WeakPtr<URLBlacklistManager> url_blacklist_manager_;
+  scoped_ptr<StringVector> blacklist_;
+  scoped_ptr<StringVector> whitelist_;
+  DISALLOW_COPY_AND_ASSIGN(BuildBlacklistTask);
+};
+
+}  // namespace
+
+URLBlacklistManager::URLBlacklistManager(Profile* profile)
+    : ALLOW_THIS_IN_INITIALIZER_LIST(ui_method_factory_(this)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(io_weak_ptr_factory_(this)),
+      pref_service_(profile->GetPrefs()) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // This has to be created on the UI thread, but is only posted later, after
+  // the initialization on the IO thread is complete.
+  // Posting a task to invoke InitializeOnIOThread from here isn't safe, since
+  // we can't get weak ptrs for the IO thread yet.
+  initialize_on_ui_task_ = ui_method_factory_.NewRunnableMethod(
+      &URLBlacklistManager::InitializeOnUIThread);
+}
+
+void URLBlacklistManager::InitializeOnIOThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(initialize_on_ui_task_ != NULL);
+  io_weak_ptr_factory_.DetachFromThread();
+  weak_ptr_ = io_weak_ptr_factory_.GetWeakPtr();
+
+  // It is now safe to resume initialization on the UI thread, since it is now
+  // possible to get weak refs to self to use on the IO thread.
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, initialize_on_ui_task_);
+}
+
+void URLBlacklistManager::InitializeOnUIThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(initialize_on_ui_task_ != NULL);
+  initialize_on_ui_task_ = NULL;
+
+  pref_change_registrar_.Init(pref_service_);
+  pref_change_registrar_.Add(prefs::kUrlBlacklist, this);
+  pref_change_registrar_.Add(prefs::kUrlWhitelist, this);
+
+  // Start enforcing the policies without a delay when they are present at
+  // startup.
+  if (pref_service_->HasPrefPath(prefs::kUrlBlacklist))
+    Update();
+}
+
+void URLBlacklistManager::ShutdownOnUIThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Cancel any pending updates, and stop listening for pref change updates.
+  // This also cancels a pending InitializeOnUIThread, if it hasn't executed
+  // yet.
+  ui_method_factory_.RevokeAll();
+  pref_change_registrar_.RemoveAll();
+}
+
+URLBlacklistManager::~URLBlacklistManager() {
+  // Cancel any weak ptrs on the IO thread. Pending tasks won't execute their
+  // updates anymore.
+  io_weak_ptr_factory_.InvalidateWeakPtrs();

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

Unnecessary, the destructor for the factory will do this automagically.

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

Done.

+}
+
+void URLBlacklistManager::Observe(int type,
+                                  const NotificationSource& source,
+                                  const NotificationDetails& details) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(type == chrome::NOTIFICATION_PREF_CHANGED);
+  PrefService* prefs = Source<PrefService>(source).ptr();
+  DCHECK(prefs == pref_service_);
+  std::string* pref_name = Details<std::string>(details).ptr();
+  DCHECK(*pref_name == prefs::kUrlBlacklist ||
+         *pref_name == prefs::kUrlWhitelist);
+  ScheduleUpdate();
+}
+
+void URLBlacklistManager::ScheduleUpdate() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Cancel pending updates, if any.
+  ui_method_factory_.RevokeAll();
+  PostUpdateTask(
+      ui_method_factory_.NewRunnableMethod(&URLBlacklistManager::Update));
+}
+
+void URLBlacklistManager::PostUpdateTask(Task* task) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // This is overriden in tests to post the task without the delay.
+  MessageLoop::current()->PostDelayedTask(FROM_HERE, task, kUpdateDelayMs);
+}
+
+void URLBlacklistManager::Update() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // The preferences can only be read on the UI thread.
+  StringVector* blacklist = ListValueToStringVector(
+      pref_service_->GetList(prefs::kUrlBlacklist));
+  StringVector* whitelist = ListValueToStringVector(
+      pref_service_->GetList(prefs::kUrlWhitelist));
+
+  BrowserThread::PostTask(

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

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.

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

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.

+      BrowserThread::FILE, FROM_HERE,
+      new BuildBlacklistTask(weak_ptr_, blacklist, whitelist));
+}
+
+void URLBlacklistManager::SetBlacklist(Blacklist* blacklist) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  blacklist_.reset(blacklist);
+}
+
+bool URLBlacklistManager::IsURLBlocked(const GURL& url) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (!blacklist_.get())

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

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?

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

Done.

+    return false;
+
+  // TODO(joaodasilva): this is a work in progress. http://crbug.com/49612
+  return false;
+}
+
+// static
+void URLBlacklistManager::RegisterPrefs(PrefService* pref_service) {
+  pref_service->RegisterListPref(prefs::kUrlBlacklist,
+                                 PrefService::UNSYNCABLE_PREF);
+  pref_service->RegisterListPref(prefs::kUrlWhitelist,
+                                 PrefService::UNSYNCABLE_PREF);
+}
+
+}  // namespace policy