Integrate SiteEngagementEvictionPolicy with QuotaManager.

chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc

Index: chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
diff --git a/chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc b/chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
index 733c5aa6d3b02725439a3b1017cf3ef5809d1e99..05e38f59f18c0568a784622bc33d42a6f38cbf0c 100644
--- a/chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
+++ b/chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
@@ -2,16 +2,30 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/run_loop.h"
+#include "base/thread_task_runner_handle.h"
 #include "chrome/browser/engagement/site_engagement_eviction_policy.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "content/public/test/mock_special_storage_policy.h"
+#include "content/public/test/mock_storage_client.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "storage/browser/quota/quota_manager.h"
+#include "storage/browser/quota/quota_manager_proxy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
+const char* kFooOrigin = "http://foo.com/";
+
 const int64 kGlobalQuota = 25 * 1024;
 
-}  // namespace
+// Returns a deterministic value for the amount of available disk space.
+int64 GetAvailableDiskSpaceForTest(const base::FilePath&) {
+  return kGlobalQuota;
+}
 
 class TestSiteEngagementScoreProvider : public SiteEngagementScoreProvider {
  public:
@@ -40,6 +54,108 @@ class TestSiteEngagementScoreProvider : public SiteEngagementScoreProvider {
   DISALLOW_COPY_AND_ASSIGN(TestSiteEngagementScoreProvider);
 };
 
+SiteEngagementScoreProvider* GetTestSiteEngagementScoreProvider(
+    content::BrowserContext*) {
+  CR_DEFINE_STATIC_LOCAL(TestSiteEngagementScoreProvider,
+                         kGlobalTestScoreProvider, ());
+  return &kGlobalTestScoreProvider;
+}
+
+}  // namespace
+
+class SiteEngagementEvictionPolicyWithQuotaManagerTest : public testing::Test {

[michaeln, 2015/10/12 23:36:26]

The production code looks pretty straight forward but the test code seems overly
complicated somehow? The ratio of test apparatus to production code is off
balance. Some thoughts on that.

I think there are two things to test...

1) Ensure the QuotaManager invokes a caller provided eviction policy. This test
doesn't need to use the SiteEngagementEvictionPolicy at all, would that keeps
track of whether its been called would be sufficient. A test like that probably
belongs in quota_manager_unittest.cc (or maybe in
quota_temporary_storage_evictor_unittest.cc).

2) Ensure the logic in the SiteEngagementEvictionPolicy does what you want. It's
a class with one public method. The most direct way of testing that is to simply
allocate the object on the stack and call its GetEvictionOrigin() method and see
that it does what you want.  I don't think the CalculateEvictionOriginForTests
function is needed really. Also, what if the policy class was constructed with a
score provider, I think that might make tests easier to compose w/o special
cases, wdyt?

class SiteEngagementEvictionPolicy {
 public:
  explicit SiteEngagementEvictionPolicy(scoped_ptr<SiteEngagementScoreProvider>
score_provider);
 private:
  score_provider_;
};

[calamity, 2015/10/13 08:34:40]

Ah yeah, this was an integration test with the QuotaManager that was good to
have when the policy was asking the QuotaManager for all its data. It's no
longer necessary so removed.
Agreed. Done.
The browser context is used to ensure lifetime is not an issue. This accounts
for the case where the SiteEngagementService is already destroyed when the UI
thread task is run.

If there are no objections to keeping the BrowserContext as the ScoreProvider
key, I think CalculateEvictionOriginForTests is a good testing API because it
directly tests the business logic of the class and is a nice place to inject a
ScoreProvider for testing.

[michaeln, 2015/10/13 20:13:39]

Ok, thnx for getting rid of the global ptr in the production code and the test
function will get stripped out by the linker.

+ public:
+  SiteEngagementEvictionPolicyWithQuotaManagerTest() : weak_factory_(this) {}
+
+  void SetUp() override {
+    ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
+    mock_special_storage_policy_ = new content::MockSpecialStoragePolicy;
+
+    quota_manager_ = new storage::QuotaManager(
+        false, data_dir_.path(), base::ThreadTaskRunnerHandle::Get().get(),

[michaeln, 2015/10/12 23:36:26]

The trailing .get()s are not needed (or wanted really). The QuotaManager
constructor accepts const refs to scoped_refptr<T>s.

(You probably copy/pasted this from another test. In the distant past, i think
the ctor took rawptr types as inputs.)

[calamity, 2015/10/13 08:34:40]

N/A.

+        base::ThreadTaskRunnerHandle::Get().get(),
+        mock_special_storage_policy_.get());
+
+    // Populate the QuotaManager with some data.
+    static const content::MockOriginData kData[] = {
+        {kFooOrigin, storage::kStorageTypeTemporary, 1},
+    };
+    quota_manager_->proxy()->RegisterClient(new content::MockStorageClient(
+        quota_manager_->proxy(), kData, storage::QuotaClient::kFileSystem,
+        arraysize(kData)));
+
+    // Set the eviction policy.
+    quota_manager_->SetTemporaryStorageEvictionPolicy(
+        make_scoped_ptr(new SiteEngagementEvictionPolicy(nullptr)));
+
+    // Don't (automatically) start the eviction for testing.
+    quota_manager_->eviction_disabled_ = true;
+    // Don't query the hard disk for remaining capacity.
+    quota_manager_->get_disk_space_fn_ = &GetAvailableDiskSpaceForTest;
+
+    score_provider_callback_ = base::Bind(&GetTestSiteEngagementScoreProvider);
+
+    // Use a TestSiteEngagementScoreProvider when retrieving the engagement
+    // scores.
+    SiteEngagementEvictionPolicy::
+        SetSiteEngagementScoreProviderCallbackForTests(
+            &score_provider_callback_);
+  }
+
+  void TearDown() override {
+    SiteEngagementEvictionPolicy::
+        SetSiteEngagementScoreProviderCallbackForTests(nullptr);
+    // Make sure the quota manager cleans up correctly.
+    quota_manager_ = nullptr;
+    base::RunLoop().RunUntilIdle();
+  }
+
+ protected:
+  void GetEvictionOrigin() {
+    quota_manager_->GetUsageAndQuotaForEviction(base::Bind(
+        &SiteEngagementEvictionPolicyWithQuotaManagerTest::DoGetEvictionOrigin,
+        base::Unretained(this)));
+  }
+
+  void DoGetEvictionOrigin(storage::QuotaStatusCode status,
+                           const storage::UsageAndQuota& usage_and_quota) {
+    quota_manager_->GetEvictionOrigin(
+        storage::kStorageTypeTemporary, kGlobalQuota,
+        base::Bind(&SiteEngagementEvictionPolicyWithQuotaManagerTest::
+                       OnDidGetEvictionOrigin,
+                   weak_factory_.GetWeakPtr()));
+  }
+
+  void OnDidGetEvictionOrigin(const GURL& origin) { eviction_origin_ = origin; }
+
+  const GURL& eviction_origin() { return eviction_origin_; }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  base::ScopedTempDir data_dir_;
+
+  scoped_refptr<storage::QuotaManager> quota_manager_;
+  scoped_refptr<content::MockSpecialStoragePolicy> mock_special_storage_policy_;
+  base::Callback<SiteEngagementScoreProvider*(content::BrowserContext*)>
+      score_provider_callback_;
+
+  GURL eviction_origin_;
+
+  base::WeakPtrFactory<SiteEngagementEvictionPolicyWithQuotaManagerTest>
+      weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SiteEngagementEvictionPolicyWithQuotaManagerTest);
+};
+
+// Ensure the SiteEngagementEvictionPolicy completes successfully
+// when used as a QuotaManager's eviction policy.
+TEST_F(SiteEngagementEvictionPolicyWithQuotaManagerTest, GetEvictionOrigin) {
+  GetEvictionOrigin();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(GURL(kFooOrigin), eviction_origin());
+}
+
 class SiteEngagementEvictionPolicyTest : public testing::Test {
  public:
   SiteEngagementEvictionPolicyTest()
@@ -49,7 +165,7 @@ class SiteEngagementEvictionPolicyTest : public testing::Test {
   ~SiteEngagementEvictionPolicyTest() override {}
 
   GURL CalculateEvictionOrigin(const std::map<GURL, int64>& usage) {
-    return SiteEngagementEvictionPolicy::CalculateEvictionOrigin(
+    return SiteEngagementEvictionPolicy::CalculateEvictionOriginForTests(
         storage_policy_, score_provider_.get(), usage, kGlobalQuota);
   }