SdchManager: remove EnableSdchSupport

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
Committed: https://crrev.com/085ac46e3a37b53f963aa836d6277082fb275c01
Cr-Commit-Position: refs/heads/master@{#343889}

[Elly Fong-Jones, 2015-08-17 15:24:41]

ellyjones@chromium.org changed reviewers:
+ droger@chromium.org, rdsmith@chromium.org

[Elly Fong-Jones, 2015-08-17 15:25:05]

rdsmith: PTAL at everything but the new obj-c tests
droger: PTAL the new obj-c tests (ios/crnet/test/*)

Thanks :)

[Randy Smith (Not in Mondays), 2015-08-17 15:39:43]

This looks basically good to me modulo the issue I raise below, but I'd suggest
also getting a pair of Cronet eyes on it (though you may be Cronet enough
yourself).

https://codereview.chromium.org/1291673003/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/1291673003/diff/1/net/base/sdch_manager.cc#ol...
net/base/sdch_manager.cc:171: return SDCH_DISABLED;
Should we obsolete this error code in the source enum?  (I.e. comment it out,
but reserve the space, since this almost certainly shows up in UMA, and edit
histograms.xml to match?)

[droger, 2015-08-17 15:47:30]

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/crnet_environment.mm
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/crnet_environment...
ios/crnet/crnet_environment.mm:333: LOG(ERROR) << "ConfigureSdch: " <<
sdch_enabled_;
Remove LOG

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
File ios/crnet/test/crnet_http_tests.mm (right):

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:138: [NSString stringWithCString:path.c_str()
encoding:NSUTF8StringEncoding];
Optional convert this to base::SysUTF8ToNSString.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:149: [NSString stringWithCString:path.c_str()
encoding:NSUTF8StringEncoding];
Use base::SysUTF8ToNSString, and same everywhere below.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:186: NSString* haystack = [request.headers
objectForKey:key];
Optional:
request.headers[key];

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:187: if (haystack == nil)
if (!haystack)

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:243: error:&error];
Remove the |error| variable and pass nullptr instead.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:244: DCHECK_NE(static_cast<NSData*>(nil),
received);
Not sure, but I think maybe just
DCHECK(received);
is more consistent with our code in general.

[Elly Fong-Jones, 2015-08-17 17:26:39]

ellyjones@chromium.org changed reviewers:
+ isherman@chromium.org

[Elly Fong-Jones, 2015-08-17 17:26:50]

isherman: PTAL for histogram change?

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/crnet_environment.mm
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/crnet_environment...
ios/crnet/crnet_environment.mm:333: LOG(ERROR) << "ConfigureSdch: " <<
sdch_enabled_;
On 2015/08/17 15:47:30, droger wrote:
> Remove LOG

Done.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
File ios/crnet/test/crnet_http_tests.mm (right):

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:138: [NSString stringWithCString:path.c_str()
encoding:NSUTF8StringEncoding];
On 2015/08/17 15:47:30, droger wrote:
> Optional convert this to base::SysUTF8ToNSString.

Done.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:149: [NSString stringWithCString:path.c_str()
encoding:NSUTF8StringEncoding];
On 2015/08/17 15:47:30, droger wrote:
> Use base::SysUTF8ToNSString, and same everywhere below.

Done.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:186: NSString* haystack = [request.headers
objectForKey:key];
On 2015/08/17 15:47:30, droger wrote:
> Optional:
> request.headers[key];

Done.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:187: if (haystack == nil)
On 2015/08/17 15:47:30, droger wrote:
> if (!haystack)

Done.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:243: error:&error];
On 2015/08/17 15:47:30, droger wrote:
> Remove the |error| variable and pass nullptr instead.

Done.

https://codereview.chromium.org/1291673003/diff/1/ios/crnet/test/crnet_http_t...
ios/crnet/test/crnet_http_tests.mm:244: DCHECK_NE(static_cast<NSData*>(nil),
received);
On 2015/08/17 15:47:30, droger wrote:
> Not sure, but I think maybe just
> DCHECK(received);
> is more consistent with our code in general.

Done.

https://codereview.chromium.org/1291673003/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/1291673003/diff/1/net/base/sdch_manager.cc#ol...
net/base/sdch_manager.cc:171: return SDCH_DISABLED;
On 2015/08/17 15:39:43, rdsmith wrote:
> Should we obsolete this error code in the source enum?  (I.e. comment it out,
> but reserve the space, since this almost certainly shows up in UMA, and edit
> histograms.xml to match?)

Done.

[Randy Smith (Not in Mondays), 2015-08-17 17:34:05]

LGTM; below comment is a suggestions.

https://codereview.chromium.org/1291673003/diff/20001/net/base/sdch_manager_u...
File net/base/sdch_manager_unittest.cc (left):

https://codereview.chromium.org/1291673003/diff/20001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:594: TEST_F(SdchManagerTest, SdchOnByDefault)
{
I'd just nuke this test.

[droger, 2015-08-17 17:35:23]

lgtm

[Elly Fong-Jones, 2015-08-17 18:27:22]

https://codereview.chromium.org/1291673003/diff/20001/net/base/sdch_manager_u...
File net/base/sdch_manager_unittest.cc (left):

https://codereview.chromium.org/1291673003/diff/20001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:594: TEST_F(SdchManagerTest, SdchOnByDefault)
{
On 2015/08/17 17:34:05, rdsmith wrote:
> I'd just nuke this test.

Done.

[Ilya Sherman, 2015-08-17 18:29:29]

histograms.xml lgtm with a comment:

https://codereview.chromium.org/1291673003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1291673003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68977: +  <int value="105"
label="defunct (DISABLED)">
Optional nit: I've seen the formatting of "DISABLED (obsolete)" used more
commonly.  Feel free to use whichever you prefer; just thought I'd mention that
in case you weren't already aware of it.

[Elly Fong-Jones, 2015-08-17 18:30:09]

https://codereview.chromium.org/1291673003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1291673003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68977: +  <int value="105"
label="defunct (DISABLED)">
On 2015/08/17 18:29:29, Ilya Sherman wrote:
> Optional nit: I've seen the formatting of "DISABLED (obsolete)" used more
> commonly.  Feel free to use whichever you prefer; just thought I'd mention
that
> in case you weren't already aware of it.

Acknowledged.

[Elly Fong-Jones, 2015-08-17 18:30:12]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2015-08-17 18:30:13]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1291673003/#ps40001
(title: "Delete obsolete test")

[commit-bot: I haz the power, 2015-08-17 18:30:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291673003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291673003/40001

[commit-bot: I haz the power, 2015-08-17 21:29:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-17 21:29:07]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elly Fong-Jones, 2015-08-18 11:31:07]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2015-08-18 11:31:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291673003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291673003/40001

[commit-bot: I haz the power, 2015-08-18 12:03:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-18 12:03:51]

Patchset 3 (id:??) landed as
https://crrev.com/085ac46e3a37b53f963aa836d6277082fb275c01
Cr-Commit-Position: refs/heads/master@{#343889}