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

chrome/browser/download/notification/download_item_notification.cc

Index: chrome/browser/download/notification/download_item_notification.cc
diff --git a/chrome/browser/download/notification/download_item_notification.cc b/chrome/browser/download/notification/download_item_notification.cc
index cdd22caae5c5ce7a38dad1f6b7d740e97e22ceac..a2cfaa7470e1413df01f58e65464c5a1887d9856 100644
--- a/chrome/browser/download/notification/download_item_notification.cc
+++ b/chrome/browser/download/notification/download_item_notification.cc
@@ -154,6 +154,27 @@ void RecordButtonClickAction(DownloadCommands::Command command) {
   }
 }
 
+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(
@@ -199,7 +220,8 @@ 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;
   }
@@ -643,46 +665,11 @@ base::string16 DownloadItemNotification::GetTitle() const {
 
 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 {