[Downloads] Cleanup DownloadResourceHandler

[Downloads] Cleanup DownloadResourceHandler

* Avoid duplicate assignments.
* Don't calculate whether strong validators are present in an HTTP
  response. A spec compliant implementation of this determination is
  already present elsewhere.

BUG=7648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236825

[Randy Smith (Not in Mondays), 2013-11-18 22:07:01]

How is this related to the GetWebContents() issue?

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
content/browser/download/download_resource_handler.cc:184:
headers->GetMimeType(&info->original_mime_type);
nit: Ignoring the return value feels like it's depending on an implementation
quirk of the routine, which is that it sets mime_type to empty if there's no
mime type.

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
File content/browser/download/download_stats.h (right):

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
content/browser/download/download_stats.h:173: bool has_strong_validator);
This change makes me wary because we're changing what statistic we're recording
without changing how we're displaying it.  That would seem to invite broken
comparisons across releases.  I understand wanting to change this, but I think
that involves retiring the old statistic name and creating a new one.

[asanka, 2013-11-19 23:53:47]

Originally this was part of the cleanup done before moving the logic into the
DownloadRequestModel. That's why it was marked as being related to the
WebContents issue. I removed the BUG annotation for that.

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
content/browser/download/download_resource_handler.cc:184:
headers->GetMimeType(&info->original_mime_type);
On 2013/11/18 22:07:02, rdsmith wrote:
> nit: Ignoring the return value feels like it's depending on an implementation
> quirk of the routine, which is that it sets mime_type to empty if there's no
> mime type.  

Done.

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
File content/browser/download/download_stats.h (right):

https://codereview.chromium.org/66993008/diff/1/content/browser/download/down...
content/browser/download/download_stats.h:173: bool has_strong_validator);
On 2013/11/18 22:07:02, rdsmith wrote:
> This change makes me wary because we're changing what statistic we're
recording
> without changing how we're displaying it.  That would seem to invite broken
> comparisons across releases.  I understand wanting to change this, but I think
> that involves retiring the old statistic name and creating a new one.  

I deprecated the old UMA and added a new one.

[Randy Smith (Not in Mondays), 2013-11-20 21:35:56]

LGTM with waffle below.

https://codereview.chromium.org/66993008/diff/150001/content/browser/download...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/66993008/diff/150001/content/browser/download...
content/browser/download/download_resource_handler.cc:174:
headers->EnumerateHeader(NULL, "ETag", &info->etag);
I'm feeling like if I objected to ignoring the return value from GetMimeType() I
should also object to ignoring the return value from EnumerateHeader().  But
here we're implicitly assuming that the value will be left alone if there's no
header of that type (which assumption is actually wrong--it's cleared, unlike in
GetMImeType, where it's left alone.  Which actually looks like a bug in
GetMimeType, because the return value is based on whether it's empty or not. 
Yuck.)  So I sorta feel like it's ok to ignore the return type here, which makes
me wonder if I was being too nit-picky on GetMImeType() :-} :-J.  

So I think I'll request: Do the same thing here as for GetMimeType(), and I'll
be ok with whichever way you got.  (Though I'll call out that if I'm reading the
code correctly, we're relying on info->original_mime_type being null going into
GetMimeType()).

[asanka, 2013-11-21 17:30:21]

https://codereview.chromium.org/66993008/diff/150001/content/browser/download...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/66993008/diff/150001/content/browser/download...
content/browser/download/download_resource_handler.cc:174:
headers->EnumerateHeader(NULL, "ETag", &info->etag);
On 2013/11/20 21:35:56, rdsmith wrote:
> I'm feeling like if I objected to ignoring the return value from GetMimeType()
I
> should also object to ignoring the return value from EnumerateHeader().  But
> here we're implicitly assuming that the value will be left alone if there's no
> header of that type (which assumption is actually wrong--it's cleared, unlike
in
> GetMImeType, where it's left alone.  Which actually looks like a bug in
> GetMimeType, because the return value is based on whether it's empty or not. 
> Yuck.)  So I sorta feel like it's ok to ignore the return type here, which
makes
> me wonder if I was being too nit-picky on GetMImeType() :-} :-J.  
> 
> So I think I'll request: Do the same thing here as for GetMimeType(), and I'll
> be ok with whichever way you got.  (Though I'll call out that if I'm reading
the
> code correctly, we're relying on info->original_mime_type being null going
into
> GetMimeType()).

Done. I chose to check the return value here as well since the behavior when
there's no header isn't documented.

[asanka, 2013-11-21 17:33:18]

rdsmith: Thanks!

asvitkine: Could you look at histograms.xml for owners approval?

[Alexei Svitkine (slow), 2013-11-21 17:39:53]

histograms lgtm with a nit

https://codereview.chromium.org/66993008/diff/200001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/66993008/diff/200001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:21923: +  <int value="15"
label="Supports ranges and strong ETag (Obsolete)"/>
Nit: Put a date - (Obsolete 11/2013)

[asanka, 2013-11-21 18:20:29]

Thanks!

https://codereview.chromium.org/66993008/diff/200001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/66993008/diff/200001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:21923: +  <int value="15"
label="Supports ranges and strong ETag (Obsolete)"/>
On 2013/11/21 17:39:54, Alexei Svitkine wrote:
> Nit: Put a date - (Obsolete 11/2013)

Done. Also updated two enum values above which also became obsolete recently.

[commit-bot: I haz the power, 2013-11-22 18:25:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/66993008/200002

[commit-bot: I haz the power, 2013-11-22 21:11:30]

Change committed as 236825