[Downloads] Correctly test page transition when calculating danger level.

[Downloads] Correctly test page transition when calculating danger level.

One of the heuristics used to minimize spurious dangerous download
warnings is to check if the navigation that resulted in the download was
initiated via the Omnibox. The test was incorrectly checking if a
ui::PageTransition value was equal to PAGE_TRANSITION_FROM_ADDRESS_BAR.
However, PAGE_TRANSITION_FROM_ADDRESS_BAR is a bit mask that can
accompany multiple core page transition types. This CL fixes the code to
correctly check the single bit.

R=pkasting@chromium.org
BUG=622834
Committed: https://crrev.com/1e639fd737e70a09c363d8342c1af3b9d3143280
Cr-Commit-Position: refs/heads/master@{#403792}

[Peter Kasting, 2016-06-29 23:35:02]

LGTM

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:893: // * If the user
navigated to the download URL via the omnibox (either by
Nit: Remove "If" from both of these bullets (you already said it above)

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:900: // A user is
considered familiar with a referring origin if a visit for a page
Nit: I'd move this to the end of the second bullet.

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1041:
ui::PAGE_TRANSITION_LINK,
So many of these enums probably should be converted to enum classes
eventually...

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1050: },
Nit: Maybe you should write a helper that constructs the desired
DownloadTestCase given the info that you actually change in each case, since so
much of it is repeated boilerplate.

This would also make the disparity I ask about in the next comment more
apparent.

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1056: {AUTOMATIC,
content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,
Shouldn't this case and the next one be using
DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS?  DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE should
mean the file is unsafe even if FROM_ADDRESS_BAR is missing, no?

[asanka, 2016-06-30 15:48:52]

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:893: // * If the user
navigated to the download URL via the omnibox (either by
On 2016/06/29 at 23:35:01, Peter Kasting (slow) wrote:
> Nit: Remove "If" from both of these bullets (you already said it above)

Done.

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:900: // A user is
considered familiar with a referring origin if a visit for a page
On 2016/06/29 at 23:35:01, Peter Kasting (slow) wrote:
> Nit: I'd move this to the end of the second bullet.

Done.

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1041:
ui::PAGE_TRANSITION_LINK,
On 2016/06/29 at 23:35:01, Peter Kasting (slow) wrote:
> So many of these enums probably should be converted to enum classes
eventually...

Acknowledged.

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1050: },
On 2016/06/29 at 23:35:01, Peter Kasting (slow) wrote:
> Nit: Maybe you should write a helper that constructs the desired
DownloadTestCase given the info that you actually change in each case, since so
much of it is repeated boilerplate.
> 
> This would also make the disparity I ask about in the next comment more
apparent.

Yeah. I rewrote the test cases so that the definition eliminates as much of the
boilerplate as possible.

https://codereview.chromium.org/2090013006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1056: {AUTOMATIC,
content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,
On 2016/06/29 at 23:35:01, Peter Kasting (slow) wrote:
> Shouldn't this case and the next one be using
DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS?  DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE should
mean the file is unsafe even if FROM_ADDRESS_BAR is missing, no?

Danger type of the download == F( |danger level of the target file type|, |page
transition type|, |whether or not there was a user gesture|, |whether or not
there was a history entry for the origin| ...)

The danger type of a download is the conclusion we reach after considering the
danger level of the file type, whether there was a user gesture, whether there
was a history entry for the origin etc... In this case and the next the danger
level for the file type is "ALLOW_ON_USER_GESTURE". That is the only danger
level for which we factor in the page transition.

In the absence of FROM_ADDRESS_BAR, both cases result in the download being
marked DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE.

It's confusing since there are a few very closely related concepts being thrown
around here with overlapping names. As you mentioned I refactored the tests so
that it's clearer. But I think we should rename these concepts so that they are
easier to tell apart.

[asanka, 2016-07-05 15:58:10]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-07-05 15:58:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2090013006/#ps60001
(title: "Merge with ToT")

[commit-bot: I haz the power, 2016-07-05 15:58:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 16:40:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-05 16:41:47]

Description was changed from

==========
[Downloads] Correctly test page transition when calculating danger level.

One of the heuristics used to minimize spurious dangerous download
warnings is to check if the navigation that resulted in the download was
initiated via the Omnibox. The test was incorrectly checking if a
ui::PageTransition value was equal to PAGE_TRANSITION_FROM_ADDRESS_BAR.
However, PAGE_TRANSITION_FROM_ADDRESS_BAR is a bit mask that can
accompany multiple core page transition types. This CL fixes the code to
correctly check the single bit.

R=pkasting@chromium.org
BUG=622834
==========

to

==========
[Downloads] Correctly test page transition when calculating danger level.

One of the heuristics used to minimize spurious dangerous download
warnings is to check if the navigation that resulted in the download was
initiated via the Omnibox. The test was incorrectly checking if a
ui::PageTransition value was equal to PAGE_TRANSITION_FROM_ADDRESS_BAR.
However, PAGE_TRANSITION_FROM_ADDRESS_BAR is a bit mask that can
accompany multiple core page transition types. This CL fixes the code to
correctly check the single bit.

R=pkasting@chromium.org
BUG=622834

Committed: https://crrev.com/1e639fd737e70a09c363d8342c1af3b9d3143280
Cr-Commit-Position: refs/heads/master@{#403792}
==========

[commit-bot: I haz the power, 2016-07-05 16:41:48]

Patchset 4 (id:??) landed as
https://crrev.com/1e639fd737e70a09c363d8342c1af3b9d3143280
Cr-Commit-Position: refs/heads/master@{#403792}