Fix for: Autofill should not ping the server again for the same form
BUG=67039
TEST=unit-tested. Any network sniffer could be used to test that there no subsequent calls to the web for the same form.
Review URL: http://codereview.chromium.org/6366014
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@72585 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/autofill/autofill_download.cc b/chrome/browser/autofill/autofill_download.cc
index cc7e119..2c9cc4c8 100644
--- a/chrome/browser/autofill/autofill_download.cc
+++ b/chrome/browser/autofill/autofill_download.cc
@@ -27,6 +27,10 @@
#define AUTO_FILL_QUERY_SERVER_NAME_START_IN_HEADER "SOMESERVER/"
#endif
+namespace {
+const size_t kMaxFormCacheSize = 16;
+};
+
struct AutoFillDownloadManager::FormRequestData {
std::vector<std::string> form_signatures;
AutoFillRequestType request_type;
@@ -35,6 +39,7 @@
AutoFillDownloadManager::AutoFillDownloadManager(Profile* profile)
: profile_(profile),
observer_(NULL),
+ max_form_cache_size_(kMaxFormCacheSize),
next_query_request_(base::Time::Now()),
next_upload_request_(base::Time::Now()),
positive_upload_rate_(0),
@@ -76,12 +81,22 @@
std::string form_xml;
FormRequestData request_data;
if (!FormStructure::EncodeQueryRequest(forms, &request_data.form_signatures,
- &form_xml))
+ &form_xml)) {
return false;
+ }
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 +203,60 @@
return true;
}
+void AutoFillDownloadManager::CacheQueryRequest(
+ const std::vector<std::string>& forms_in_query,
+ const std::string& query_data) {
+ std::string signature = GetCombinedSignature(forms_in_query);
+ for (QueryRequestCache::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() > max_form_cache_size_)
+ cached_forms_.pop_back();
+}
+
+bool AutoFillDownloadManager::CheckCacheForQueryRequest(
+ const std::vector<std::string>& forms_in_query,
+ std::string* query_data) const {
+ std::string signature = GetCombinedSignature(forms_in_query);
+ for (QueryRequestCache::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(
+ const std::vector<std::string>& forms_in_query) const {
+ size_t total_size = forms_in_query.size();
+ 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 +319,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 {
diff --git a/chrome/browser/autofill/autofill_download.h b/chrome/browser/autofill/autofill_download.h
index 8937557f..157062c 100644
--- a/chrome/browser/autofill/autofill_download.h
+++ b/chrome/browser/autofill/autofill_download.h
@@ -6,8 +6,10 @@
#define CHROME_BROWSER_AUTOFILL_AUTOFILL_DOWNLOAD_H_
#pragma once
+#include <list>
#include <map>
#include <string>
+#include <vector>
#include "base/scoped_vector.h"
#include "base/time.h"
@@ -96,6 +98,7 @@
friend class AutoFillDownloadTestHelper; // unit-test.
struct FormRequestData;
+ typedef std::list<std::pair<std::string, std::string> > QueryRequestCache;
// Initiates request to AutoFill servers to download/upload heuristics.
// |form_xml| - form structure XML to upload/download.
@@ -106,6 +109,25 @@
bool StartRequest(const std::string& form_xml,
const FormRequestData& request_data);
+ // Each request is page visited. We store last |max_form_cache_size|
+ // request, to avoid going over the wire. Set to 16 in constructor. Warning:
+ // the search is linear (newest first), so do not make the constant very big.
+ void set_max_form_cache_size(size_t max_form_cache_size) {
+ max_form_cache_size_ = max_form_cache_size;
+ }
+
+ // Caches query request. |forms_in_query| is a vector of form signatures in
+ // the query. |query_data| is the successful data returned over the wire.
+ void CacheQueryRequest(const std::vector<std::string>& forms_in_query,
+ const std::string& query_data);
+ // Returns true if query is in the cache, while filling |query_data|, false
+ // otherwise. |forms_in_query| is a vector of form signatures in the query.
+ bool CheckCacheForQueryRequest(const std::vector<std::string>& forms_in_query,
+ std::string* query_data) const;
+ // Concatenates |forms_in_query| into one signature.
+ std::string GetCombinedSignature(
+ const std::vector<std::string>& forms_in_query) const;
+
// URLFetcher::Delegate implementation:
virtual void OnURLFetchComplete(const URLFetcher* source,
const GURL& url,
@@ -123,6 +145,10 @@
std::map<URLFetcher*, FormRequestData> url_fetchers_;
AutoFillDownloadManager::Observer *observer_;
+ // Cached QUERY requests.
+ QueryRequestCache cached_forms_;
+ size_t max_form_cache_size_;
+
// Time when next query/upload requests are allowed. If 50x HTTP received,
// exponential back off is initiated, so this times will be in the future
// for awhile.
diff --git a/chrome/browser/autofill/autofill_download_unittest.cc b/chrome/browser/autofill/autofill_download_unittest.cc
index aaf2a5e9..b9967d6 100644
--- a/chrome/browser/autofill/autofill_download_unittest.cc
+++ b/chrome/browser/autofill/autofill_download_unittest.cc
@@ -53,6 +53,10 @@
download_manager.SetObserver(NULL);
}
+ void LimitCache(size_t cache_size) {
+ download_manager.set_max_form_cache_size(cache_size);
+ }
+
// AutoFillDownloadManager::Observer overridables:
virtual void OnLoadedAutoFillHeuristics(
const std::string& heuristic_xml) {
@@ -61,7 +65,6 @@
response.type_of_response = QUERY_SUCCESSFULL;
responses_.push_back(response);
};
-
virtual void OnUploadedAutoFillHeuristics(const std::string& form_signature) {
ResponseData response;
response.type_of_response = UPLOAD_SUCCESSFULL;
@@ -276,6 +279,16 @@
fetcher = factory.GetFetcherByID(3);
EXPECT_EQ(NULL, fetcher);
+ // Modify form structures to miss the cache.
+ form.fields.push_back(webkit_glue::FormField(ASCIIToUTF16("Address line 2"),
+ ASCIIToUTF16("address2"),
+ string16(),
+ ASCIIToUTF16("text"),
+ 0,
+ false));
+ form_structure = new FormStructure(form);
+ form_structures.push_back(form_structure);
+
// Request with id 3.
EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures,
@@ -329,3 +342,178 @@
// Make sure consumer of URLFetcher does the right thing.
URLFetcher::set_factory(NULL);
}
+
+TEST(AutoFillDownloadTest, CacheQueryTest) {
+ MessageLoopForUI message_loop;
+ AutoFillDownloadTestHelper helper;
+ // Create and register factory.
+ TestURLFetcherFactory factory;
+ URLFetcher::set_factory(&factory);
+
+ FormData form;
+ form.method = ASCIIToUTF16("post");
+ form.fields.push_back(webkit_glue::FormField(ASCIIToUTF16("username"),
+ ASCIIToUTF16("username"),
+ string16(),
+ ASCIIToUTF16("text"),
+ 0,
+ false));
+ form.fields.push_back(webkit_glue::FormField(ASCIIToUTF16("First Name"),
+ ASCIIToUTF16("firstname"),
+ string16(),
+ ASCIIToUTF16("text"),
+ 0,
+ false));
+ form.fields.push_back(webkit_glue::FormField(ASCIIToUTF16("Last Name"),
+ ASCIIToUTF16("lastname"),
+ string16(),
+ ASCIIToUTF16("text"),
+ 0,
+ false));
+ FormStructure *form_structure = new FormStructure(form);
+ ScopedVector<FormStructure> form_structures0;
+ form_structures0.push_back(form_structure);
+
+ form.fields.push_back(webkit_glue::FormField(ASCIIToUTF16("email"),
+ ASCIIToUTF16("email"),
+ string16(),
+ ASCIIToUTF16("text"),
+ 0,
+ false));
+ // Slightly different form - so different request.
+ form_structure = new FormStructure(form);
+ ScopedVector<FormStructure> form_structures1;
+ form_structures1.push_back(form_structure);
+
+ form.fields.push_back(webkit_glue::FormField(ASCIIToUTF16("email2"),
+ ASCIIToUTF16("email2"),
+ string16(),
+ ASCIIToUTF16("text"),
+ 0,
+ false));
+ // Slightly different form - so different request.
+ form_structure = new FormStructure(form);
+ ScopedVector<FormStructure> form_structures2;
+ form_structures2.push_back(form_structure);
+
+ // Limit cache to two forms.
+ helper.LimitCache(2);
+
+ const char *responses[] = {
+ "<autofillqueryresponse>"
+ "<field autofilltype=\"0\" />"
+ "<field autofilltype=\"3\" />"
+ "<field autofilltype=\"5\" />"
+ "</autofillqueryresponse>",
+ "<autofillqueryresponse>"
+ "<field autofilltype=\"0\" />"
+ "<field autofilltype=\"3\" />"
+ "<field autofilltype=\"5\" />"
+ "<field autofilltype=\"9\" />"
+ "</autofillqueryresponse>",
+ "<autofillqueryresponse>"
+ "<field autofilltype=\"0\" />"
+ "<field autofilltype=\"3\" />"
+ "<field autofilltype=\"5\" />"
+ "<field autofilltype=\"9\" />"
+ "<field autofilltype=\"0\" />"
+ "</autofillqueryresponse>",
+ };
+
+ // Request with id 0.
+ MockAutoFillMetrics mock_metric_logger;
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures0,
+ mock_metric_logger));
+ // No responses yet
+ EXPECT_EQ(static_cast<size_t>(0), helper.responses_.size());
+
+ TestURLFetcher* fetcher = factory.GetFetcherByID(0);
+ ASSERT_TRUE(fetcher);
+ fetcher->delegate()->OnURLFetchComplete(fetcher, GURL(),
+ net::URLRequestStatus(),
+ 200, ResponseCookies(),
+ std::string(responses[0]));
+ ASSERT_EQ(static_cast<size_t>(1), helper.responses_.size());
+ EXPECT_EQ(responses[0], helper.responses_.front().response);
+
+ helper.responses_.clear();
+
+ // No actual request - should be a cache hit.
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures0,
+ mock_metric_logger));
+ // Data is available immediately from cache - no over-the-wire trip.
+ ASSERT_EQ(static_cast<size_t>(1), helper.responses_.size());
+ EXPECT_EQ(responses[0], helper.responses_.front().response);
+ helper.responses_.clear();
+
+ // Request with id 1.
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures1,
+ mock_metric_logger));
+ // No responses yet
+ EXPECT_EQ(static_cast<size_t>(0), helper.responses_.size());
+
+ fetcher = factory.GetFetcherByID(1);
+ ASSERT_TRUE(fetcher);
+ fetcher->delegate()->OnURLFetchComplete(fetcher, GURL(),
+ net::URLRequestStatus(),
+ 200, ResponseCookies(),
+ std::string(responses[1]));
+ ASSERT_EQ(static_cast<size_t>(1), helper.responses_.size());
+ EXPECT_EQ(responses[1], helper.responses_.front().response);
+
+ helper.responses_.clear();
+
+ // Request with id 2.
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures2,
+ mock_metric_logger));
+
+ fetcher = factory.GetFetcherByID(2);
+ ASSERT_TRUE(fetcher);
+ fetcher->delegate()->OnURLFetchComplete(fetcher, GURL(),
+ net::URLRequestStatus(),
+ 200, ResponseCookies(),
+ std::string(responses[2]));
+ ASSERT_EQ(static_cast<size_t>(1), helper.responses_.size());
+ EXPECT_EQ(responses[2], helper.responses_.front().response);
+
+ helper.responses_.clear();
+
+ // No actual requests - should be a cache hit.
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures1,
+ mock_metric_logger));
+
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures2,
+ mock_metric_logger));
+
+ ASSERT_EQ(static_cast<size_t>(2), helper.responses_.size());
+ EXPECT_EQ(responses[1], helper.responses_.front().response);
+ EXPECT_EQ(responses[2], helper.responses_.back().response);
+ helper.responses_.clear();
+
+ // The first structure should've expired.
+ // Request with id 3.
+ EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+ EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures0,
+ mock_metric_logger));
+ // No responses yet
+ EXPECT_EQ(static_cast<size_t>(0), helper.responses_.size());
+
+ fetcher = factory.GetFetcherByID(3);
+ ASSERT_TRUE(fetcher);
+ fetcher->delegate()->OnURLFetchComplete(fetcher, GURL(),
+ net::URLRequestStatus(),
+ 200, ResponseCookies(),
+ std::string(responses[0]));
+ ASSERT_EQ(static_cast<size_t>(1), helper.responses_.size());
+ EXPECT_EQ(responses[0], helper.responses_.front().response);
+
+ // Make sure consumer of URLFetcher does the right thing.
+ URLFetcher::set_factory(NULL);
+}
+
