SafeBrowsing PVer4: Send mutable response to the database and the stores

components/safe_browsing_db/v4_protocol_manager_util.h

Index: components/safe_browsing_db/v4_protocol_manager_util.h
diff --git a/components/safe_browsing_db/v4_protocol_manager_util.h b/components/safe_browsing_db/v4_protocol_manager_util.h
index 7b510de3dedf59396fa290aebdfe411106bfd1b5..5f0469543df5f4b33a378d8a859510abe9d6176b 100644
--- a/components/safe_browsing_db/v4_protocol_manager_util.h
+++ b/components/safe_browsing_db/v4_protocol_manager_util.h
@@ -58,7 +58,7 @@ struct UpdateListIdentifier {
   ThreatType threat_type;
 
   UpdateListIdentifier(PlatformType, ThreatEntryType, ThreatType);
-  UpdateListIdentifier(const ListUpdateResponse&);
+  UpdateListIdentifier(const ListUpdateResponse*);

[Scott Hess - ex-Googler, 2016/06/28 03:58:43]

Should this be explicit?  The change to pointer makes me wonder if it could
match in new contexts.

[vakh (use Gerrit instead), 2016/06/28 21:34:14]

Not sure what you mean by that. What could be the new context(s)?
But, changed back to reference.

[vakh (use Gerrit instead), 2016/06/28 21:59:48]

nparker@ told me that you probable meant the use of "explicit" keyword.
I don't feel there's a reason to use explicit here -- as long as the caller
passes something that can become ListUpdateResponse (whether implicitly or
explicitly), we shouldn't care.
To ensure that the values inside the ListUpdateResponse are valid, I already
have the DCHECKs in place inside the constructor.

[Scott Hess - ex-Googler, 2016/06/28 22:22:25]

What I meant by explicit was the keyword, and why I mentioned it is that you
changed from reference to pointer.  As a reference, it can't be matched
inappropriately, I think.  But as a pointer, I think it could also match
nullptr, and perhaps NULL, which means it might be able to match 0, and that
maybe leads to other weird cases (though I _think_ it can't match arbitrary
integers, or integer-convertable things, I think just the special case of 0. 
Unless c++11 changes things, or I'm wrong).  I think with explicit you'd have to
cast NULL to a ListUpdateResponse* explicitly to make that case happen.

In any case, the pointer version needs a NULL check.  The reference version does
not, because it can't be NULL.

It may be that it should be explicit either way:
   https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

[vakh (use Gerrit instead), 2016/06/29 19:58:52]

Done. Thanks for pointing it out.

 
   bool operator==(const UpdateListIdentifier& other) const;
   bool operator!=(const UpdateListIdentifier& other) const;
@@ -82,6 +82,9 @@ typedef base::hash_map<UpdateListIdentifier, std::string> StoreFileNameMap;
 // Represents the state of each store.
 typedef base::hash_map<UpdateListIdentifier, std::string> StoreStateMap;
 
+// Sever response, parsed in vector form.
+typedef std::vector<std::unique_ptr<ListUpdateResponse>> ParsedServerResponse;
+
 // Enumerate failures for histogramming purposes.  DO NOT CHANGE THE
 // ORDERING OF THESE VALUES.
 enum V4OperationResult {