PVer4: V4Database: Return stores that have a hash prefix for a given full hash

PVer4: V4Database: Return stores that have a hash prefix for a given full hash

Overall design document: https://goto.google.com/design-doc-v4store

BUG=543161
Committed: https://crrev.com/84307af5a87d46b25b2aaf2edec0e7a99c2f0d76
Cr-Commit-Position: refs/heads/master@{#406686}

[vakh (use Gerrit instead), 2016-07-19 22:58:02]

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

[vakh (use Gerrit instead), 2016-07-19 22:58:46]

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

[commit-bot: I haz the power, 2016-07-19 22:59:09]

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

[Nathan Parker, 2016-07-19 23:05:56]

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:171: void
V4Database::GetStoresMatchingFullHash(const FullHash& full_hash,
What's the usecase for this function?  I think the caller should specify which
stores it's interested in, since we don't want to check against stores that
aren't relevant (whitelists, etc).

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:34: typedef
std::vector<UpdateListIdentifier> StoresMatched;
A thought: It might be easier to read to not typedef this.  That way the reader
doesn't have to look up what StoresMatched is.

[vakh (use Gerrit instead), 2016-07-20 00:07:34]

Allow the caller to specify stores_to_look, return matching hash prefix

[vakh (use Gerrit instead), 2016-07-20 00:07:36]

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

[vakh (use Gerrit instead), 2016-07-20 00:07:43]

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:171: void
V4Database::GetStoresMatchingFullHash(const FullHash& full_hash,
On 2016/07/19 23:05:56, Nathan Parker wrote:
> What's the usecase for this function?  I think the caller should specify which
> stores it's interested in, since we don't want to check against stores that
> aren't relevant (whitelists, etc).

Done.

The goal of this method is to identify the stores that match a given full hash,
and then return the matching hash prefix.
I've updated the method to accept the list of stores to look at.

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2160193003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:34: typedef
std::vector<UpdateListIdentifier> StoresMatched;
On 2016/07/19 23:05:56, Nathan Parker wrote:
> A thought: It might be easier to read to not typedef this.  That way the
reader
> doesn't have to look up what StoresMatched is.

Now that it is a map of store identifiers to the matched hash prefix, I think a
typedef is useful.

[commit-bot: I haz the power, 2016-07-20 00:08:28]

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), 2016-07-20 00:09:53]

Nit: Updated a comment

[Nathan Parker, 2016-07-20 20:44:13]

lgtm

with some comments

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

https://codereview.chromium.org/2160193003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:175: for (const auto& store_pair :
*store_map_) {
This is sort of NxM complexity with two loops (for loop, and count()).  Instead,
just iterate over stores_to_look and use store_map->find(id), since it is a map.

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

https://codereview.chromium.org/2160193003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database_unittest.cc:41: bool
hash_prefix_matches_;
nit: "matches" here sounds like a plural of match, and I was expecting a list. 
Maybe "hash_prefix_has_match_" or just "has_match_". Or is it "should_match"?

https://codereview.chromium.org/2160193003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database_unittest.cc:381:
base::hash_set<UpdateListIdentifier> stores_to_look;
could do
stores_to_look = {linux_malware_id, win_malware_id}
I think.

[Nathan Parker, 2016-07-20 20:44:52]

You probably don't need to repeat the design doc link in each CL, if it's in the
bug.

[vakh (use Gerrit instead), 2016-07-20 21:09:03]

incorporate nparker@ feedback

[vakh (use Gerrit instead), 2016-07-20 21:09:05]

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

[commit-bot: I haz the power, 2016-07-20 21:09:44]

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), 2016-07-20 21:10:04]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-07-20 21:10:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2160193003/#ps60001
(title: "incorporate nparker@ feedback")

[vakh (use Gerrit instead), 2016-07-20 21:10:27]

Thanks for the review.

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

https://codereview.chromium.org/2160193003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:175: for (const auto& store_pair :
*store_map_) {
On 2016/07/20 20:44:13, Nathan Parker (OOO till Aug 1) wrote:
> This is sort of NxM complexity with two loops (for loop, and count()). 
Instead,
> just iterate over stores_to_look and use store_map->find(id), since it is a
map.

Done.

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

https://codereview.chromium.org/2160193003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database_unittest.cc:41: bool
hash_prefix_matches_;
On 2016/07/20 20:44:13, Nathan Parker (OOO till Aug 1) wrote:
> nit: "matches" here sounds like a plural of match, and I was expecting a list.

> Maybe "hash_prefix_has_match_" or just "has_match_". Or is it "should_match"?

Done.

https://codereview.chromium.org/2160193003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database_unittest.cc:381:
base::hash_set<UpdateListIdentifier> stores_to_look;
On 2016/07/20 20:44:13, Nathan Parker (OOO till Aug 1) wrote:
> could do
> stores_to_look = {linux_malware_id, win_malware_id}
> I think.

Done.

[commit-bot: I haz the power, 2016-07-20 21:10:33]

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-20 22:13:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-20 22:15:42]

Description was changed from

==========
PVer4: V4Database: Return stores that have a hash prefix for a given full hash

Overall design document: https://goto.google.com/design-doc-v4store

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

to

==========
PVer4: V4Database: Return stores that have a hash prefix for a given full hash

Overall design document: https://goto.google.com/design-doc-v4store

BUG=543161

Committed: https://crrev.com/84307af5a87d46b25b2aaf2edec0e7a99c2f0d76
Cr-Commit-Position: refs/heads/master@{#406686}
==========

[commit-bot: I haz the power, 2016-07-20 22:15:43]

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