Refactor URLBlacklistManager to use URLMatcher.

chrome/browser/managed_mode.cc

Index: chrome/browser/managed_mode.cc
diff --git a/chrome/browser/managed_mode.cc b/chrome/browser/managed_mode.cc
index e9740520dee396e19aa247293df6b817f4051240..5698d6b77204081e60487b3d2b01e7c27e1e259d 100644
--- a/chrome/browser/managed_mode.cc
+++ b/chrome/browser/managed_mode.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/managed_mode_url_filter.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_modal_dialogs/app_modal_dialog.h"
@@ -17,12 +18,55 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/browser_thread.h"

[Joao da Silva, 2012/07/13 12:32:11]

Nit: order

[Bernhard Bauer, 2012/07/13 14:21:07]

Done.

 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
+using content::BrowserThread;
+
+// A bridge from ManagedMode (which lives on the UI thread) to
+// ManagedModeURLFilter (which lives on the IO thread).
+class ManagedMode::URLFilterContext {
+ public:
+  URLFilterContext() {}
+  ~URLFilterContext() {}
+
+  const ManagedModeURLFilter* url_filter() const {
+    return &url_filter_;
+  }
+
+  void SetActive(bool in_managed_mode) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    // Because ManagedMode is a singleton, we can pass the pointer to
+    // |url_filter_| unretained.

[Joao da Silva, 2012/07/13 12:32:11]

What do you think of this pattern instead:

- ManagedMode owns ManagedModeURLFilter. It is created from UI but lives on IO.
- Every call to the filter is posted Unretained() to IO
- The destructor of ManagedMode() posts DeleteSoon() to IO to delete the filter.
This makes sure any task previously posted is executed before deletion.
- The getter can still DCHECK it's on IO and get the filter, since as long as
the ManagedMode instance is alive then so is the filter on IO.

[Bernhard Bauer, 2012/07/13 14:21:07]

Yup; see the comment below.
This won't work with the current lifetime model of ManagedMode. It's a
singleton, so it will be destroyed *very* late during shutdown. Specifically, at
this point not only is the IO thread already shut down (which would just result
in a leak), but the whole BrowserThread infrastructure (which uses singletons)
is as well, which means I can't even call PostTask anymore. 

What I decided to do instead is to make the singleton leaky, which shouldn't
cause any problems (and it doesn't make shutdown take longer). I'll still add
the DeleteSoon, but it's only going to be called in tests et al.
Do we really need to make sure that a posted task actually gets executed when
we're shutting down?
***

I'd still like to keep the URLFilterContext around as a bridge that can be used
by ManagedMode from the UI thread. It also simplifies things if we change the
lifetime of ManagedMode at some point in the future so it gets destructed
earlier. Then we only need to make URLFilterContext RefCountedThreadSafe and can
post tasks on it.

+    BrowserThread::PostTask(BrowserThread::IO,
+                            FROM_HERE,
+                            base::Bind(
+                                &ManagedModeURLFilter::SetActive,
+                                base::Unretained(&url_filter_),
+                                in_managed_mode));
+  }
+
+  void SetWhitelist(scoped_ptr<base::ListValue> whitelist) {

[Joao da Silva, 2012/07/13 12:32:11]

Is this used?

[Bernhard Bauer, 2012/07/13 14:21:07]

Not yet, but it will be once I have the source of the whitelist. Like I said,
this is somewhat of a work in progress.

+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    BrowserThread::PostTask(BrowserThread::IO,
+                            FROM_HERE,
+                            base::Bind(
+                                &ManagedModeURLFilter::SetWhitelist,
+                                base::Unretained(&url_filter_),
+                                base::Passed(&whitelist),
+                                base::Closure()));
+  }
+
+ private:
+  ManagedModeURLFilter url_filter_;
+
+  DISALLOW_COPY_AND_ASSIGN(URLFilterContext);
+};
+
 // static
 ManagedMode* ManagedMode::GetInstance() {
-  return Singleton<ManagedMode>::get();
+  return Singleton<ManagedMode, LeakySingletonTraits<ManagedMode> >::get();
 }
 
 // static
@@ -132,6 +176,18 @@ void ManagedMode::LeaveManagedModeImpl() {
     SetInManagedMode(NULL);
 }
 
+const ManagedModeURLFilter* ManagedMode::GetURLFilter() {

[battre, 2012/07/13 14:10:53]

nit: // static

[Bernhard Bauer, 2012/07/13 14:21:07]

Done.

+  return GetInstance()->GetURLFilterImpl();
+}
+
+const ManagedModeURLFilter* ManagedMode::GetURLFilterImpl() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!url_filter_context_.get())
+    url_filter_context_.reset(new URLFilterContext);
+
+  return url_filter_context_->url_filter();
+}
+
 std::string ManagedMode::GetDebugPolicyProviderName() const {
   // Save the string space in official builds.
 #ifdef NDEBUG
@@ -192,6 +248,8 @@ ManagedMode::ManagedMode() : managed_profile_(NULL) {
 }
 
 ManagedMode::~ManagedMode() {
+  // This class usually is a leaky singleton, so this destructor shouldn't be
+  // called. We still do some cleanup, in case we're owned by a unit test.
   BrowserList::RemoveObserver(this);
   DCHECK_EQ(0u, callbacks_.size());
   DCHECK_EQ(0u, browsers_to_close_.size());
@@ -248,16 +306,19 @@ void ManagedMode::SetInManagedMode(Profile* newly_managed_profile) {
   // Register the ManagementPolicy::Provider before changing the pref when
   // setting it, and unregister it after changing the pref when clearing it,
   // so pref observers see the correct ManagedMode state.
-  if (newly_managed_profile) {
+  bool in_managed_mode = !!newly_managed_profile;
+  if (in_managed_mode) {
     DCHECK(!managed_profile_ || managed_profile_ == newly_managed_profile);
     extensions::ExtensionSystem::Get(
         newly_managed_profile)->management_policy()->RegisterProvider(this);
-    g_browser_process->local_state()->SetBoolean(prefs::kInManagedMode, true);
   } else {
     extensions::ExtensionSystem::Get(
         managed_profile_)->management_policy()->UnregisterProvider(this);
-    g_browser_process->local_state()->SetBoolean(prefs::kInManagedMode, false);
   }
+
+  url_filter_context_->SetActive(in_managed_mode);

[Joao da Silva, 2012/07/13 12:32:11]

Is this guaranteed to be non-NULL at this stage?

[Bernhard Bauer, 2012/07/13 14:21:07]

Hm, good point. I probably do need to set up everything on the UI thread first.

+  g_browser_process->local_state()->SetBoolean(prefs::kInManagedMode,
+                                               in_managed_mode);
   managed_profile_ = newly_managed_profile;
 
   // This causes the avatar and the profile menu to get updated.