Implement additional UI changes for dangerous download warnings.

chrome/browser/download/download_shelf_context_menu.cc

Index: chrome/browser/download/download_shelf_context_menu.cc
diff --git a/chrome/browser/download/download_shelf_context_menu.cc b/chrome/browser/download/download_shelf_context_menu.cc
index 33be97bde5a2c16ffb56d0943df35447826e05b1..73cca42642271fce1622f0c3dba451dbe6eb7b0e 100644
--- a/chrome/browser/download/download_shelf_context_menu.cc
+++ b/chrome/browser/download/download_shelf_context_menu.cc
@@ -6,9 +6,11 @@
 
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_manager.h"
+#include "content/browser/tab_contents/page_navigator.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -21,8 +23,17 @@ DownloadShelfContextMenu::DownloadShelfContextMenu(
 }
 
 ui::SimpleMenuModel* DownloadShelfContextMenu::GetMenuModel() {
-  return download_item_->IsComplete() ? GetFinishedMenuModel()
-                                      : GetInProgressMenuModel();
+  ui::SimpleMenuModel* model;
+
+  if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS &&

[dmazzoni, 2011/12/01 19:49:22]

What does it mean if GetSafetyState() == DANGEROUS but GetDangerType() is not
DANGEROUS_URL or DANGEROUS_CONTENT?

I'd prefer something more like:

if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS) {
  if (...DANGEROUS_URL ||
      ...DANGEROUS_CONTENT) {
    model = GetMaliciousMenuModel():
  } else {
    NOTREACHED();
  }
}

Or, alternatively, use a switch() inside so that new enum values would cause a
compile error.

[asanka, 2011/12/02 00:05:04]

Done. Went with your 'if' suggestion.

+      (download_item_->GetDangerType() == DownloadStateInfo::DANGEROUS_URL ||
+       download_item_->GetDangerType() == DownloadStateInfo::DANGEROUS_CONTENT))
+    model = GetMaliciousMenuModel();
+  else if (download_item_->IsComplete())
+    model = GetFinishedMenuModel();
+  else
+    model = GetInProgressMenuModel();
+  return model;
 }
 
 bool DownloadShelfContextMenu::IsCommandIdEnabled(int command_id) const {
@@ -82,6 +93,25 @@ void DownloadShelfContextMenu::ExecuteCommand(int command_id) {
       if (download_item_->IsPartialDownload())
         download_item_->TogglePause();
       break;
+    case DISCARD:
+      if (download_item_->IsPartialDownload())
+        download_item_->Cancel(true);

[dmazzoni, 2011/12/01 19:49:22]

Does Cancel take effect immediately, does it close all file handles before
returning so that Delete will definitely succeed?

It seems pretty important to delete malicious files, and I know that deleting
files can be problematic on Windows, so I wonder if this should be a deferred
delete that retries?

[asanka, 2011/12/02 00:05:04]

Calling Delete() results in a task posted to the FILE thread that deletes the
actual file.  The DownloadItem cancels and removes itself without waiting for
the file to be deleted.

The Cancel() call (called implicitly from Delete()), causes
DownloadFileManager::CancelDownload() to be called, which also attempts to
delete the file.

+      download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);

[Randy Smith (Not in Mondays), 2011/12/01 19:50:46]

Remove (which Delete() calls) does a Cancel unilaterally, so it's not necessary
here.  I'm not sure whether it's desirable or not; that's up to you.  

[asanka, 2011/12/02 00:05:04]

Done.

+      // We have been deleted!

[dmazzoni, 2011/12/01 19:49:22]

Get rid of this comment

[Randy Smith (Not in Mondays), 2011/12/01 19:50:46]

Have you looked over the callers to this routine, and the ownership structure of
this class and its containing classes?  My memory is that the DownloadItemView
deletes itself when the DownloadItem goes away, and I suspect that that cascades
down to the DownloadShelfContextMenu.  Are you certain that the stack unwinding
all the way back up from here never assumes that the various "this" objects are
valid?  It would be worthwhile having comments on all such return values, so
that no-one later makes the (not unreasonable) assumption that "this" will still
be around in a method of the containing classes.

[asanka, 2011/12/02 00:05:04]

Done.

[asanka, 2011/12/02 00:05:04]

Done. In general, any time we run a message loop we run the risk of some
extension deleting the download item and causing DownloadItemView and everything
within it to disappear.

+      break;
+    case KEEP:
+      download_item_->DangerousDownloadValidated();
+      break;
+    case LEARN_MORE: {
+      Browser* browser = BrowserList::GetLastActive();
+      DCHECK(browser && browser->is_type_tabbed());
+      GURL learn_more_url(
+          l10n_util::GetStringUTF8(IDS_DOWNLOAD_SAFE_BROWSING_LEARN_MORE_URL));
+      OpenURLParams params(learn_more_url, GURL(), NEW_FOREGROUND_TAB,
+                           content::PAGE_TRANSITION_TYPED, false);
+      browser->OpenURL(params);
+      break;
+    }
     default:
       NOTREACHED();
   }
@@ -113,6 +143,12 @@ string16 DownloadShelfContextMenu::GetLabelForCommandId(int command_id) const {
         return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_RESUME_ITEM);
       return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_PAUSE_ITEM);
     }
+    case DISCARD:
+      return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_DISCARD);
+    case KEEP:
+      return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_KEEP);
+    case LEARN_MORE:
+      return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_LEARN_MORE);
     default:
       NOTREACHED();
       break;
@@ -161,3 +197,20 @@ ui::SimpleMenuModel* DownloadShelfContextMenu::GetFinishedMenuModel() {
 
   return finished_download_menu_model_.get();
 }
+
+ui::SimpleMenuModel* DownloadShelfContextMenu::GetMaliciousMenuModel() {
+  if (malicious_download_menu_model_.get())
+    return malicious_download_menu_model_.get();
+
+  malicious_download_menu_model_.reset(new ui::SimpleMenuModel(this));
+
+  malicious_download_menu_model_->AddItemWithStringId(
+      DISCARD, IDS_DOWNLOAD_MENU_DISCARD);
+  malicious_download_menu_model_->AddItemWithStringId(
+      KEEP, IDS_DOWNLOAD_MENU_KEEP);
+  malicious_download_menu_model_->AddSeparator();
+  malicious_download_menu_model_->AddItemWithStringId(
+      LEARN_MORE, IDS_DOWNLOAD_MENU_LEARN_MORE);
+
+  return malicious_download_menu_model_.get();
+}