Fetch incremental updates. Store new state in V4Store.

components/safe_browsing_db/v4_update_protocol_manager.cc

Index: components/safe_browsing_db/v4_update_protocol_manager.cc
diff --git a/components/safe_browsing_db/v4_update_protocol_manager.cc b/components/safe_browsing_db/v4_update_protocol_manager.cc
index f0fad8d07acf607a566378cdc9baf9f0eaf42e7c..46dcf335f92fa38b501b15eb5266e6493a9b1470 100644
--- a/components/safe_browsing_db/v4_update_protocol_manager.cc
+++ b/components/safe_browsing_db/v4_update_protocol_manager.cc
@@ -77,11 +77,9 @@ class V4UpdateProtocolManagerFactoryImpl
   std::unique_ptr<V4UpdateProtocolManager> CreateProtocolManager(
       net::URLRequestContextGetter* request_context_getter,
       const V4ProtocolConfig& config,
-      const base::hash_map<UpdateListIdentifier, std::string>&
-          current_list_states,
       V4UpdateCallback callback) override {
-    return std::unique_ptr<V4UpdateProtocolManager>(new V4UpdateProtocolManager(
-        request_context_getter, config, current_list_states, callback));
+    return std::unique_ptr<V4UpdateProtocolManager>(
+        new V4UpdateProtocolManager(request_context_getter, config, callback));
   }
 
  private:
@@ -97,14 +95,12 @@ V4UpdateProtocolManagerFactory* V4UpdateProtocolManager::factory_ = NULL;
 std::unique_ptr<V4UpdateProtocolManager> V4UpdateProtocolManager::Create(
     net::URLRequestContextGetter* request_context_getter,
     const V4ProtocolConfig& config,
-    const base::hash_map<UpdateListIdentifier, std::string>&
-        current_list_states,
     V4UpdateCallback callback) {
   if (!factory_) {
     factory_ = new V4UpdateProtocolManagerFactoryImpl();
   }
   return factory_->CreateProtocolManager(request_context_getter, config,
-                                         current_list_states, callback);
+                                         callback);
 }
 
 void V4UpdateProtocolManager::ResetUpdateErrors() {
@@ -115,11 +111,8 @@ void V4UpdateProtocolManager::ResetUpdateErrors() {
 V4UpdateProtocolManager::V4UpdateProtocolManager(
     net::URLRequestContextGetter* request_context_getter,
     const V4ProtocolConfig& config,
-    const base::hash_map<UpdateListIdentifier, std::string>&
-        current_list_states,
     V4UpdateCallback update_callback)
-    : current_list_states_(current_list_states),
-      update_error_count_(0),
+    : update_error_count_(0),
       update_back_off_mult_(1),
       next_update_interval_(base::TimeDelta::FromSeconds(
           base::RandInt(kV4TimerStartIntervalSecMin,
@@ -130,8 +123,6 @@ V4UpdateProtocolManager::V4UpdateProtocolManager(
       update_callback_(update_callback) {
   // Do not auto-schedule updates. Let the owner (V4LocalDatabaseManager) do it
   // when it is ready to process updates.
-  DVLOG(1) << "V4UpdateProtocolManager::V4UpdateProtocolManager: "
-           << "next_update_interval_: " << next_update_interval_;
 }
 
 V4UpdateProtocolManager::~V4UpdateProtocolManager() {}
@@ -140,7 +131,9 @@ bool V4UpdateProtocolManager::IsUpdateScheduled() const {
   return update_timer_.IsRunning();
 }
 
-void V4UpdateProtocolManager::ScheduleNextUpdate() {
+void V4UpdateProtocolManager::ScheduleNextUpdate(
+    std::unique_ptr<StoreStateMap> store_state_map) {
+  store_state_map_ = std::move(store_state_map);
   ScheduleNextUpdateWithBackoff(false);
 }
 
@@ -198,13 +191,13 @@ void V4UpdateProtocolManager::ScheduleNextUpdateAfterInterval(
                       &V4UpdateProtocolManager::IssueUpdateRequest);
 }
 
+// static
 std::string V4UpdateProtocolManager::GetBase64SerializedUpdateRequestProto(
-    const base::hash_map<UpdateListIdentifier, std::string>&
-        current_list_states) {
+    const StoreStateMap* store_state_map) {

[Scott Hess - ex-Googler, 2016/06/24 23:01:19]

WRT my other comment about "Does StoreStateMap need unique_ptr?", in this case
the code requires that store_state_map not be NULL, so it could be a reference.

I'm not entirely clear if there's potential to get a NULL here, now that I think
about it.

[vakh (use Gerrit instead), 2016/06/27 19:38:27]

It can't be NULL but I added a check with NOTREACHED, instead of causing a crash
later.

[Scott Hess - ex-Googler, 2016/06/27 22:19:17]

To be clear, where I was going with all of that is that if it can't be NULL,
then if it were a reference this code wouldn't have to ask the question, the
calling code would have to.  If it had a well-defined result for the NULL case
which can be used in the regular flow, it makes sense to do the check here.  If
the NULL case should be detected and lead to not sending the request, it should
be detected in the caller, instead.

[Scott Hess - ex-Googler, 2016/06/27 22:37:58]

That was ambiguous - I mean if this function's signature were |const
StoreStateMap&| then this code could assume it always has a map.  The map could
be empty, of course :-).

[vakh (use Gerrit instead), 2016/06/27 23:58:53]

Done.
Also added a DCHECK to ensure the store_state_map isn't empty at least in DEBUG
builds because that's unexpected.

   // Build the request. Client info and client states are not added to the
   // request protocol buffer. Client info is passed as params in the url.
   FetchThreatListUpdatesRequest request;
-  for (const auto& entry : current_list_states) {
+  for (const auto& entry : *store_state_map) {
     const auto& list_to_update = entry.first;
     const auto& state = entry.second;
     ListUpdateRequest* list_update_request = request.add_list_update_requests();
@@ -277,7 +270,7 @@ void V4UpdateProtocolManager::IssueUpdateRequest() {
   }
 
   std::string req_base64 =
-      GetBase64SerializedUpdateRequestProto(current_list_states_);
+      GetBase64SerializedUpdateRequestProto(store_state_map_.get());
   GURL update_url;
   net::HttpRequestHeaders headers;
   GetUpdateUrlAndHeaders(req_base64, &update_url, &headers);
@@ -320,6 +313,9 @@ void V4UpdateProtocolManager::OnURLFetchComplete(
     }
     request_.reset();
 
+    UMA_HISTOGRAM_COUNTS("SafeBrowsing.V4UpdateResponseSizeKB",
+                         data.size() / 1024);
+
     // Invoke the callback with list_update_responses.
     // The caller should update its state now, based on list_update_responses.
     // The callback must call ScheduleNextUpdate() at the end to resume