Recording download mime types for normal profile

content/browser/download/download_item_impl.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.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 1147 matching lines @@
   OnAllDataSaved(total_bytes, std::move(secure_hash));
   MaybeCompleteDownload();
 }
 
 // **** Download progression cascade
 
 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.
     file_name = target_path_.AsUTF8Unsafe();
   } else {
     // See if it's set programmatically.
     file_name = forced_file_path_.AsUTF8Unsafe();
     // Possibly has a 'download' attribute for the anchor.
     if (file_name.empty())
       file_name = suggested_filename_;
@@ skipping 77 matching lines @@
 
     // Otherwise, this was a resumption attempt which ended with an
     // interruption. Continue with current target path.
     TransitionTo(TARGET_RESOLVED_INTERNAL);
     InterruptWithPartialState(
         offset, std::move(hash_state), new_create_info.result);
     UpdateObservers();
     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_);
 
   if (state_ == RESUMING_INTERNAL)
     UpdateValidatorsOnResumption(new_create_info);
 
   if (state_ == INITIAL_INTERNAL && job_->UsesParallelRequests())
     RecordDownloadCount(USES_PARALLEL_REQUESTS);
 
@@ skipping 278 matching lines @@
 
 void DownloadItemImpl::Completed() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   DVLOG(20) << __func__ << "() " << DebugString(false);
 
   DCHECK(all_data_saved_);
   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.
   } else if (GetOpenWhenComplete() ||
              ShouldOpenFileBasedOnExtension() ||
              IsTemporary()) {
     // If the download is temporary, like in drag-and-drop, do not open it but
     // we still need to set it auto-opened so that it can be removed from the
     // download shelf.
     if (!IsTemporary())
@@ skipping 599 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content