S: GetThreatSeverity should handle the case of CheckResourceUrl.

GetThreatSeverity should handle the case of CheckResourceUrl.

Added the case statement and a unit test that forces a full hash match
which causes this method to get executed.

BUG=689672
Review-Url: https://codereview.chromium.org/2680163002
Cr-Commit-Position: refs/heads/master@{#448885}
Committed: https://chromium.googlesource.com/chromium/src/+/4e2788493f4bef375b52e78e5a9ee38f2d92b191

[vakh (use Gerrit instead), 2017-02-07 22:32:47]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-07 22:33:57]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org, shess@chromium.org

[vakh (use Gerrit instead), 2017-02-07 22:33:57]

Please review this small patch soon.

I'd need to get this merged into M57 so the sooner we submit this, the better.
Thanks!

[commit-bot: I haz the power, 2017-02-07 22:34:17]

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

[vakh (use Gerrit instead), 2017-02-07 22:38:26]

rebase and fix compile-time errors caused by rebase

[vakh (use Gerrit instead), 2017-02-07 22:38:28]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 22:39:55]

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

[Nathan Parker, 2017-02-07 22:59:59]

lgtm

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:94: default:
How about replacing the default with all the other threat types, so the compiler
will catch it next time we add one?

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:41:
FullHashInfos full_hash_infos)
nit: const FullHashInfos&, here and below.  Performance isn't as critical in
tests, but a good habit.

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:182:
DCHECK(threat_type != SB_THREAT_TYPE_SAFE || threat_hash.empty());
nit: I had to think about what this was asserting.  Would this be clearer as

if (threat_type == SB_THREAT_SAFE) 
  DCHECK(threat_has.empty();

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:777:
base::Time::Now() + base::TimeDelta::FromMinutes(5));
Is this time used?  Anything with a real clock can be flakey.  If we don't need
a real time, just use base::Time(0)

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:805: //  
CheckExtensionIDs()
I'll just leave and arrow pointing to these...
/'\
 |
 | 
 |


;-)

[vakh (use Gerrit instead), 2017-02-07 23:11:08]

nparker@'s review

[vakh (use Gerrit instead), 2017-02-07 23:11:10]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-07 23:11:21]

Thanks for the quick review.

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:94: default:
On 2017/02/07 22:59:58, Nathan Parker wrote:
> How about replacing the default with all the other threat types, so the
compiler
> will catch it next time we add one?

Done.

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:41:
FullHashInfos full_hash_infos)
On 2017/02/07 22:59:58, Nathan Parker wrote:
> nit: const FullHashInfos&, here and below.  Performance isn't as critical in
> tests, but a good habit.

Done.

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:182:
DCHECK(threat_type != SB_THREAT_TYPE_SAFE || threat_hash.empty());
On 2017/02/07 22:59:58, Nathan Parker wrote:
> nit: I had to think about what this was asserting.  Would this be clearer as
> 
> if (threat_type == SB_THREAT_SAFE) 
>   DCHECK(threat_has.empty();

I read it as: "Either it is not safe, or the hash is empty"

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:777:
base::Time::Now() + base::TimeDelta::FromMinutes(5));
On 2017/02/07 22:59:59, Nathan Parker wrote:
> Is this time used?  Anything with a real clock can be flakey.  If we don't
need
> a real time, just use base::Time(0)

Done.

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:805: //  
CheckExtensionIDs()
On 2017/02/07 22:59:58, Nathan Parker wrote:
> I'll just leave and arrow pointing to these...
> /'\
>  |
>  | 
>  |
> 
> 
> ;-)

Acknowledged. Will implement very soon.

[commit-bot: I haz the power, 2017-02-07 23:11:25]

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

[Scott Hess - ex-Googler, 2017-02-07 23:37:51]

LGTM.

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:182:
DCHECK(threat_type != SB_THREAT_TYPE_SAFE || threat_hash.empty());
On 2017/02/07 23:11:21, vakh (Varun Khaneja) wrote:
> On 2017/02/07 22:59:58, Nathan Parker wrote:
> > nit: I had to think about what this was asserting.  Would this be clearer as
> > 
> > if (threat_type == SB_THREAT_SAFE) 
> >   DCHECK(threat_has.empty();
> 
> I read it as: "Either it is not safe, or the hash is empty"

Well, that's also how I read it - but I still have to think about what it is
asserting.

I think what you're asserting is that when it is safe, the hash is empty?  IMHO
Nathan's phrasing says exactly that.  Similar would be:

if (!thread_hash.empty())
  DCHECK_NE(SB_THREAD_TYPE_SAFE, thread_type);

But ... it's your code, so I'm not blocking.

I will mention that DCHECK in test code has poor outcomes.  Sometimes/often you
end up with a crash without a decent backtrace.

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (left):

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:23: typedef
base::Callback<void()> NullCallback;
For future reference, base::Closure is exactly this.

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:459:
ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
These can just be FullHashInfos(), right?  FullHashInfos({}) feels like it's
doing something tricksy.

[commit-bot: I haz the power, 2017-02-07 23:37:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 23:37:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vakh (use Gerrit instead), 2017-02-07 23:51:08]

shess@'s review + revert explicit cases

[vakh (use Gerrit instead), 2017-02-07 23:51:10]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-07 23:51:22]

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:182:
DCHECK(threat_type != SB_THREAT_TYPE_SAFE || threat_hash.empty());
On 2017/02/07 23:37:51, Scott Hess wrote:
> On 2017/02/07 23:11:21, vakh (Varun Khaneja) wrote:
> > On 2017/02/07 22:59:58, Nathan Parker wrote:
> > > nit: I had to think about what this was asserting.  Would this be clearer
as
> > > 
> > > if (threat_type == SB_THREAT_SAFE) 
> > >   DCHECK(threat_has.empty();
> > 
> > I read it as: "Either it is not safe, or the hash is empty"
> 
> Well, that's also how I read it - but I still have to think about what it is
> asserting.
> 
> I think what you're asserting is that when it is safe, the hash is empty? 
IMHO
> Nathan's phrasing says exactly that.  Similar would be:
> 
> if (!thread_hash.empty())
>   DCHECK_NE(SB_THREAD_TYPE_SAFE, thread_type);
> 
> But ... it's your code, so I'm not blocking.
> 
> I will mention that DCHECK in test code has poor outcomes.  Sometimes/often
you
> end up with a crash without a decent backtrace.

Acknowledged.

Re-wrote to be more accurate and less complicated to understand.
It is all still within DCHECK though.

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (left):

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:23: typedef
base::Callback<void()> NullCallback;
On 2017/02/07 23:37:51, Scott Hess wrote:
> For future reference, base::Closure is exactly this.

Acknowledged.

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:459:
ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
On 2017/02/07 23:37:51, Scott Hess wrote:
> These can just be FullHashInfos(), right?  FullHashInfos({}) feels like it's
> doing something tricksy.

../../components/safe_browsing_db/v4_local_database_manager_unittest.cc:459:46:
error: parentheses were disambiguated as a function declaration
[-Werror,-Wvexing-parse]
  ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos());
                                             ^~~~~~~~~~~~~~~~~

[commit-bot: I haz the power, 2017-02-07 23:52:16]

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

[vakh (use Gerrit instead), 2017-02-07 23:52:32]

Nit: Remove extra empty line

[vakh (use Gerrit instead), 2017-02-07 23:52:34]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-07 23:52:43]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-02-07 23:52:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2680163002/#ps80001
(title: "Nit: Remove extra empty line")

[commit-bot: I haz the power, 2017-02-07 23:52:55]

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

[Scott Hess - ex-Googler, 2017-02-07 23:55:42]

still lgtm

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2680163002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:459:
ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos({}));
On 2017/02/07 23:51:22, vakh (Varun Khaneja) wrote:
> On 2017/02/07 23:37:51, Scott Hess wrote:
> > These can just be FullHashInfos(), right?  FullHashInfos({}) feels like it's
> > doing something tricksy.
> 
>
../../components/safe_browsing_db/v4_local_database_manager_unittest.cc:459:46:
> error: parentheses were disambiguated as a function declaration
> [-Werror,-Wvexing-parse]
>   ScopedFakeGetHashProtocolManagerFactory pin(FullHashInfos());
>                                              ^~~~~~~~~~~~~~~~~

Reeeeeeeeeeeeally?  Wow.  I guess.  What if you didn't have a convenient
constructor to trigger using uniform initialization, though?  OK.

[commit-bot: I haz the power, 2017-02-08 00:21:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 00:21:23]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vakh (use Gerrit instead), 2017-02-08 02:19:53]

Nit: s/Time::Time/Time

[vakh (use Gerrit instead), 2017-02-08 02:19:55]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-08 02:19:59]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-02-08 02:20:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2680163002/#ps100001
(title: "Nit: s/Time::Time/Time")

[commit-bot: I haz the power, 2017-02-08 02:20:46]

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, 2017-02-08 03:11:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486520399911770,
"parent_rev": "6af1d2696c2d41ed9a32358d9870992c0835a2d4", "commit_rev":
"4e2788493f4bef375b52e78e5a9ee38f2d92b191"}

[commit-bot: I haz the power, 2017-02-08 03:12:34]

Description was changed from

==========
GetThreatSeverity should handle the case of CheckResourceUrl.

Added the case statement and a unit test that forces a full hash match
which causes this method to get executed.

BUG=689672
==========

to

==========
GetThreatSeverity should handle the case of CheckResourceUrl.

Added the case statement and a unit test that forces a full hash match
which causes this method to get executed.

BUG=689672

Review-Url: https://codereview.chromium.org/2680163002
Cr-Commit-Position: refs/heads/master@{#448885}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e2788493f4bef375b52e78e5a9e...
==========

[commit-bot: I haz the power, 2017-02-08 03:12:35]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4e2788493f4bef375b52e78e5a9e...