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

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

Open the learn more page about downloading malicious file, when user clicks a notification of suspicious download.

BUG=488109
TEST=none

Committed: https://crrev.com/3e89893025e6eed6a6ddd3518901e5d7503105e8
Cr-Commit-Position: refs/heads/master@{#330688}

[yoshiki, 2015-05-14 16:39:58]

yoshiki@chromium.org changed reviewers:
+ asanka@chromium.org

[yoshiki, 2015-05-14 16:40:31]

Asanka, PTAL. Thanks.

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

https://codereview.chromium.org/1126343006/diff/1/chrome/browser/download/not...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/1126343006/diff/1/chrome/browser/download/not...
chrome/browser/download/notification/download_notification_item.cc:120:
item_->SetOpenWhenComplete(!item_->GetOpenWhenComplete());  // Toggle
Does this also cause the notification to close automatically when the download
is opened?

https://codereview.chromium.org/1126343006/diff/1/chrome/browser/download/not...
chrome/browser/download/notification/download_notification_item.cc:123: } else
if (item_->IsDangerous()) {
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:47:56]

Patchset #2 (id:20001) has been deleted

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

Asanka, PTAL. Thanks.

https://codereview.chromium.org/1126343006/diff/1/chrome/browser/download/not...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/1126343006/diff/1/chrome/browser/download/not...
chrome/browser/download/notification/download_notification_item.cc:120:
item_->SetOpenWhenComplete(!item_->GetOpenWhenComplete());  // Toggle
On 2015/05/14 22:07:42, asanka wrote:
> Does this also cause the notification to close automatically when the download
> is opened?

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.

https://codereview.chromium.org/1126343006/diff/1/chrome/browser/download/not...
chrome/browser/download/notification/download_notification_item.cc:123: } else
if (item_->IsDangerous()) {
On 2015/05/14 22:07:42, asanka wrote:
> 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();
> }

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.

[asanka, 2015-05-19 20:16:40]

lgtm

[yoshiki, 2015-05-20 01:51:26]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-05-20 01:52:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126343006/60001

[commit-bot: I haz the power, 2015-05-20 03:23:48]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-20 03:24:44]

Patchset 3 (id:??) landed as
https://crrev.com/3e89893025e6eed6a6ddd3518901e5d7503105e8
Cr-Commit-Position: refs/heads/master@{#330688}