Implement additional UI changes for dangerous download warnings.

chrome/browser/download/download_shelf_context_menu.cc

 // Copyright (c) 2011 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/download_shelf_context_menu.h"
 
 #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"
 
 DownloadShelfContextMenu::~DownloadShelfContextMenu() {}
 
 DownloadShelfContextMenu::DownloadShelfContextMenu(
     BaseDownloadItemModel* download_model)
     : download_model_(download_model),
       download_item_(download_model->download()) {
 }
 
 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 {
   switch (command_id) {
     case SHOW_IN_FOLDER:
     case OPEN_WHEN_COMPLETE:
       return download_item_->CanShowInFolder();
     case ALWAYS_OPEN_TYPE:
       return download_item_->CanOpenDownload() &&
           !Extension::IsExtension(download_item_->GetStateInfo().target_name);
@@ skipping 39 matching lines @@
     case CANCEL:
       download_model_->CancelTask();
       break;
     case TOGGLE_PAUSE:
       // It is possible for the download to complete before the user clicks the
       // menu item, recheck if the download is in progress state before toggling
       // pause.
       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();
   }
 }
 
 bool DownloadShelfContextMenu::GetAcceleratorForCommandId(
     int command_id, ui::Accelerator* accelerator) {
   return false;
 }
 
@@ skipping 11 matching lines @@
       return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_OPEN);
     case ALWAYS_OPEN_TYPE:
       return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_ALWAYS_OPEN_TYPE);
     case CANCEL:
       return l10n_util::GetStringUTF16(IDS_DOWNLOAD_MENU_CANCEL);
     case TOGGLE_PAUSE: {
       if (download_item_->IsPaused())
         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;
   }
   return string16();
 }
 
 ui::SimpleMenuModel* DownloadShelfContextMenu::GetInProgressMenuModel() {
   if (in_progress_download_menu_model_.get())
     return in_progress_download_menu_model_.get();
@@ skipping 28 matching lines @@
       ALWAYS_OPEN_TYPE, IDS_DOWNLOAD_MENU_ALWAYS_OPEN_TYPE);
   finished_download_menu_model_->AddSeparator();
   finished_download_menu_model_->AddItemWithStringId(
       SHOW_IN_FOLDER, IDS_DOWNLOAD_MENU_SHOW);
   finished_download_menu_model_->AddSeparator();
   finished_download_menu_model_->AddItemWithStringId(
       CANCEL, IDS_DOWNLOAD_MENU_CANCEL);
 
   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();
+}