Fix for: Autofill should not ping the server again for the same form...

chrome/browser/autofill/autofill_download.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autofill/autofill_download.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/logging.h"
@@ skipping 17 matching lines @@
 #endif
 
 struct AutoFillDownloadManager::FormRequestData {
   std::vector<std::string> form_signatures;
   AutoFillRequestType request_type;
 };
 
 AutoFillDownloadManager::AutoFillDownloadManager(Profile* profile)
     : profile_(profile),
       observer_(NULL),
+      number_of_forms_to_cache_(16),

[dhollowa, 2011/01/25 23:33:02]

Let's pull this |16| out into a named constant within the .cc file. 
kFormCacheSize say?

[GeorgeY, 2011/01/25 23:51:30]

Done.

       next_query_request_(base::Time::Now()),
       next_upload_request_(base::Time::Now()),
       positive_upload_rate_(0),
       negative_upload_rate_(0),
       fetcher_id_for_unittest_(0),
       is_testing_(false) {
   // |profile_| could be NULL in some unit-tests.
   if (profile_) {
     PrefService* preferences = profile_->GetPrefs();
     positive_upload_rate_ =
@@ skipping 22 matching lines @@
     const ScopedVector<FormStructure>& forms,
     const AutoFillMetrics& metric_logger) {
   if (next_query_request_ > base::Time::Now()) {
     // We are in back-off mode: do not do the request.
     return false;
   }
   std::string form_xml;
   FormRequestData request_data;
   if (!FormStructure::EncodeQueryRequest(forms, &request_data.form_signatures,
                                          &form_xml))
     return false;

[dhollowa, 2011/01/25 23:33:02]

nit: style guide says this should be wrapped in braces due to the conditional
that spans two lines.

[GeorgeY, 2011/01/25 23:51:30]

Done.

 
   request_data.request_type = AutoFillDownloadManager::REQUEST_QUERY;
   metric_logger.Log(AutoFillMetrics::QUERY_SENT);
 
+  std::string query_data;
+  if (CheckCacheForQueryRequest(request_data.form_signatures, &query_data)) {
+    VLOG(1) << "AutoFillDownloadManager: query request has been retrieved from"
+            << "the cache";
+    if (observer_)
+      observer_->OnLoadedAutoFillHeuristics(query_data);
+    return true;
+  }
+
   return StartRequest(form_xml, request_data);
 }
 
 bool AutoFillDownloadManager::StartUploadRequest(
     const FormStructure& form, bool form_was_matched) {
   if (next_upload_request_ > base::Time::Now()) {
     // We are in back-off mode: do not do the request.
     return false;
   }
 
@@ skipping 86 matching lines @@
                                            URLFetcher::POST,
                                            this);
   url_fetchers_[fetcher] = request_data;
   fetcher->set_automatically_retry_on_5xx(false);
   fetcher->set_request_context(Profile::GetDefaultRequestContext());
   fetcher->set_upload_data("text/plain", form_xml);
   fetcher->Start();
   return true;
 }
 
+void AutoFillDownloadManager::CacheQueryRequest(
+    std::vector<std::string> const& forms_in_query,
+    std::string const& query_data) {
+  std::string signature = GetCombinedSignature(forms_in_query);
+  for (std::list<std::pair<std::string, std::string> >::iterator it =
+       cached_forms_.begin(); it != cached_forms_.end(); ++it) {
+    if (it->first == signature) {
+      // We hit the cache, move to the first position and return.
+      std::pair<std::string, std::string> data = *it;
+      cached_forms_.erase(it);
+      cached_forms_.push_front(data);
+      return;
+    }
+  }
+  std::pair<std::string, std::string> data;
+  data.first = signature;
+  data.second = query_data;
+  cached_forms_.push_front(data);
+  while (cached_forms_.size() > number_of_forms_to_cache_)
+    cached_forms_.pop_back();
+}
+
+bool AutoFillDownloadManager::CheckCacheForQueryRequest(
+    std::vector<std::string> const& forms_in_query,
+    std::string* query_data) const {
+  std::string signature = GetCombinedSignature(forms_in_query);
+  for (std::list<std::pair<std::string, std::string> >::const_iterator it =
+       cached_forms_.begin(); it != cached_forms_.end(); ++it) {
+    if (it->first == signature) {
+      // We hit the cache, fill the data and return.
+      *query_data = it->second;
+      return true;
+    }
+  }
+  return false;
+}
+
+std::string AutoFillDownloadManager::GetCombinedSignature(
+    std::vector<std::string> const& forms_in_query) const {
+  size_t total_size = forms_in_query.size();

[dhollowa, 2011/01/25 23:33:02]

My read of this is that if I passed in { "a", "b", "c" }, then I'd get back
"ab,c,".  And |total_size| would be 1 too big.  A small unit test for this
method in isolation would be helpful.

[GeorgeY, 2011/01/25 23:51:30]

nope "a,b,c\0" so everything is right. I do not think any unit tests needed for
*this* function. (evermore it is indirectly tested in every other test)

[dhollowa, 2011/01/26 00:15:02]

Ok, I misread it.  Seems ok.

On 2011/01/25 23:51:30, GeorgeY wrote:

+  for (size_t i = 0; i < forms_in_query.size(); ++i)
+    total_size += forms_in_query[i].length();
+  std::string signature;
+
+  signature.reserve(total_size);
+
+  for (size_t i = 0; i < forms_in_query.size(); ++i) {
+    if (i)
+      signature.append(",");
+    signature.append(forms_in_query[i]);
+  }
+  return signature;
+}
+
 void AutoFillDownloadManager::OnURLFetchComplete(
     const URLFetcher* source,
     const GURL& url,
     const net::URLRequestStatus& status,
     int response_code,
     const ResponseCookies& cookies,
     const std::string& data) {
   std::map<URLFetcher *, FormRequestData>::iterator it =
       url_fetchers_.find(const_cast<URLFetcher*>(source));
   if (it == url_fetchers_.end()) {
@@ skipping 42 matching lines @@
                  << " request has failed with response " << response_code;
     if (observer_) {
       observer_->OnHeuristicsRequestError(it->second.form_signatures[0],
                                           it->second.request_type,
                                           response_code);
     }
   } else {
     VLOG(1) << "AutoFillDownloadManager: " << type_of_request
             << " request has succeeded";
     if (it->second.request_type == AutoFillDownloadManager::REQUEST_QUERY) {
+      CacheQueryRequest(it->second.form_signatures, data);
       if (observer_)
         observer_->OnLoadedAutoFillHeuristics(data);
     } else {
       double new_positive_upload_rate = 0;
       double new_negative_upload_rate = 0;
       AutoFillUploadXmlParser parse_handler(&new_positive_upload_rate,
                                             &new_negative_upload_rate);
       buzz::XmlParser parser(&parse_handler);
       parser.Parse(data.data(), data.length(), true);
       if (parse_handler.succeeded()) {
         SetPositiveUploadRate(new_positive_upload_rate);
         SetNegativeUploadRate(new_negative_upload_rate);
       }
 
       if (observer_)
         observer_->OnUploadedAutoFillHeuristics(it->second.form_signatures[0]);
     }
   }
   delete it->first;
   url_fetchers_.erase(it);
 }