Add UMA for measuring Content-Dispostion header use and abuse.

content/browser/download/download_stats.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_stats.h"
 
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "content/browser/download/download_resource_handler.h"
 #include "content/public/browser/download_interrupt_reasons.h"
+#include "net/http/http_content_disposition.h"
 
 namespace content {
 
+namespace {
+
 // All possible error codes from the network module. Note that the error codes
 // are all positive (since histograms expect positive sample values).
 const int kAllInterruptReasonCodes[] = {
 #define INTERRUPT_REASON(label, value) (value),
 #include "content/public/browser/download_interrupt_reason_values.h"
 #undef INTERRUPT_REASON
 };
 
+// These values are based on net::HttpContentDisposition::ParseResult values.
+// Values other than HEADER_PRESENT and IS_VALID are only measured if |IS_VALID|
+// is true.
+enum ContentDispositionCountTypes {
+  // Count of downloads which had a Content-Disposition headers. The total
+  // number of downloads is measured by UNTHROTTLED_COUNT.
+  CONTENT_DISPOSITION_HEADER_PRESENT = 0,
+
+  // At least one of 'name', 'filename' or 'filenae*' attributes were valid and
+  // yielded a non-empty filename.
+  CONTENT_DISPOSITION_IS_VALID,
+
+  // The following enum values correspond to
+  // net::HttpContentDisposition::ParseResult.
+  CONTENT_DISPOSITION_HAS_DISPOSITION_TYPE,
+  CONTENT_DISPOSITION_HAS_UNKNOWN_TYPE,
+  CONTENT_DISPOSITION_HAS_NAME,
+  CONTENT_DISPOSITION_HAS_FILENAME,
+  CONTENT_DISPOSITION_HAS_EXT_FILENAME,
+  CONTENT_DISPOSITION_HAS_NON_ASCII_STRINGS,
+  CONTENT_DISPOSITION_HAS_PERCENT_ENCODED_STRINGS,
+  CONTENT_DISPOSITION_HAS_RFC2047_ENCODED_STRINGS,
+
+  // Only have the 'name' attribute is present.
+  CONTENT_DISPOSITION_HAS_NAME_ONLY,
+
+  CONTENT_DISPOSITION_LAST_ENTRY
+};
+
+void RecordContentDispositionCount(ContentDispositionCountTypes type,
+                                   bool record) {
+  if (!record)
+    return;
+  UMA_HISTOGRAM_ENUMERATION(
+      "Download.ContentDisposition", type, CONTENT_DISPOSITION_LAST_ENTRY);
+}
+
+} // namespace
+
 void RecordDownloadCount(DownloadCountTypes type) {
   UMA_HISTOGRAM_ENUMERATION(
       "Download.Counts", type, DOWNLOAD_COUNT_TYPES_LAST_ENTRY);
 }
 
 void RecordDownloadSource(DownloadSource source) {
   UMA_HISTOGRAM_ENUMERATION(
       "Download.Sources", source, DOWNLOAD_SOURCE_LAST_ENTRY);
 }
 
@@ skipping 213 matching lines @@
       download_content = DOWNLOAD_CONTENT_VIDEO;
     }
   }
 
   // Record the value.
   UMA_HISTOGRAM_ENUMERATION("Download.ContentType",
                             download_content,
                             DOWNLOAD_CONTENT_MAX);
 }
 
+void RecordDownloadContentDisposition(

[rvargas (doing something else), 2012/12/14 22:49:54]

Isn't it better to generate the histograms directly from net? That would also be
cleaner.

Maybe I'm missing the reason for getting this only for a download.

[asanka, 2012/12/15 01:57:32]

The data gathered here would be used to decide whether or not to deprecate some
non-standard features we support ('name' attribute, non-ascii quoted-strings,
RFC 2047 and percent encoding) and in general to measure the use of the C-D
header.

The question we are asking is: "would removing this feature adversely affect the
handling of downloads?" This is why we are only looking at requests that result
in a download.

[rvargas (doing something else), 2012/12/15 02:46:57]

But that can be answered by generating the same histograms for every C-D that we
see at the net, no?. I was just thinking that doing it in net would eliminate
the whole need of the struct (now enum... I mean translation to and from it),
because all would be done from within a single cc file. That would still seem
better to me, unless there is something that breaks that I'm not seeing.

But I don't want to bike-shed the review :)

[asanka, 2012/12/16 18:32:04]

HttpContentDisposition is instantiated in multiple locations. So the UMA would
have to be controlled from outside the class. I chose to do so after the request
has been recognized as a download. However, since that decision is made outside
of net/, the UMA also moved there.

Given that the C-D header is used primarily to control downloads, generating
histogram data for every C-D seen would give us roughly the same data. But then
if we see an apparently useless header like 'Content-Disposition: filename=foo',
we would have a hard time deciding whether to ignore it (since the
disposition-type is assumed to be 'inline' and the filename is ignored), or log
it (because it might be the result of a 'Save As' or treated as a download due
to the MIME type, in which case the filename is significant).

+    const std::string& content_disposition_string) {
+  if (content_disposition_string.empty())
+    return;
+  net::HttpContentDisposition content_disposition(
+      content_disposition_string, "");
+  const net::HttpContentDisposition::ParseResult& result =
+      content_disposition.parse_result();
+
+  bool is_valid = !result.filename().empty();
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HEADER_PRESENT, true);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_IS_VALID, is_valid);
+  if (!is_valid)
+    return;
+
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_DISPOSITION_TYPE,
+                                result.has_disposition_type);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_UNKNOWN_TYPE,
+                                result.has_unknown_disposition_type);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_NAME,
+                                result.has_name);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_FILENAME,
+                                result.has_filename);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_EXT_FILENAME,
+                                result.has_ext_filename);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_NON_ASCII_STRINGS,
+                                result.has_non_ascii_strings);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_PERCENT_ENCODED_STRINGS,
+                                result.has_percent_encoded_strings);
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_RFC2047_ENCODED_STRINGS,
+                                result.has_rfc2047_encoded_strings);
+
+  RecordContentDispositionCount(CONTENT_DISPOSITION_HAS_NAME_ONLY,
+                                (result.has_name && !result.has_filename &&
+                                 !result.has_ext_filename));
+}
+
 void RecordFileThreadReceiveBuffers(size_t num_buffers) {
     UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Download.FileThreadReceiveBuffers", num_buffers, 1,
       100, 100);
 }
 
 void RecordBandwidth(double actual_bandwidth, double potential_bandwidth) {
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Download.ActualBandwidth", actual_bandwidth, 1, 1000000000, 50);
   UMA_HISTOGRAM_CUSTOM_COUNTS(
@@ skipping 67 matching lines @@
                            disk_write_time_ms * 100 / elapsed_time_ms);
 }
 
 void RecordSavePackageEvent(SavePackageEvent event) {
   UMA_HISTOGRAM_ENUMERATION("Download.SavePackage",
                             event,
                             SAVE_PACKAGE_LAST_ENTRY);
 }
 
 }  // namespace content