Split content settings pref related logic into a separate class

chrome/browser/content_settings/content_settings_pref_provider_unittest.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 "components/content_settings/core/browser/content_settings_pref_provider.h"
 
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/default_pref_store.h"
 #include "base/prefs/overlay_user_pref_store.h"
 #include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/prefs/testing_pref_store.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/platform_thread.h"
 #include "base/values.h"
 #include "chrome/browser/content_settings/content_settings_mock_observer.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/prefs/pref_service_mock_factory.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/content_settings/core/browser/content_settings_pref.h"
 #include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/test/content_settings_test_utils.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 using ::testing::_;
 
 namespace content_settings {
 
 class DeadlockCheckerThread : public base::PlatformThread::Delegate {
  public:
   explicit DeadlockCheckerThread(PrefProvider* provider)
       : provider_(provider) {}
 
   void ThreadMain() override {
-    bool got_lock = provider_->lock_.Try();
+    bool got_lock = provider_->content_settings_pref()->lock_.Try();
     EXPECT_TRUE(got_lock);
     if (got_lock)
-      provider_->lock_.Release();
+      provider_->content_settings_pref()->lock_.Release();
   }
  private:
   PrefProvider* provider_;
   DISALLOW_COPY_AND_ASSIGN(DeadlockCheckerThread);
 };
 
 // A helper for observing an preference changes and testing whether
 // |PrefProvider| holds a lock when the preferences change.
 class DeadlockCheckerObserver {
  public:
   // |DeadlockCheckerObserver| doesn't take the ownership of |prefs| or
-  // ||provider|.
+  // |provider|.
   DeadlockCheckerObserver(PrefService* prefs, PrefProvider* provider)
       : provider_(provider),
       notification_received_(false) {
     pref_change_registrar_.Init(prefs);
     pref_change_registrar_.Add(
         prefs::kContentSettingsPatternPairs,

[msramek, 2015/03/09 19:02:29]

This feels incorrect to me, since in this unittest we're testing the Provider
class, but kContentSettingsPatternPairs is a property of the new Pref class.
Shouldn't we have this callback triggered by Notify() or NotifyObservers()?
Then, we wouldn't have to reach one layer under by invoking 

provider_->content_settings_pref()

above. It's maybe not a big deal now, but it will get weird when we split it to
N preferences and then test all N locks here.

[raymes, 2015/03/09 22:13:52]

Hmm I'm not exactly sure what you mean. Could you sketch the code out for me so
I can see more clearly? 

One thought I had is to not access the lock directly above. Instead we could
just call SetWebsiteSetting which is a part of the public interface and will try
to acquire the lock. If it can't get it, it will deadlock the test and timeout.

Maybe what we need to do is add a separate unittest for ContentSettingsPref
after it is changed to store an individual content setting. Then the deadlock
test would be a part of this, since all the locking happens inside of that
class. WDYT?

[msramek, 2015/03/10 19:40:36]

What I had in mind was - when kContentSettingsPatternPairs callback fires, the
provider will call NotifyObservers(), so we'd add this class as an observer...
or not, because this is a unittest, not a browsertest. Argh. Sorry for the
confusion.

An easy way out is to wrap

provider_->content_settings_pref()->lock_

into

PrefProvider::GetPrefLock()

which doesn't need to be public; instead, we can add DeadlockCheckerThread as a
friend (it originally was, now it's a friend of the Pref class). But that's
still ugly.

I agree that SetWebsiteSetting() probably should do the trick.

And of course, adding a separate unittest for the Pref class is the cleanest and
correct way to do it. I just wanted to avoid suggesting structural changes, so
that you can land this ASAP. We can polish and refactor later :)

         base::Bind(
             &DeadlockCheckerObserver::OnContentSettingsPatternPairsChanged,
             base::Unretained(this)));
   }
   virtual ~DeadlockCheckerObserver() {}
 
   bool notification_received() const {
     return notification_received_;
   }
 
@@ skipping 381 matching lines @@
   base::Time second = pref_content_settings_provider.GetLastUsage(
       pattern, pattern, CONTENT_SETTINGS_TYPE_GEOLOCATION);
 
   base::TimeDelta delta = second - first;
   EXPECT_EQ(delta.InSeconds(), 10);
 
   pref_content_settings_provider.ShutdownOnUIThread();
 }
 
 }  // namespace content_settings