Restructure SDCH layering to allow more separation (observer/1->[0,n]

net/url_request/sdch_dictionary_fetcher_unittest.cc

Index: net/url_request/sdch_dictionary_fetcher_unittest.cc
diff --git a/net/base/sdch_dictionary_fetcher_unittest.cc b/net/url_request/sdch_dictionary_fetcher_unittest.cc
similarity index 77%
rename from net/base/sdch_dictionary_fetcher_unittest.cc
rename to net/url_request/sdch_dictionary_fetcher_unittest.cc
index a8b814e77505e6e03a0aa9418013649c4faac43b..9f47427e03fbfde6ca73657c6d46a31f1bb3b15a 100644
--- a/net/base/sdch_dictionary_fetcher_unittest.cc
+++ b/net/url_request/sdch_dictionary_fetcher_unittest.cc
@@ -2,10 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/base/sdch_dictionary_fetcher.h"
+#include "net/url_request/sdch_dictionary_fetcher.h"
 
 #include <string>
 
+#include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
@@ -31,8 +32,8 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
   static void AddUrlHandler() {
     net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
     jobs_requested_ = 0;
-    filter->AddHostnameHandler("http", kTestDomain,
-                               &URLRequestSpecifiedResponseJob::Factory);
+    filter->AddHostnameHandler(
+        "http", kTestDomain, &URLRequestSpecifiedResponseJob::Factory);
   }
 
   static void RemoveUrlHandler() {
@@ -41,17 +42,17 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
     jobs_requested_ = 0;
   }
 
-  static URLRequestJob* Factory(
-      URLRequest* request,
-      net::NetworkDelegate* network_delegate,
-      const std::string& scheme) {
+  static URLRequestJob* Factory(URLRequest* request,
+                                net::NetworkDelegate* network_delegate,
+                                const std::string& scheme) {
     ++jobs_requested_;
     return new URLRequestSpecifiedResponseJob(request, network_delegate);
   }
 
   static std::string ExpectedResponseForURL(const GURL& url) {
     return base::StringPrintf("Response for %s\n%s\nEnd Response for %s\n",
-                              url.spec().c_str(), kSampleBufferContext,
+                              url.spec().c_str(),
+                              kSampleBufferContext,
                               url.spec().c_str());
   }
 
@@ -73,57 +74,48 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
 
 int URLRequestSpecifiedResponseJob::jobs_requested_(0);
 
-class SdchTestDelegate : public SdchFetcher::Delegate {
+class SdchDictionaryFetcherTest : public ::testing::Test {
  public:
   struct DictionaryAdditions {
     DictionaryAdditions(const std::string& dictionary_text,
                         const GURL& dictionary_url)
-        : dictionary_text(dictionary_text),
-          dictionary_url(dictionary_url) {}
-
+        : dictionary_text(dictionary_text), dictionary_url(dictionary_url) {}
 
     std::string dictionary_text;
     GURL dictionary_url;
   };
 
-  void AddSdchDictionary(const std::string& dictionary_text,
-                         const GURL& dictionary_url) override {
-    dictionary_additions.push_back(
-        DictionaryAdditions(dictionary_text, dictionary_url));
-  }
-
-  void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) {
-    out->swap(dictionary_additions);
-    dictionary_additions.clear();
-  }
-
- private:
-  std::vector<DictionaryAdditions> dictionary_additions;
-};
-
-class SdchDictionaryFetcherTest : public ::testing::Test {
- public:
   SdchDictionaryFetcherTest() {}
 
   void SetUp() override {
     DCHECK(!fetcher_.get());

[Ryan Sleevi, 2014/11/05 22:21:39]

DCHECK in tests is BAD! :)

ASSERT_FALSE(fetcher_.get()) if you go the SetUp route.

[Randy Smith (Not in Mondays), 2014/11/05 23:50:00]

Switching to the ctor makes this moot.

 
     URLRequestSpecifiedResponseJob::AddUrlHandler();
-    fetcher_delegate_.reset(new SdchTestDelegate);
     context_.reset(new TestURLRequestContext);
     fetcher_.reset(new SdchDictionaryFetcher(
-        fetcher_delegate_.get(), context_.get()));
+        context_.get(),
+        base::Bind(&SdchDictionaryFetcherTest::OnDictionaryFetched,
+                   base::Unretained(this))));

[Ryan Sleevi, 2014/11/05 22:21:39]

Is there a reason you need to do this in SetUp/TearDown, rather than CTor/DTor?

You don't appear to be using any of the ASSERTs that would be used during this.

[Randy Smith (Not in Mondays), 2014/11/05 23:50:00]

I had thought that a single instance of the test object was used for all tests,
so I wanted to reset between tests.  But it works in the ctor/dtor, so I'm good
with doing it that way.  Done.

[Ryan Sleevi, 2014/11/06 00:03:45]

Common mistake See
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...
for details :)

   }
 
   void TearDown() override {
     URLRequestSpecifiedResponseJob::RemoveUrlHandler();
     fetcher_.reset();
     context_.reset();
-    fetcher_delegate_.reset();
+  }
+
+  void OnDictionaryFetched(const std::string& dictionary_text,
+                           const GURL& dictionary_url) {
+    dictionary_additions.push_back(
+        DictionaryAdditions(dictionary_text, dictionary_url));
+  }
+
+  void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) {
+    out->swap(dictionary_additions);
+    dictionary_additions.clear();
   }
 
   SdchDictionaryFetcher* fetcher() { return fetcher_.get(); }
-  SdchTestDelegate* manager() { return fetcher_delegate_.get(); }
 
   // May not be called outside the SetUp()/TearDown() interval.
   int JobsRequested() {
@@ -139,9 +131,9 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
   }
 
  private:
-  scoped_ptr<SdchTestDelegate> fetcher_delegate_;
   scoped_ptr<TestURLRequestContext> context_;
   scoped_ptr<SdchDictionaryFetcher> fetcher_;
+  std::vector<DictionaryAdditions> dictionary_additions;
 };
 
 // Schedule a fetch and make sure it happens.
@@ -151,11 +143,12 @@ TEST_F(SdchDictionaryFetcherTest, Basic) {
 
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, JobsRequested());
-  std::vector<SdchTestDelegate::DictionaryAdditions> additions;
-  manager()->GetDictionaryAdditions(&additions);
+  std::vector<DictionaryAdditions> additions;
+  GetDictionaryAdditions(&additions);
   ASSERT_EQ(1u, additions.size());
-  EXPECT_EQ(URLRequestSpecifiedResponseJob::ExpectedResponseForURL(
-      dictionary_url), additions[0].dictionary_text);
+  EXPECT_EQ(
+      URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
+      additions[0].dictionary_text);
 }
 
 // Multiple fetches of the same URL should result in only one request.
@@ -167,11 +160,12 @@ TEST_F(SdchDictionaryFetcherTest, Multiple) {
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1, JobsRequested());
-  std::vector<SdchTestDelegate::DictionaryAdditions> additions;
-  manager()->GetDictionaryAdditions(&additions);
+  std::vector<DictionaryAdditions> additions;
+  GetDictionaryAdditions(&additions);
   ASSERT_EQ(1u, additions.size());
-  EXPECT_EQ(URLRequestSpecifiedResponseJob::ExpectedResponseForURL(
-      dictionary_url), additions[0].dictionary_text);
+  EXPECT_EQ(
+      URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
+      additions[0].dictionary_text);
 }
 
 // A cancel should result in no actual requests being generated.
@@ -189,5 +183,4 @@ TEST_F(SdchDictionaryFetcherTest, Cancel) {
   // Synchronous execution may have resulted in a single job being scheduled.
   EXPECT_GE(1, JobsRequested());
 }
-
 }