[net] Remove logic for fixing up Gzip encoding type for downloads.

[net] Remove logic for fixing up Gzip encoding type for downloads.

Some servers incorrectly send 'Content-Encoding: gzip' for responses
that should be downloaded as gzipped files. UMA indicates that the
number of servers that exhibit this incorrect behavior is vanishingly
small. Hence the network stack  will no longer perform this encoding
fixup.

What an embedder of the network stack does with a response often depends
on the response headers and the capabilities of the embedder. The
current implementation was based on an inaccurate prediction of the
inteded disposition of the response. In addition to the inaccuracy, the
implementation was also becoming a maintenance burden.

With this change, resources downloaded with the incorrect
Content-Encoding will be saved uncompressed.

BUG=318217
Committed: https://crrev.com/87b5f2ece37da37ef0b0a04da9aee538ef0f9c4b
Cr-Commit-Position: refs/heads/master@{#321890}

[asanka, 2015-03-18 15:17:59]

Patchset #1 (id:1) has been deleted

[asanka, 2015-03-18 15:18:06]

Patchset #1 (id:20001) has been deleted

[asanka, 2015-03-18 15:39:03]

asanka@chromium.org changed reviewers:
+ rdsmith@chromium.org, rsleevi@chromium.org

[asanka, 2015-03-18 15:41:33]

This is based on the Net.GzipEncodingFixupResult histogram. If you feel this
might cause unnecessary grief, we can look into alternatives that would shift
the burden to the embedder.

[Randy Smith (Not in Mondays), 2015-03-18 17:44:14]

Hmmm.  Let me make explicit the argument I think you're making, in pieces, so
that I can respond to it; please correct me if I'm not representing your POV
accurately.

We're currently clearing content encoding in FixupEncodingTypes for single gzip
Content-Encodings for three reasons:
* Mime-type is also gzip.  This is almost certainly a server bug (or else a
double encoding, which would be weird), but a known one (in Apache).  It happens
rarely, about 2 times for every million gzip single encodings.  It does not any
IsSupported() calls.  The user experience here is likely to be a .gz file that's
already been decompressed (because we can't display mime type gzip), which is a
pain, but not horrible.  

* We know that we're downloading and the file name indicates that it's gzip.  
This basically never happens (O(once every billion gzip single encodings)).  The
user experience would again be a file on disk with the .gz suffix that's been
decompressed.

* We avoid decompressing when the file name indicates that its gzip and we don't
know how to handle the mime type (because we expect we'll eventually decide to
download it).  This is also very rare (O(once per ten million gzip single
encodes)).  Same user experience.

Ok, having worked through all that, LGTM :-}.  The only one I'm hesitant about
removing is the first one--it's not needed to avoid the IsSupported* calls and
isn't crazy rare.  Having said that, it is pretty rare, it would be nice to get
rid of the code, and the result isn't a horrible UE.

[Ryan Sleevi, 2015-03-19 02:55:06]

LGTM, with an added caveat of maybe even nuking the comment.

I gave Pat and Daniel over at Moz a heads up via Twitter at
https://twitter.com/sleevi_/status/578388218476933120 , as they've been on a
spree of "removing weird historical hacks" and it'd be nice for them to do the
same.

https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc#ne...
net/filter/filter.cc:215: // downloads.
I'm inclined to suggest nuking this whole bit.

I don't think there's ever been a time where I've looked through the code and
wanted to know what the "once upon a time" was; for that, there's source control
history.

[Randy Smith (Not in Mondays), 2015-03-19 15:01:06]

https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc#ne...
net/filter/filter.cc:215: // downloads.
On 2015/03/19 02:55:06, Ryan Sleevi wrote:
> I'm inclined to suggest nuking this whole bit.
> 
> I don't think there's ever been a time where I've looked through the code and
> wanted to know what the "once upon a time" was; for that, there's source
control
> history.

+1 :-}.

[Randy Smith (Not in Mondays), 2015-03-19 15:02:22]

On 2015/03/19 15:01:06, rdsmith wrote:
> https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc
> File net/filter/filter.cc (right):
> 
>
https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc#ne...
> net/filter/filter.cc:215: // downloads.
> On 2015/03/19 02:55:06, Ryan Sleevi wrote:
> > I'm inclined to suggest nuking this whole bit.
> > 
> > I don't think there's ever been a time where I've looked through the code
and
> > wanted to know what the "once upon a time" was; for that, there's source
> control
> > history.
> 
> +1 :-}.

Actually, let me call out that I spaced in reviewing this and only looked at
filter.cc.  Happy to do a real review if you'd like; if I don't hear from you
I'll assume Ryan did it and that's good enough.

[asanka, 2015-03-19 20:27:52]

Thanks for the reviews. I'll wait until you are comfortable with the review
coverage. The one thing I'd look at is whether we need to salvage any of the
tests that are being removed. I added a basic gzip test since there was one
(non-logging) code path that wasn't being exercised in the unittests.

https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/1008873006/diff/40001/net/filter/filter.cc#ne...
net/filter/filter.cc:215: // downloads.
On 2015/03/19 15:01:06, rdsmith wrote:
> On 2015/03/19 02:55:06, Ryan Sleevi wrote:
> > I'm inclined to suggest nuking this whole bit.
> > 
> > I don't think there's ever been a time where I've looked through the code
and
> > wanted to know what the "once upon a time" was; for that, there's source
> control
> > history.
> 
> +1 :-}.

Cool. I removed the comment.

I thought I'd add the tombstone to document why we aren't doing this since it's
sometimes difficult to determine whether the absence of code is intentional or
not. It wasn't meant to document revision history.

Relatedly, I recently discovered 'git log -L'. E.g.

  git log -L /FixupEncodingTypes\(/,/^}/:net/filter/filter.cc

will give a nice trace through history of how this function evolved. Pretty neat
and useful for this sort of code archaeology.

[Randy Smith (Not in Mondays), 2015-03-23 15:37:22]

Wow, that was bracing; lots of ugly code removed.  Review complete, LGTM++.

[asanka, 2015-03-23 15:43:56]

On 2015/03/23 15:37:22, rdsmith wrote:
> Wow, that was bracing; lots of ugly code removed.  Review complete, LGTM++.

Thanks!

[asanka, 2015-03-23 15:44:01]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-03-23 15:44:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1008873006/#ps80001
(title: "Add a basic test")

[commit-bot: I haz the power, 2015-03-23 15:44:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008873006/80001

[commit-bot: I haz the power, 2015-03-23 15:51:27]

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

[commit-bot: I haz the power, 2015-03-23 15:51:28]

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

[asanka, 2015-03-23 15:54:28]

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

[asanka, 2015-03-23 15:55:08]

Whoops. Needs review for the histogram changes.

+isherman : histograms pls.

[Ilya Sherman, 2015-03-23 22:11:27]

histograms.xml LGTM

[Ryan Sleevi, 2015-03-23 22:15:09]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-03-23 22:16:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008873006/80001

[commit-bot: I haz the power, 2015-03-23 23:14:13]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-23 23:14:59]

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