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

chrome/browser/autofill/autofill_download.cc

Index: chrome/browser/autofill/autofill_download.cc
===================================================================
--- chrome/browser/autofill/autofill_download.cc	(revision 72523)
+++ chrome/browser/autofill/autofill_download.cc	(working copy)
@@ -35,6 +35,7 @@
 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),
@@ -82,6 +83,15 @@
   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);
 }
 
@@ -188,6 +198,60 @@
   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,
@@ -250,6 +314,7 @@
     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 {