Recording download mime types for normal profile

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 507ace3f58836cc192837a19cf459b4915197981..4599601ead4c4ce2f57e7f488d41bf21a2b673ac 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -1165,9 +1165,6 @@ void DownloadItemImpl::Init(bool active,
                             DownloadType download_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (active)
-    RecordDownloadCount(START_COUNT);
-
   std::string file_name;
   if (download_type == SRC_HISTORY_IMPORT) {
     // target_path_ works for History and Save As versions.
@@ -1265,6 +1262,15 @@ void DownloadItemImpl::Start(
     return;
   }
 
+  if (state_ == INITIAL_INTERNAL) {
+    RecordDownloadCount(START_COUNT);

[asanka, 2017/03/23 14:34:43]

START_COUNT is probably better if recorded for all regular download requests
(i.e. don't filter START_COUNT based on INITIAL_INTERNAL. Instead just record it
each time we hit Start()). I think it's OK to move the metric here so that we
don't count save package requests assuming that the noise introduced by save
package requests was small.

However, if we limit it to new downloads only, then we are removing all the
resumption attempts from START_COUNT (roughly 2% overall, and 5% change on
mobile). This is a larger change that would make it challenging to interpret UMA
without carefully filtering out the old and new data. This is likely to confuse
lots of folks who look at these stats.

One possibility is to add a new count, NEW_DOWNLOAD_COUNT or somesuch that's
recorded here. That way it'll be easier to measure our success rate (i.e.
COMPLETED / NEW_DOWNLOAD_COUNT or (COMPLETED + CANCELLED) / NEW_DOWNLOAD_COUNT
depending on who you ask :-)). CUrrently we have to do some arithmetic to get at
success rates.

[shaktisahu, 2017/03/23 15:33:28]

Oh, I misunderstood the comments of Init() that |active| is true for new
downloads only, but it is not :-)
So START_COUNT includes the resumptions already... 
I will move this line (and the  START_COUNT_NORMAL_PROFILE metrics) before the
INITIAL_INTERNAL check.
Not sure if it is required to add the NEW_DOWNLOAD_COUNT metrics, but sounds
like a good idea if there is no such metrics today.
How do we calculate the number of new downloads  (or number of resumption
attempts that you mentioned 2%) today?

[asanka, 2017/03/23 16:23:45]

Count of all resumptions = Count of (Download.Source["initiated by manual
resumption"]) + Count of (Download.Source["Initiated by automatic resumption"])

Count of new downloads = Count of (Download.Counts["STARTED"]) - count of all
resumptions.

[shaktisahu, 2017/03/23 23:05:34]

Done.

+    RecordDownloadMimeType(mime_type_);

[asanka, 2017/03/23 14:34:43]

Ditto here, but in this case it's probably easier to just change the name of the
metric so that old and new data doesn't get mixed. I agree that recording the
MIME type here is the right way to go. HTTP 206 responses aren't guaranteed to
have the same MIME type as the original response and we don't MIME sniff
resumption responses.

[shaktisahu, 2017/03/23 15:33:28]

Are you saying it is okay to remove the old metric name (Download.ContentType)
altogether and replace it by the new name?

[asanka, 2017/03/23 16:23:45]

We need to keep the old one in histograms.xml since older builds of Chrome will
continue to report the old metric. It just needs to be annotated as <obsolete>
in the xml file.

In C++ land, we can change the metrics recording logic to record to a new
histogram name.

[shaktisahu, 2017/03/23 23:05:34]

Done.

+    if (!GetBrowserContext()->IsOffTheRecord()) {
+      RecordDownloadCount(START_COUNT_NORMAL_PROFILE);
+      RecordDownloadMimeTypeForNormalProfile(mime_type_);
+    }
+  }
+
   // Successful download start.
   DCHECK(download_file_);
   DCHECK(job_);
@@ -1563,6 +1569,9 @@ void DownloadItemImpl::Completed() {
   end_time_ = base::Time::Now();
   TransitionTo(COMPLETE_INTERNAL);
   RecordDownloadCompleted(start_tick_, received_bytes_);
+  if (!GetBrowserContext()->IsOffTheRecord()) {
+    RecordDownloadCount(COMPLETED_COUNT_NORMAL_PROFILE);
+  }
 
   if (auto_opened_) {
     // If it was already handled by the delegate, do nothing.