Refactor URLBlacklistManager to use URLMatcher.

chrome/browser/managed_mode.cc

 // Copyright (c) 2012 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/managed_mode.h"
 
 #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"
 #include "chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/chrome_notification_types.h"
 #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
 void ManagedMode::RegisterPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kInManagedMode, false);
 }
 
 // static
 void ManagedMode::Init(Profile* profile) {
   GetInstance()->InitImpl(profile);
@@ skipping 89 matching lines @@
 void ManagedMode::LeaveManagedMode() {
   GetInstance()->LeaveManagedModeImpl();
 }
 
 void ManagedMode::LeaveManagedModeImpl() {
   bool confirmed = PlatformConfirmLeave();
   if (confirmed)
     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
   NOTREACHED();
   return std::string();
 #else
   return "Managed Mode";
 #endif
 }
 
@@ skipping 40 matching lines @@
   DCHECK_EQ(1u, count);
   if (browsers_to_close_.empty())
     FinalizeEnter(true);
 }
 
 ManagedMode::ManagedMode() : managed_profile_(NULL) {
   BrowserList::AddObserver(this);
 }
 
 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());
 }
 
 void ManagedMode::Observe(int type,
                           const content::NotificationSource& source,
                           const content::NotificationDetails& details) {
   // Return early if we don't have any queued callbacks.
   if (callbacks_.empty())
@@ skipping 36 matching lines @@
 
 bool ManagedMode::PlatformConfirmLeave() {
   // TODO(bauerb): Show platform-specific confirmation dialog.
   return true;
 }
 
 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.
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
       content::NotificationService::AllBrowserContextsAndSources(),
       content::NotificationService::NoDetails());
 }