PVer4: Re-use the PVer3 implementation to get full hashes

PVer4: Re-use the PVer3 implementation to get full hashes
(but not do anything with that yet).

BUG=543161

[vakh (use Gerrit instead), 2016-08-19 00:24:13]

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

[commit-bot: I haz the power, 2016-08-19 00:24:49]

Dry run: 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-08-19 00:30:17]

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

[commit-bot: I haz the power, 2016-08-19 00:30:18]

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...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vakh (use Gerrit instead), 2016-08-19 00:32:21]

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

[vakh (use Gerrit instead), 2016-08-19 00:32:22]

nparker@, shess@: This CL takes the implementation of CheckBrowseUrl one step
further by requesting full hashes (and then ignoring them).

Some of the code in this CL will go away after http://crbug.com/616647 gets
fixed.
The fix for that bug would move the caching logic for full hashes into
V4GetHashProtocolManager.
Following that, I'll modify that class to support PVer4 data structures i.e.
HashPrefix, FullHash, etc.
And following that, I'll remove the TODOs that I am adding in this CL.

kcarattini@ will get to http://crbug.com/616647 this/next week.

[vakh (use Gerrit instead), 2016-08-19 00:34:10]

BUILD: v4_local_database_manager depends on :v4_get_hash_protocol_manager

[vakh (use Gerrit instead), 2016-08-19 00:34:12]

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

[commit-bot: I haz the power, 2016-08-19 00:34:59]

Dry run: 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-08-19 01:57:47]

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

[commit-bot: I haz the power, 2016-08-19 01:57:48]

Dry run: This issue passed the CQ dry run.

[kcarattini, 2016-08-19 03:30:55]

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

[kcarattini, 2016-08-19 03:30:56]

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right):

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.h:71: // synchronously.
Works with V3 data structures.
Is this method going away eventually after the caching is moved? If so, can you
add that in the comment? If not, and these structures are going to be used with
V4 going forward, then we shouldn't call them V3 structures.

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.h:89: virtual void
GetFullHashesWithApis(const std::vector<SBPrefix>& prefixes,
Does this method need to be updated too?

[Scott Hess - ex-Googler, 2016-08-23 20:25:46]

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

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:305:
FullHashCallback callback) {
I think these are in the opposite order from the header?

I think having the function overloaded has strong potential for confusion. 
IMHO, you should probably just rename the V3 version to GetFullHashesV3 or
something, which will make it clear which call sites need conversion.  Yeah, it
means you'll touch more files than you might like, but *shrug*, you'll have to
touch them eventually.

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:325: callback);
I'm a tiny bit nervous about DCHECK(!thread_types.empty()) plus
*(thread_types.begin()).  If someone breaks the contract in production, then
that becomes an undefined something.  Upgrading to a CHECK() would make sense to
me (though in the long term DCHECK makes more sense).

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

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:171:
DCHECK_EQ(matched_hash_prefixes.empty(), matched_threat_types.empty());
Since they all have to match, I'd pick the shortest one as canonical.  Maybe it
would let you get away without the middle line being wrapped :-).

Alternately, since the next line checks for a specific empty(), the if() and
else could each have appropriate DCHECK() statements for empty() or !empty(). 
That would cost 4 lines, which is your current count.

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

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:156:
base::Time::Now());
Will full_hash_results acquire contents later?  Otherwise
std::vector<SBFullHashResult>() inline makes sense to me.

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2261603002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:229: typedef
base::hash_set<ThreatType, EnumClassHash> ThreatTypeSet;
These feel like more-expensive versions of std::bit_set<>, both in terms of
storage and CPU.

[vakh (use Gerrit instead), 2016-09-12 22:07:56]

I've re-written most of the code that's touched in this CL eliminating the need
for this CL so I am going to delete it.

Please see https://codereview.chromium.org/2233103002/ for reviewing the
re-write. Thanks and really sorry for wasting your review time on this CL but I
felt it was better to re-write it than to add a poor/hacky patch.

[vakh (use Gerrit instead), 2016-09-12 22:08:13]

Description was changed from

==========
PVer4: Re-use the PVer3 implementation to get full hashes
(but not do anything with that yet).

BUG=543161
==========

to

==========
PVer4: Re-use the PVer3 implementation to get full hashes
(but not do anything with that yet).

BUG=543161
==========