Support new Safe Browsing list "goog-badresource-shavar" in SafeBrowsingDatabase.

Support new Safe Browsing list "goog-badresource-shavar" in SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204
Committed: https://crrev.com/fbe7992cbb2da2bf1f129c3e2b6ca7271da86dc8
Cr-Commit-Position: refs/heads/master@{#376460}

[veranika, 2016-02-05 20:27:21]

Description was changed from

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204
==========

to

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

This CL would not be submitted until goog-badresource-shavar is actually served.

BUG=582204
==========

[veranika, 2016-02-05 20:54:45]

Description was changed from

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

This CL would not be submitted until goog-badresource-shavar is actually served.

BUG=582204
==========

to

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204
==========

[veranika, 2016-02-05 20:54:45]

veranika@chromium.org changed reviewers:
+ nparker@chromium.org, rkaplow@chromium.org

[veranika, 2016-02-05 20:56:12]

rkaplow: could you please review the histograms.xml change?
nparker: could you please review everything but histograms.xml?

[rkaplow, 2016-02-05 21:21:29]

lgtm

https://codereview.chromium.org/1673733002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/local_database_manager_unittest.cc (right):

https://codereview.chromium.org/1673733002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/local_database_manager_unittest.cc:15: 
unneeded?

[Nathan Parker, 2016-02-16 21:42:00]

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:98: case IPBLACKLIST:
Now that there are three levels, how about turning this into generalized ranking
code? The switch could set "cur_rank_level" to 1, 2,or 3.  Then if
cur_rank_level > highest_rank_level, save the index and threat.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:152: return threat;
Same comment as above.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:417: return true;
Do you need to check that the DB is available, like is done in CheckBrowseUrl?

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.h (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:75:
std::vector<std::string> url_hit_hash;
Why is this needed?

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:357: bool
enable_resource_blacklist_;
Do you need this?  If you want a flag or a finch control, it might be better to
put it outside of database_manager code.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager_unittest.cc (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager_unittest.cc:15: 
rm blank line

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.h:78: bool
enable_resource_blacklist);
Do you need this?

[veranika, 2016-02-17 15:37:54]

https://codereview.chromium.org/1673733002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/local_database_manager_unittest.cc (right):

https://codereview.chromium.org/1673733002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/local_database_manager_unittest.cc:15: 
On 2016/02/05 21:21:29, rkaplow wrote:
> unneeded?

Done.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:98: case IPBLACKLIST:
On 2016/02/16 21:42:00, nparker wrote:
> Now that there are three levels, how about turning this into generalized
ranking
> code? The switch could set "cur_rank_level" to 1, 2,or 3.  Then if
> cur_rank_level > highest_rank_level, save the index and threat.

Done, but in my code smaller numbers correspond to more severe threats. I did
that to be able to return immediately once the severest list is encountered.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:152: return threat;
On 2016/02/16 21:42:00, nparker wrote:
> Same comment as above.

Done.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:417: return true;
On 2016/02/16 21:42:00, nparker wrote:
> Do you need to check that the DB is available, like is done in CheckBrowseUrl?
 

Done.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.h (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:75:
std::vector<std::string> url_hit_hash;
On 2016/02/16 21:42:00, nparker wrote:
> Why is this needed?

In the resource request incident report we don't report the specific resource
URL that triggered the incident, but the hash of the URL pattern. This is less
privacy intrusive and this way we can report evil.com only once even if multiple
scripts were loaded from evil.com.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:357: bool
enable_resource_blacklist_;
On 2016/02/16 21:42:00, nparker wrote:
> Do you need this?  If you want a flag or a finch control, it might be better
to
> put it outside of database_manager code.

Probably I don't. I added it here mostly to be uniform with similar lists.
Deleted.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager_unittest.cc (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager_unittest.cc:15: 
On 2016/02/16 21:42:00, nparker wrote:
> rm blank line

Done.

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1673733002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.h:78: bool
enable_resource_blacklist);
On 2016/02/16 21:42:00, nparker wrote:
> Do you need this?

Probably I don't. I added it here mostly to be uniform with similar lists.
Deleted.

[Nathan Parker, 2016-02-17 16:00:40]

lgtm

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:79: int
GetThreatLevel(ListType threat) {
How about GetThreatSeverity?

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:96: default:
Can we remove the default?  That way new threats have to be specifically ranked.

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:113: int threat_level =
GetThreatLevel(threat);
Cool, this is better.  Thanks.

[veranika, 2016-02-18 14:53:26]

Resolved comments and rebase-updated the patch.

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:79: int
GetThreatLevel(ListType threat) {
On 2016/02/17 16:00:39, nparker wrote:
> How about GetThreatSeverity? 

Done.

https://codereview.chromium.org/1673733002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.cc:96: default:
On 2016/02/17 16:00:39, nparker wrote:
> Can we remove the default?  That way new threats have to be specifically
ranked.

Done.

[veranika, 2016-02-19 15:23:34]

The CQ bit was checked by veranika@chromium.org

[veranika, 2016-02-19 15:23:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1673733002/#ps160001
(title: "Post-merge test fix")

[commit-bot: I haz the power, 2016-02-19 15:24:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673733002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673733002/160001

[commit-bot: I haz the power, 2016-02-19 16:58:18]

Description was changed from

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204
==========

to

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204
==========

[commit-bot: I haz the power, 2016-02-19 16:58:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-19 16:59:26]

Description was changed from

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204
==========

to

==========
Support new Safe Browsing list "goog-badresource-shavar" in
SafeBrowsingDatabase.

Load the list from servers. Check URLs for presence in the database.

BUG=582204

Committed: https://crrev.com/fbe7992cbb2da2bf1f129c3e2b6ca7271da86dc8
Cr-Commit-Position: refs/heads/master@{#376460}
==========

[commit-bot: I haz the power, 2016-02-19 16:59:27]

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

[igorcov, 2016-10-26 11:37:33]

igorcov@chromium.org changed reviewers:
+ igorcov@chromium.org

[igorcov, 2016-10-26 11:37:34]

https://codereview.chromium.org/1673733002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1673733002/diff/160001/components/safe_browsi...
components/safe_browsing_db/util.cc:126: *list = kResourceBlacklist;
The break statement was stolen from above and now MODULEWHITELIST is impossible
to happen

[grt (UTC plus 2), 2016-10-26 11:44:48]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2016-10-26 11:44:49]

https://codereview.chromium.org/1673733002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1673733002/diff/160001/components/safe_browsi...
components/safe_browsing_db/util.cc:126: *list = kResourceBlacklist;
On 2016/10/26 11:37:34, igorcov wrote:
> The break statement was stolen from above and now MODULEWHITELIST is
impossible
> to happen

fix out for review in https://codereview.chromium.org/2451163002/