SdchManager: remove EnableSdchSupport

This change removes support for globally enabling or disabling SDCH. SDCH can
still be enabled or disabled at the URLRequestContext level by creating or
nulling an individual URLRequestContext's SdchManager. The only non-test user of
this method was CrNet, which now only creates an SdchManager if SDCH is to be
enabled.

This change adds a new CrNet integration test to validate that the default
behavior is correct.

BUG=387891

Review URL: https://codereview.chromium.org/1291673003

Cr-Commit-Position: refs/heads/master@{#343889}

ios/crnet/crnet_environment.mm

@@ -328,15 +328,15 @@ bool IsRequestSupported(NSURLRequest* request) override {
 
 void CrNetEnvironment::ConfigureSdchOnNetworkThread() {
   DCHECK(base::MessageLoop::current() == network_io_thread_->message_loop());
+  net::URLRequestContext* context =
+      main_context_getter_->GetURLRequestContext();
 
   if (!sdch_enabled_) {
-    net::SdchManager::EnableSdchSupport(false);
+    DCHECK_EQ(static_cast<net::SdchManager*>(nullptr), context->sdch_manager());
     return;
   }
 
   sdch_manager_.reset(new net::SdchManager());
-  net::URLRequestContext* context =
-      main_context_getter_->GetURLRequestContext();
   sdch_owner_.reset(new net::SdchOwner(sdch_manager_.get(), context));
   if (!sdch_pref_store_filename_.empty()) {
     base::FilePath path(sdch_pref_store_filename_);

ios/crnet/test/crnet_http_tests.mm

@@ -6,7 +6,9 @@
 
 #import "CrNet.h"
 
+#include "base/logging.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/strings/sys_string_conversions.h"
 #import "ios/third_party/gcdwebserver/src/GCDWebServer/Core/GCDWebServer.h"
 #include "net/base/mac/url_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -134,15 +136,23 @@ void StartWebServer() {
   // Registers a fixed response |text| to be returned to requests for |path|,
   // which is relative to |server_root_|.
   void RegisterPathText(const std::string& path, const std::string& text) {
-    NSString* nspath =
-        [NSString stringWithCString:path.c_str() encoding:NSUTF8StringEncoding];
+    NSString* nspath = base::SysUTF8ToNSString(path);
     NSData* data = [NSData dataWithBytes:text.c_str() length:text.length()];
     [web_server_ addGETHandlerForPath:nspath
                           staticData:data
                          contentType:@"text/plain"
                             cacheAge:30];
   }
 
+  void RegisterPathHandler(const std::string& path,
+                           GCDWebServerProcessBlock handler) {
+    NSString* nspath = base::SysUTF8ToNSString(path);
+    [web_server_ addHandlerForMethod:@"GET"
+                                path:nspath
+                        requestClass:NSClassFromString(@"GCDWebServerRequest")
+                        processBlock:handler];
+  }
+
   // Launches the supplied |task| and blocks until it completes, with a timeout
   // of 1 second.
   void StartDataTaskAndWaitForCompletion(NSURLSessionDataTask* task) {
@@ -159,6 +169,22 @@ GURL GetURL(const std::string& path) {
     return server_root_.Resolve(real_path);
   }
 
+  // Some convenience functions for working with GCDWebServerRequest and
+  // GCDWebServerResponse.
+
+  // Returns true if the value for the request header |header| is not nil and
+  // contains the string |target|.
+  bool HeaderValueContains(GCDWebServerRequest* request,
+                           const std::string& header,
+                           const std::string& target) {
+    NSString* key = base::SysUTF8ToNSString(header);
+    NSString* needle = base::SysUTF8ToNSString(target);
+    NSString* haystack = request.headers[key];
+    if (!haystack)
+      return false;
+    return [haystack rangeOfString:needle].location != NSNotFound;
+  }
+
   base::scoped_nsobject<NSURLSession> session_;
   base::scoped_nsobject<TestDelegate> delegate_;
 
@@ -176,21 +202,45 @@ GURL GetURL(const std::string& path) {
   NSURL* url = net::NSURLWithGURL(GetURL(kPath));
   NSURLRequest* req = [NSURLRequest requestWithURL:url];
   NSURLResponse* resp = nil;
-  NSError* error = nil;
   NSData* received = [NSURLConnection sendSynchronousRequest:req
                                            returningResponse:&resp
-                                                       error:&error];
+                                                       error:nullptr];
   EXPECT_EQ(0, memcmp([received bytes], kData, sizeof(kData)));
 }
 
 TEST_F(HttpTest, NSURLSessionReceivesData) {
-  const char data[] = "foobar";
-  RegisterPathText("/foo", data);
+  const char kPath[] = "/foo";
+  const char kData[] = "foobar";
+  RegisterPathText(kPath, kData);
   StartWebServer();
 
-  NSURL* url = net::NSURLWithGURL(GetURL("foo"));
+  NSURL* url = net::NSURLWithGURL(GetURL(kPath));
   NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
   StartDataTaskAndWaitForCompletion(task);
   EXPECT_EQ(nil, [delegate_ error]);
-  EXPECT_EQ(strlen(data), [delegate_ receivedBytes]);
+  EXPECT_EQ(strlen(kData), [delegate_ receivedBytes]);
+}
+
+TEST_F(HttpTest, SdchDisabledByDefault) {
+  const char kPath[] = "/foo";
+  RegisterPathHandler(kPath,
+      ^GCDWebServerResponse* (GCDWebServerRequest* req) {
+        EXPECT_FALSE(HeaderValueContains(req, "Accept-Encoding", "sdch"));
+        return nil;
+      });
+  StartWebServer();
+  NSURL* url = net::NSURLWithGURL(GetURL(kPath));
+  NSURLRequest* req = [NSURLRequest requestWithURL:url];
+  NSURLResponse* resp = nil;
+  NSError* error = nil;
+  NSData* received = [NSURLConnection sendSynchronousRequest:req
+                                           returningResponse:&resp
+                                                       error:&error];
+  DCHECK(received);
 }
+
+// TODO(ellyjones): There needs to be a test that enabling SDCH works, but
+// because CrNet is static and 'uninstall' only disables it, there is no way to
+// have an individual test enable or disable SDCH.
+// Probably there is a way to get gtest tests to run in a separate process, but
+// I'm not sure what it is.

net/base/sdch_manager.cc

@@ -38,9 +38,6 @@ void StripTrailingDot(GURL* gurl) {
 
 namespace net {
 
-// static
-bool SdchManager::g_sdch_enabled_ = true;
-
 SdchManager::DictionarySet::DictionarySet() {}
 
 SdchManager::DictionarySet::~DictionarySet() {}
@@ -103,11 +100,6 @@ void SdchManager::SdchErrorRecovery(SdchProblemCode problem) {
                             SDCH_MAX_PROBLEM_CODE);
 }
 
-// static
-void SdchManager::EnableSdchSupport(bool enabled) {
-  g_sdch_enabled_ = enabled;
-}
-
 void SdchManager::BlacklistDomain(const GURL& url,
                                   SdchProblemCode blacklist_reason) {
   SetAllowLatencyExperiment(url, false);
@@ -167,9 +159,6 @@ int SdchManager::BlacklistDomainExponential(const std::string& domain) {
 
 SdchProblemCode SdchManager::IsInSupportedDomain(const GURL& url) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!g_sdch_enabled_ )
-    return SDCH_DISABLED;
-
   if (blacklisted_domains_.empty())
     return SDCH_OK;
 
@@ -456,7 +445,7 @@ void SdchManager::UrlSafeBase64Encode(const std::string& input,
 scoped_ptr<base::Value> SdchManager::SdchInfoToValue() const {
   scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue());
 
-  value->SetBoolean("sdch_enabled", sdch_enabled());
+  value->SetBoolean("sdch_enabled", true);
 
   scoped_ptr<base::ListValue> entry_list(new base::ListValue());
   for (const auto& entry: dictionaries_) {

net/base/sdch_manager.h

@@ -98,11 +98,6 @@ class NET_EXPORT SdchManager {
   // Record stats on various errors.
   static void SdchErrorRecovery(SdchProblemCode problem);
 
-  // Enables or disables SDCH compression.
-  static void EnableSdchSupport(bool enabled);
-
-  static bool sdch_enabled() { return g_sdch_enabled_; }
-
   // Briefly prevent further advertising of SDCH on this domain (if SDCH is
   // enabled). After enough calls to IsInSupportedDomain() the blacklisting
   // will be removed. Additional blacklists take exponentially more calls

net/base/sdch_manager_unittest.cc

@@ -100,18 +100,12 @@ class MockSdchObserver : public SdchObserver {
 class SdchManagerTest : public testing::Test {
  protected:
   SdchManagerTest()
-      : sdch_manager_(new SdchManager),
-        default_support_(sdch_manager_->sdch_enabled()) { }
+      : sdch_manager_(new SdchManager) {}
 
   ~SdchManagerTest() override {}
 
   SdchManager* sdch_manager() { return sdch_manager_.get(); }
 
-  // Reset globals back to default state.
-  void TearDown() override {
-    SdchManager::EnableSdchSupport(default_support_);
-  }
-
   // Attempt to add a dictionary to the manager and probe for success or
   // failure.
   bool AddSdchDictionary(const std::string& dictionary_text,
@@ -122,7 +116,6 @@ class SdchManagerTest : public testing::Test {
 
  private:
   scoped_ptr<SdchManager> sdch_manager_;
-  bool default_support_;
 };
 
 static std::string NewSdchDictionary(const std::string& domain) {
@@ -140,9 +133,6 @@ static std::string NewSdchDictionary(const std::string& domain) {
 TEST_F(SdchManagerTest, DomainSupported) {
   GURL google_url("http://www.google.com");
 
-  SdchManager::EnableSdchSupport(false);
-  EXPECT_EQ(SDCH_DISABLED, sdch_manager()->IsInSupportedDomain(google_url));
-  SdchManager::EnableSdchSupport(true);
   EXPECT_EQ(SDCH_OK, sdch_manager()->IsInSupportedDomain(google_url));
 }
 
@@ -591,15 +581,6 @@ TEST_F(SdchManagerTest, ExpirationCheckedProperly) {
   EXPECT_EQ(SDCH_OK, problem_code);
 }
 
-TEST_F(SdchManagerTest, SdchOnByDefault) {
-  GURL google_url("http://www.google.com");
-  scoped_ptr<SdchManager> sdch_manager(new SdchManager);
-
-  EXPECT_EQ(SDCH_OK, sdch_manager->IsInSupportedDomain(google_url));
-  SdchManager::EnableSdchSupport(false);
-  EXPECT_EQ(SDCH_DISABLED, sdch_manager->IsInSupportedDomain(google_url));
-}
-
 // Confirm dispatch of notification.
 TEST_F(SdchManagerTest, SdchDictionaryUsed) {
   MockSdchObserver observer;

net/base/sdch_problem_code_list.h

@@ -123,9 +123,10 @@ SDCH_PROBLEM_CODE(DECODE_ERROR, 96)
 SDCH_PROBLEM_CODE(LATENCY_TEST_DISALLOWED, 100)
 
 // General SDCH problems.
-// SDCH is disabled.
-SDCH_PROBLEM_CODE(DISABLED, 105)
-// SDCH over https is disabled.
+// SDCH is enabled or disabled per URLRequestContext now so this value is never
+// used.
+// SDCH_PROBLEM_CODE(DISABLED, 105)
+
 // SDCH always supports secure schemes now, so this enum value is unused.
 // SDCH_PROBLEM_CODE(SECURE_SCHEME_NOT_SUPPORTED, 106)
 

net/filter/filter.cc

@@ -379,8 +379,7 @@ Filter* Filter::PrependNewFilter(FilterType type_id,
       break;
     case FILTER_TYPE_SDCH:
     case FILTER_TYPE_SDCH_POSSIBLE:
-      if (filter_context.GetURLRequestContext()->sdch_manager() &&
-          SdchManager::sdch_enabled()) {
+      if (filter_context.GetURLRequestContext()->sdch_manager()) {
         first_filter.reset(
             InitSdchFilter(type_id, filter_context, buffer_size));
       }

net/url_request/url_request_http_job.cc

@@ -344,13 +344,10 @@ void URLRequestHttpJob::NotifyHeadersComplete() {
   if (sdch_manager) {
     SdchProblemCode rv = sdch_manager->IsInSupportedDomain(request()->url());
     if (rv != SDCH_OK) {
-      // If SDCH is just disabled, it is not a real error.
-      if (rv != SDCH_DISABLED) {
-        SdchManager::SdchErrorRecovery(rv);
-        request()->net_log().AddEvent(
-            NetLog::TYPE_SDCH_DECODING_ERROR,
-            base::Bind(&NetLogSdchResourceProblemCallback, rv));
-      }
+      SdchManager::SdchErrorRecovery(rv);
+      request()->net_log().AddEvent(
+          NetLog::TYPE_SDCH_DECODING_ERROR,
+          base::Bind(&NetLogSdchResourceProblemCallback, rv));
     } else {
       const std::string name = "Get-Dictionary";
       std::string url_text;
@@ -559,13 +556,10 @@ void URLRequestHttpJob::AddExtraHeaders() {
       SdchProblemCode rv = sdch_manager->IsInSupportedDomain(request()->url());
       if (rv != SDCH_OK) {
         advertise_sdch = false;
-        // If SDCH is just disabled, it is not a real error.
-        if (rv != SDCH_DISABLED) {
-          SdchManager::SdchErrorRecovery(rv);
-          request()->net_log().AddEvent(
-              NetLog::TYPE_SDCH_DECODING_ERROR,
-              base::Bind(&NetLogSdchResourceProblemCallback, rv));
-        }
+        SdchManager::SdchErrorRecovery(rv);
+        request()->net_log().AddEvent(
+            NetLog::TYPE_SDCH_DECODING_ERROR,
+            base::Bind(&NetLogSdchResourceProblemCallback, rv));
       }
     }
     if (advertise_sdch) {

tools/metrics/histograms/histograms.xml

@@ -69108,8 +69108,13 @@ To add a new entry, add it with any value and run test to compute valid value.
   <int value="95" label="PRIOR_TO_DICTIONARY"/>
   <int value="96" label="DECODE_ERROR"/>
   <int value="100" label="LATENCY_TEST_DISALLOWED"/>
-  <int value="105" label="DISABLED"/>
-  <int value="106" label="SECURE_SCHEME_NOT_SUPPORTED"/>
+  <int value="105" label="defunct (DISABLED)">
+    No longer centrally tracked, since SDCH is enabled or disabled per
+    URLRequestContext.
+  </int>
+  <int value="106" label="defunct (SECURE_SCHEME_NOT_SUPPORTED)">
+    SDCH support is now always enabled for secure schemes.
+  </int>
   <int value="107" label="DICTIONARY_USED_AFTER_DELETION"/>
 </enum>