Open the learn more page when user clicks a notification of malicious download

chrome/browser/download/notification/download_notification_item.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_notification_item.h"
 
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_item_model.h"
@@ skipping 99 matching lines @@
   if (item_)
     item_->RemoveObserver(this);
 }
 
 void DownloadNotificationItem::OnNotificationClick() {
   if (openable_) {
     if (item_->IsDone()) {
       item_->OpenDownload();
       CloseNotificationByUser();
     } else {
       item_->SetOpenWhenComplete(!item_->GetOpenWhenComplete());  // Toggle

[asanka, 2015/05/14 22:07:42]

Does this also cause the notification to close automatically when the download
is opened?

[yoshiki, 2015/05/18 16:51:59]

No, the notification doesn't close. When the download finishes, the file opens
and the notification center closes if it's opened, but the notification leaves
in the notification center.

     }
+#if defined(FULL_SAFE_BROWSING)
+  } else if (item_->IsDangerous()) {

[asanka, 2015/05/14 22:07:42]

At this point, I think it would be readable to break this into blocks with early
returns. A chain of if() else if() ... blocks with heterogeneous conditionals
become very difficult to reason about as they grow larger.

For example, the fact that IsDangerous() reaches here and won't satisfy the
openable_ condition above can only be deemed by inspecting how 'openable_' is
set, and then backtracking to figure out that GetPossibleActions() won't return
OPEN... for a dangerous download. Nobody reading this code is going to put in
that much effort unless they are specifically looking for a bug in this logic.

Ideally we could find a way to centralize the concept of 'possible actions' into
GetPossibleActions(), where it also determines a default action. Then
OnNotificationClick() would just invoke the default action.

Or if you want to keep the default action determination here, let's organize
this method in a manner similar to GetPossibleActions(). I.e. something like:

OnNotificationClick() {
#if defined...
  if (item_->IsDangerous()) {
     Open help center article;
     return;
  }
#endif

  if (openable_) {
    ...
    return;
  }

  if (item_->GetState() == INTERRUPTED or CANCELLED) {
    ...
    return;
  }

  CloseNotificationByUser();
}

[yoshiki, 2015/05/18 16:51:59]

Thanks, I refined the code of OnNotificationClick and GetPossibleActions
(renamed to GetExtraActions) to make them similar. 

GetPossibleActions was originally in DownloadCommands and moved to
DownloadNotificationItem, so some code didn't match the code here.

+    DownloadCommands(item_).ExecuteCommand(
+        DownloadCommands::LEARN_MORE_SCANNING);
+#endif
   } else if (item_->GetState() == content::DownloadItem::INTERRUPTED ||
              item_->GetState() == content::DownloadItem::CANCELLED) {
     GetBrowser()->OpenURL(content::OpenURLParams(
         GURL(chrome::kChromeUIDownloadsURL), content::Referrer(),
         NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
         false /* is_renderer_initiated */));
     CloseNotificationByUser();
   } else if (item_->IsDone()) {
     CloseNotificationByUser();
   }
@@ skipping 332 matching lines @@
   NOTREACHED();
   return base::string16();
 }
 
 Browser* DownloadNotificationItem::GetBrowser() {
   chrome::ScopedTabbedBrowserDisplayer browser_displayer(
       profile_, chrome::GetActiveDesktop());
   DCHECK(browser_displayer.browser());
   return browser_displayer.browser();
 }