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..9f0158799d4830c83703b68826b4e602713711e6 100644
--- a/chrome/browser/net/chrome_network_delegate_unittest.cc
+++ b/chrome/browser/net/chrome_network_delegate_unittest.cc
@@ -9,6 +9,12 @@
 #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"
 
@@ -32,7 +38,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 +86,192 @@ 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_,
+        NULL, &force_google_safesearch_, NULL));
+  }
+
+  void SetSafesearch(bool value) {

[battre, 2012/10/30 12:56:10]

In the chrome_network_delegate.cc you use the spelling SafeSearch

[Sergiu, 2012/10/30 21:58:17]

Done.

+    force_google_safesearch_.SetValue(value);

[battre, 2012/10/30 12:56:10]

if you use SafeSearch, this should be force_google_safe_search_

[Sergiu, 2012/10/30 21:58:17]

Done.

+  }
+
+  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);
+    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 is equal to the query part (between
+  // ? and #) of the url_string.

[battre, 2012/10/30 12:56:10]

This comment is incorrect. The function does not check that the part between ?
and # of *url_string* is equal to expected.

[Sergiu, 2012/10/30 21:58:17]

Done.

+  void CheckAddedParameters(const std::string& url_string,
+                            const std::string& expected) {

[battre, 2012/10/30 12:56:10]

nit: rename |expected| to |expected_query_parameter|?

[Sergiu, 2012/10/30 21:58:17]

Done.

+    // 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();
+
+    EXPECT_EQ(expected, request.url().query());
+  }
+
+ private:
+  scoped_refptr<extensions::EventRouterForwarder> forwarder_;
+  TestingProfile profile_;
+  TestingPrefService prefs_;
+  BooleanPrefMember enable_referrers_;

[battre, 2012/10/30 12:56:10]

do you use the referrers anywhere?

[Sergiu, 2012/10/30 21:58:17]

They are used in the delegate constructor which does not accept it as NULL (see
[1]). I'm not sure what the logic behind that was exactly though, it was set in
the other unit test as well.

[1]
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_...

[battre, 2012/10/31 13:16:55]

I think we should make enable_referrers in the ChromeNetworkDelegate optional as
well.

[Sergiu, 2012/11/01 18:01:52]

Since this is a long commit already with a lot of changes I'll make that as a
separate commit.

+  BooleanPrefMember force_google_safesearch_;
+  scoped_ptr<net::URLRequest> request_;
+  TestURLRequestContext context_;
+  TestDelegate delegate_;
+  scoped_ptr<MessageLoopForIO> loop_;
+};
+
+TEST_F(ChromeNetworkDelegateSafesearchTest, SafesearchOn) {
+  // Tests with SafeSearch on, request parameters should be rewritten.
+  const std::string kSafeParameter = chrome::kSafeSearchSafeParameter;
+  const std::string kSsuiParameter = chrome::kSafeSearchSsuiParameter;
+  const std::string kBothParameters = kSafeParameter + "&" + kSsuiParameter;
+  SetSafesearch(true);
+  scoped_ptr<net::NetworkDelegate> delegate(CreateNetworkDelegate());
+  SetDelegate(delegate.get());

[battre, 2012/10/30 12:56:10]

how about this?
net::NetworkDelegate delegate;
SetDelegate(&delegate);

[Sergiu, 2012/10/30 21:58:17]

Could you explain a bit more here? Do you mean use a normal object instead of a
scoped_ptr? I still have to call the ChromeNetworkDelegate constructor somehow
don't I?

[battre, 2012/10/31 13:16:55]

Oh, I am sorry, I did not notice this. This solution is good.

+
+  // Test the home page.
+  CheckAddedParameters("http://google.com/", kBothParameters);
+
+  // Test the search home page.
+  CheckAddedParameters("http://google.com/webhp",
+                       kBothParameters);
+
+  // Test different valid search pages with parameters.
+  CheckAddedParameters("http://google.com/search?q=google",
+                       "q=google&" + kBothParameters);
+
+  CheckAddedParameters("http://google.com/?q=google",
+                       "q=google&" + kBothParameters);
+
+  CheckAddedParameters("http://google.com/webhp?q=google",
+                       "q=google&" + kBothParameters);
+
+  // Test the valid pages with safe set to off.
+  CheckAddedParameters("http://google.com/search?q=google&safe=off",
+                       "q=google&" + kBothParameters);
+
+  CheckAddedParameters("http://google.com/?q=google&safe=off",
+                       "q=google&" + kBothParameters);
+
+  CheckAddedParameters("http://google.com/webhp?q=google&safe=off",
+                       "q=google&" + kBothParameters);

[battre, 2012/10/30 12:56:10]

do the same thing with "safe" replaced with "%73afe"

[Sergiu, 2012/10/30 21:58:17]

See comment in the chrome_network_delegate.cc file about this matter. Added a
test with the expected result of adding the two parameters at the end.

+
+  // Test the home page, different TLDs.
+  CheckAddedParameters("http://google.de/", kBothParameters);
+  CheckAddedParameters("http://google.ro/", kBothParameters);
+  CheckAddedParameters("http://google.nl/", kBothParameters);
+
+  // Test the search home page, different TLD.
+  CheckAddedParameters("http://google.de/webhp", kBothParameters);
+
+  // Test the search page with parameters, different TLD.
+  CheckAddedParameters("http://google.de/search?q=google",
+                       "q=google&" + kBothParameters);
+
+  // Test the home page with parameters, different TLD.
+  CheckAddedParameters("http://google.de/?q=google",
+                       "q=google&" + kBothParameters);
+
+  // Test the search page with the parameters set.
+  CheckAddedParameters("http://google.de/?q=google&" + kBothParameters,
+                       "q=google&" + kBothParameters);
+
+  // Test some possibly tricky combinations.
+  CheckAddedParameters("http://google.com/?q=goog&" + kSafeParameter +
+                       "&ssui=one",
+                       "q=goog&" + kBothParameters);
+
+  CheckAddedParameters("http://google.de/?q=goog&unsafe=active&" +
+                       kSsuiParameter,
+                       "q=goog&unsafe=active&" + kBothParameters);
+
+  CheckAddedParameters("http://google.de/?q=goog&safe=off&ssui=off",
+                       "q=goog&" + kBothParameters);
+
+  // Test various combinations where we should not add anything.
+  CheckAddedParameters("http://google.com/?q=goog&" + kSsuiParameter + "&" +
+                       kSafeParameter,
+                       "q=goog&" + kBothParameters);
+
+  CheckAddedParameters("http://google.com/?" + kSsuiParameter + "&q=goog&" +
+                       kSafeParameter,
+                       "q=goog&" + kBothParameters);
+
+  CheckAddedParameters("http://google.com/?" + kSsuiParameter + "&" +
+                       kSafeParameter + "&q=goog",
+                       "q=goog&" + kBothParameters);
+
+  // Test that another website is not affected, without parameters.
+  CheckAddedParameters("http://google.com/finance", "");
+
+  // Test that another website is not affected, with parameters.
+  CheckAddedParameters("http://google.com/finance?q=goog", "q=goog");
+
+  // Test that another website is not affected with redirects, with parameters.
+  CheckAddedParameters("http://finance.google.com/?q=goog", "q=goog");
+
+  // Test with percent-encoded data (%26 is &)
+  CheckAddedParameters("http://google.com/?q=%26%26%26&" + kSsuiParameter +
+                       "&" + kSafeParameter + "&param=%26%26%26",
+                       "q=%26%26%26&param=%26%26%26&" + kBothParameters);
+}
+
+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/", "");
+
+  // Test the search home page.
+  CheckAddedParameters("http://google.com/webhp", "");
+
+  // Test the home page with parameters.
+  CheckAddedParameters("http://google.com/search?q=google",
+                       "q=google");
+
+  // Test the search page with parameters.
+  CheckAddedParameters("http://google.com/?q=google",
+                       "q=google");
+
+  // Test the search webhp page with parameters.
+  CheckAddedParameters("http://google.com/webhp?q=google",
+                       "q=google");
+
+  // Test the home page with parameters and safe set to off.
+  CheckAddedParameters("http://google.com/search?q=google&safe=off",
+                       "q=google&safe=off");
+
+  // Test the home page with parameters and safe set to active.
+  CheckAddedParameters("http://google.com/search?q=google&safe=active",
+                       "q=google&safe=active");
+}