Add a SafeSearch preference, policy and implementation.

chrome/browser/net/chrome_network_delegate_unittest.cc

Index: chrome/browser/net/chrome_network_delegate_unittest.cc
diff --git a/chrome/browser/net/chrome_network_delegate_unittest.cc b/chrome/browser/net/chrome_network_delegate_unittest.cc
index 3c4aaa156f0122a75115c62f962a72e4fcba9db8..b7ffa82cae38cb29ca631085f67fe8268ea3b03f 100644
--- a/chrome/browser/net/chrome_network_delegate_unittest.cc
+++ b/chrome/browser/net/chrome_network_delegate_unittest.cc
@@ -9,9 +9,20 @@
 #include "base/message_loop.h"
 #include "chrome/browser/api/prefs/pref_member.h"
 #include "chrome/browser/extensions/event_router_forwarder.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/testing_pref_service.h"
+#include "chrome/test/base/testing_profile.h"
+#include "net/base/completion_callback.h"
+#include "net/url_request/url_request.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+const std::string kSingleParameters = chrome::kSafeSearchParameters;
+const std::string kMultipleParameters = "&"+kSingleParameters;
+}
+
 class ChromeNetworkDelegateTest : public testing::Test {
  protected:
   ChromeNetworkDelegateTest()
@@ -32,7 +43,7 @@ class ChromeNetworkDelegateTest : public testing::Test {
   scoped_ptr<ChromeNetworkDelegate> CreateNetworkDelegate() {
     return scoped_ptr<ChromeNetworkDelegate>(new ChromeNetworkDelegate(
         forwarder_.get(), NULL, NULL, NULL, NULL, NULL, &pref_member_, NULL,
-        NULL));
+        NULL, NULL));
   }
 
   // Implementation moved here for access to private bits.
@@ -80,3 +91,181 @@ class ChromeNetworkDelegateTest : public testing::Test {
 TEST_F(ChromeNetworkDelegateTest, NeverThrottleLogic) {
   NeverThrottleLogicImpl();
 }
+
+class ChromeNetworkDelegateSafesearchTest : public testing::Test {
+ public:
+  scoped_ptr<net::NetworkDelegate> CreateNetworkDelegate() {
+    return scoped_ptr<net::NetworkDelegate>(new ChromeNetworkDelegate(
+        forwarder_.get(), NULL, NULL, NULL, NULL, NULL, &enable_referrers_,

[Bernhard Bauer, 2012/10/17 15:23:54]

Do we need |enable_referrers_|?

[Sergiu, 2012/10/19 08:40:55]

The other test has it as well, because on line 272 chrome_network_delegate does:
  if (!enable_referrers_->GetValue())
    request->set_referrer(std::string());
so it can't be NULL. I'm not sure why it doesn't check for enable_referrers
first.

+        NULL, &force_google_safesearch_, NULL));
+  }
+
+  void SetSafesearch(bool value) {
+    force_google_safesearch_.SetValue(value);
+  }
+
+  void SetDelegate(net::NetworkDelegate* delegate) {
+    context_.set_network_delegate(delegate);
+  }
+
+ protected:
+  ChromeNetworkDelegateSafesearchTest()
+      : forwarder_(new extensions::EventRouterForwarder()) {
+  }
+
+  virtual void SetUp() OVERRIDE {
+    prefs_.RegisterBooleanPref(prefs::kForceSafeSearch, false,
+                               PrefService::UNSYNCABLE_PREF);
+    force_google_safesearch_.Init(prefs::kForceSafeSearch,
+                            profile_.GetTestingPrefService(), NULL);

[Bernhard Bauer, 2012/10/17 15:23:54]

Nit: align with parameter on previous line.

[Sergiu, 2012/10/19 08:40:55]

Done.

+    prefs_.RegisterBooleanPref(prefs::kEnableReferrers, false,
+                               PrefService::UNSYNCABLE_PREF);
+    enable_referrers_.Init(prefs::kEnableReferrers,
+                           profile_.GetTestingPrefService(), NULL);
+    loop_.reset(new MessageLoopForIO());
+  }
+
+  // Verifies that the expected string in present only once in the url_string
+  void CheckAddedParameters(const std::string& url_string,
+                            std::string expected,
+                            const bool exact) {
+    // Show the URL in the trace so we know where we failed.
+    SCOPED_TRACE(url_string);
+
+    TestURLRequest request(GURL(url_string), &delegate_, &context_);
+
+    request.Start();
+    MessageLoop::current()->RunAllPending();
+
+    if (exact) {
+      EXPECT_STREQ(expected.c_str(), request.url().query().c_str());

[Bernhard Bauer, 2012/10/17 15:23:54]

You should be able just to do EXPECT_EQ.

[Sergiu, 2012/10/19 08:40:55]

True, didn't modify it since one of them was a string and the other was not, now
fixed.

+    } else {
+      EXPECT_TRUE(request.url().query().rfind(expected));

[Bernhard Bauer, 2012/10/17 15:23:54]

rfind() returns the position, so we check here that the query does not start
with 
|expected|, no?

In general, I'm not really sure why we have this inexact matching here. It would
seem easier for someone looking at the test if they could see exactly what query
string is expected.

[Sergiu, 2012/10/19 08:40:55]

Makes sense, I've refactored the code such that we do a direct check for the
query part, it should be clearer that way.

+      // Make sure that it gets put only once.
+      EXPECT_EQ(request.url().query().find(expected),
+                request.url().query().rfind(expected));
+    }
+  }
+
+  virtual void TearDown() OVERRIDE {

[Bernhard Bauer, 2012/10/17 15:23:54]

This is unnecessary.

[Sergiu, 2012/10/19 08:40:55]

Done.

+  }
+
+ private:
+  scoped_refptr<extensions::EventRouterForwarder> forwarder_;
+  TestingProfile profile_;
+  TestingPrefService prefs_;
+  BooleanPrefMember enable_referrers_;
+  BooleanPrefMember force_google_safesearch_;
+  scoped_ptr<net::URLRequest> request_;
+  TestURLRequestContext context_;
+  TestDelegate delegate_;
+  scoped_ptr<MessageLoopForIO> loop_;
+};
+
+TEST_F(ChromeNetworkDelegateSafesearchTest, SafesearchOn) {

[Bernhard Bauer, 2012/10/17 15:23:54]

Some more test cases:

?q=google&safe=active&ssui=on (we shouldn't modify the request in that case)
?q=google&ssui=on&safe=active (I think we shouldn't modify that request either,
as all the parameters are already set)
?q=google&safe=active&ssui=onemilliondollars
?q=google&unsafe=active&ssui=on

[Sergiu, 2012/10/19 08:40:55]

Done with the new way.

+  // Tests with SafeSearch on, request parameters should be rewritten.
+  SetSafesearch(true);
+  scoped_ptr<net::NetworkDelegate> delegate(CreateNetworkDelegate());
+  SetDelegate(delegate.get());
+
+  // Test the home page.
+  CheckAddedParameters("http://google.com/", kSingleParameters,

[Bernhard Bauer, 2012/10/17 15:23:54]

I think we could directly test the ForceGoogleSafeSearch() method now, no?

[Sergiu, 2012/10/19 08:40:55]

I think it's better to test it this way, at least like this you know you don't
introduce any redirect loops, etc.

+                       true);
+
+  // Test the search home page.
+  CheckAddedParameters("http://google.com/webhp",
+                       kSingleParameters,
+                       true);
+
+  // Test different vaild search pages with parameters.

[Bernhard Bauer, 2012/10/17 15:23:54]

Nit: "valid"

[Sergiu, 2012/10/19 08:40:55]

Done.

+  CheckAddedParameters("http://google.com/search?q=google",
+                       kMultipleParameters,
+                       false);
+
+  CheckAddedParameters("http://google.com/?q=google",
+                       kMultipleParameters,
+                       false);
+
+  CheckAddedParameters("http://google.com/webhp?q=google",
+                       kMultipleParameters,
+                       false);
+
+  // Test the valid pages with safe set to off.
+  CheckAddedParameters("http://google.com/search?q=google&safe=off",
+                       kMultipleParameters,
+                       false);
+
+  CheckAddedParameters("http://google.com/?q=google&safe=off",
+                       kMultipleParameters,
+                       false);
+
+  CheckAddedParameters("http://google.com/webhp?q=google&safe=off",
+                       kMultipleParameters,
+                       false);
+
+  // Test the home page, different TLDs.
+  CheckAddedParameters("http://google.de/", kSingleParameters, true);
+  CheckAddedParameters("http://google.ro/", kSingleParameters, true);
+  CheckAddedParameters("http://google.nl/", kSingleParameters, true);
+
+  // Test the search home page, different TLD.
+  CheckAddedParameters("http://google.de/webhp", kSingleParameters,
+                       true);
+
+  // Test the home page with parameters, different TLD.
+  CheckAddedParameters("http://google.de/search?q=google",
+                       kMultipleParameters,
+                       false);
+
+  // Test the search page with parameters, different TLD.
+  CheckAddedParameters("http://google.de/?q=google",
+                       kMultipleParameters,
+                       false);
+
+  // Test that another website is not affected, without parameters
+  CheckAddedParameters("http://google.com/finance", "", true);
+
+  // Test that another website is not affected, with parameters
+  CheckAddedParameters("http://google.com/finance?q=goog", "q=goog", true);
+
+  // Test that another website is not affected with redirects, with parameters
+  CheckAddedParameters("http://finance.google.com/?q=goog", "q=goog", true);
+}
+
+TEST_F(ChromeNetworkDelegateSafesearchTest, SafesearchOff) {
+  // Tests with SafeSearch settings off, delegate should not alter requests.
+  SetSafesearch(false);
+  scoped_ptr<net::NetworkDelegate> delegate(CreateNetworkDelegate());
+  SetDelegate(delegate.get());
+
+  // Test the home page.
+  CheckAddedParameters("http://google.com/", "", true);
+
+  // Test the search home page.
+  CheckAddedParameters("http://google.com/webhp", "", true);
+
+  // Test the home page with parameters.
+  CheckAddedParameters("http://google.com/search?q=google",
+                       "q=google",
+                       true);
+
+  // Test the search page with parameters.
+  CheckAddedParameters("http://google.com/?q=google",
+                       "q=google",
+                       true);
+
+  // Test the search webhp page with parameters.
+  CheckAddedParameters("http://google.com/webhp?q=google",
+                       "q=google",
+                       true);
+
+  // Test the home page with parameters and safe set to off.
+  CheckAddedParameters("http://google.com/search?q=google&safe=off",
+                       "q=google&safe=off",
+                       true);
+
+  // Test the home page with parameters and safe set to active.
+  CheckAddedParameters("http://google.com/search?q=google&safe=active",
+                       "q=google&safe=active",
+                       true);
+}