Add UMA metrics for the frequency of closing dangerous download notifications.

chrome/browser/download/notification/download_item_notification.cc

 // Copyright 2015 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 "chrome/browser/download/notification/download_item_notification.h"
 
 #include "base/files/file_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_crx_util.h"
@@ skipping 136 matching lines @@
       content::RecordAction(
           UserMetricsAction("DownloadNotification.Button_Pause"));
       break;
     case DownloadCommands::RESUME:
       content::RecordAction(
           UserMetricsAction("DownloadNotification.Button_Resume"));
       break;
   }
 }
 
+struct DownloadItemNotificationCommandEntry {

[asanka, 2015/09/17 14:29:40]

This is no longer necessary since you are only using the |id| field.

[Deepak, 2015/09/18 04:38:01]

Done.

+  DownloadCommands::Command command;
+  int id;
+};
+
+DownloadItemNotificationCommandEntry kDownloadNotificationCommand[] = {

[asanka, 2015/09/17 14:29:40]

Pick a name that implies that this is a mapping from DownloadCommands to its
corresponding string ID suitable for use as a button label for the action.
Something like kDownloadCommandToLabelStringIDMap or something shorter.

Also document that the ordering follows the enum.

[Deepak, 2015/09/18 04:38:01]

Done.

+    {DownloadCommands::SHOW_IN_FOLDER, IDS_DOWNLOAD_LINK_SHOW},
+    {DownloadCommands::OPEN_WHEN_COMPLETE,
+     IDS_DOWNLOAD_STATUS_OPEN_WHEN_COMPLETE},
+    {DownloadCommands::ALWAYS_OPEN_TYPE, -1},
+    {DownloadCommands::PLATFORM_OPEN, -1},
+    {DownloadCommands::CANCEL, IDS_DOWNLOAD_LINK_CANCEL},
+    {DownloadCommands::PAUSE, IDS_DOWNLOAD_LINK_PAUSE},
+    {DownloadCommands::RESUME, IDS_DOWNLOAD_LINK_RESUME},
+    {DownloadCommands::DISCARD, IDS_DISCARD_DOWNLOAD},
+    {DownloadCommands::KEEP, IDS_CONFIRM_DOWNLOAD},
+    {DownloadCommands::LEARN_MORE_SCANNING,
+     IDS_DOWNLOAD_LINK_LEARN_MORE_SCANNING},
+    {DownloadCommands::LEARN_MORE_INTERRUPTED, -1},
+};
+
 }  // anonymous namespace
 
 DownloadItemNotification::DownloadItemNotification(
     content::DownloadItem* item,
     DownloadNotificationManagerForProfile* manager)
     : item_(item),
       weak_factory_(this) {
   ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
 
   message_center::RichNotificationData data;
@@ skipping 25 matching lines @@
     // Dangerous notifications don't have a click handler.
     return false;
   }
   return true;
 }
 
 void DownloadItemNotification::OnNotificationClose() {
   visible_ = false;
 
   if (item_ && item_->IsDangerous() && !item_->IsDone()) {
-    // TODO(yoshiki): Add metrics.
+    content::RecordAction(
+        UserMetricsAction("DownloadNotification.Close_Dangerous"));
     item_->Cancel(true /* by_user */);
     return;
   }
 
   if (image_decode_status_ == IN_PROGRESS) {
     image_decode_status_ = NOT_STARTED;
     ImageDecoder::Cancel(this);
   }
 }
 
@@ skipping 423 matching lines @@
           IDS_DOWNLOAD_STATUS_DOWNLOAD_FAILED_TITLE, file_name);
       break;
     case content::DownloadItem::MAX_DOWNLOAD_STATE:
       NOTREACHED();
   }
   return title_text;
 }
 
 base::string16 DownloadItemNotification::GetCommandLabel(
     DownloadCommands::Command command) const {
-  int id = -1;
-  switch (command) {

[asanka, 2015/09/17 14:29:40]

The advantage of using a switch is that:

1. Compilers are good about optimizing switches, specially when they are dense.
So even though it looks like a control flow structure that looks at each case
label, that's not how it behaves. In fact I'd argue that there's not much of a
performance gain here.

2. If a new value is added to the DownloadCommands::Command enum, the compiler
will fail to build this switch statement since it lacks a label for the new
value. You lose this when switching to a map. Get get something like that back,
you'll have to add a new enum to the end of the DownloadCommands::Command enum
(like MAX), and then add a static_assert() to validate the size of the map (i.e.
something like static_assert(arraysize(kDownloadNotificationCommand) ==
DownloadCommands::MAX, "...").

3. switches like this are compatible with enum classes. Maps aren't. This is, of
course, only a problem if we want to switch to using enum classes for
DownloadCommands (which would be a good thing), but that's not currently
planned.

By the time you accomplish 2, and given 1, you aren't gaining much by switching
to a map.

[Deepak, 2015/09/18 04:38:01]

Acknowledged.

-    case DownloadCommands::OPEN_WHEN_COMPLETE:
-      if (item_ && !item_->IsDone())
-        id = IDS_DOWNLOAD_STATUS_OPEN_WHEN_COMPLETE;
-      else
-        id = IDS_DOWNLOAD_STATUS_OPEN_WHEN_COMPLETE;

[asanka, 2015/09/17 14:29:40]

How did this slip by? I think it's supposed to say "Open" after the download is
done. Which kind of makes the map not work.

[Deepak, 2015/09/18 04:38:01]

I think here the message is "In both the cases when the download is going on, as
well as once download get completed, label is same"

-      break;
-    case DownloadCommands::PAUSE:
-      // Only for non menu.
-      id = IDS_DOWNLOAD_LINK_PAUSE;
-      break;
-    case DownloadCommands::RESUME:
-      // Only for non menu.
-      id = IDS_DOWNLOAD_LINK_RESUME;
-      break;
-    case DownloadCommands::SHOW_IN_FOLDER:
-      id = IDS_DOWNLOAD_LINK_SHOW;
-      break;
-    case DownloadCommands::DISCARD:
-      id = IDS_DISCARD_DOWNLOAD;
-      break;
-    case DownloadCommands::KEEP:
-      id = IDS_CONFIRM_DOWNLOAD;
-      break;
-    case DownloadCommands::CANCEL:
-      id = IDS_DOWNLOAD_LINK_CANCEL;
-      break;
-    case DownloadCommands::LEARN_MORE_SCANNING:
-      id = IDS_DOWNLOAD_LINK_LEARN_MORE_SCANNING;
-      break;
-    case DownloadCommands::ALWAYS_OPEN_TYPE:
-    case DownloadCommands::PLATFORM_OPEN:
-    case DownloadCommands::LEARN_MORE_INTERRUPTED:
-      // Only for menu.
-      NOTREACHED();
-      return base::string16();
-  }
-  CHECK(id != -1);
-  return l10n_util::GetStringUTF16(id);
+  int id = kDownloadNotificationCommand[command].id;

[asanka, 2015/09/17 14:29:40]

This lookup is off by one and will access past the end of the array for
LEARN_MORE_INTERRUPTED. You need bounds checks before using |command| as an
index.

[Deepak, 2015/09/18 04:38:01]

Done.

+  if (id != -1)
+    return l10n_util::GetStringUTF16(id);
+  NOTREACHED();
+  return base::string16();
 }
 
 base::string16 DownloadItemNotification::GetWarningStatusString() const {
   // Should only be called if IsDangerous().
   DCHECK(item_->IsDangerous());
   base::string16 elided_filename =
       item_->GetFileNameToReportUser().LossyDisplayName();
   switch (item_->GetDangerType()) {
     case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: {
       return l10n_util::GetStringUTF16(IDS_PROMPT_MALICIOUS_DOWNLOAD_URL);
@@ skipping 157 matching lines @@
                                                     profile_id);
 
   message_center::NotificationList::Notifications visible_notifications =
       g_browser_process->message_center()->GetVisibleNotifications();
   for (const auto& notification : visible_notifications) {
     if (notification->id() == notification_id_in_message_center)
       return true;
   }
   return false;
 }