From 85062ca24ea4d3fb2537f60d9af3362fadaf2ae5 Mon Sep 17 00:00:00 2001 From: "erikchen@chromium.org" Date: Mon, 18 Aug 2014 22:38:25 +0000 Subject: Store in memory cookies in a hash set instead of a list. The old code was performing expensive string searches against cookies stored in a list. See the bug for Instruments/DTrace results of the performance benefits. BUG=401629 Review URL: https://codereview.chromium.org/454623003 Cr-Commit-Position: refs/heads/master@{#290383} git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290383 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browsing_data_cookie_helper_unittest.cc | 227 ++++++++++++--------- 1 file changed, 129 insertions(+), 98 deletions(-) (limited to 'chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc') diff --git a/chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc b/chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc index faa861c..64a796d 100644 --- a/chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc +++ b/chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc @@ -15,12 +15,102 @@ namespace { +// Test expectations for a given cookie. +class CookieExpectation { + public: + CookieExpectation() : matched_(false) {} + + bool MatchesCookie(const net::CanonicalCookie& cookie) const { + if (!source_.empty() && source_ != cookie.Source()) + return false; + if (!domain_.empty() && domain_ != cookie.Domain()) + return false; + if (!path_.empty() && path_ != cookie.Path()) + return false; + if (!name_.empty() && name_ != cookie.Name()) + return false; + if (!value_.empty() && value_ != cookie.Value()) + return false; + return true; + } + + std::string source_; + std::string domain_; + std::string path_; + std::string name_; + std::string value_; + bool matched_; +}; + +// Matches a CookieExpectation against a Cookie. +class CookieMatcher { + public: + explicit CookieMatcher(const net::CanonicalCookie& cookie) + : cookie_(cookie) {} + bool operator()(const CookieExpectation& expectation) { + return expectation.MatchesCookie(cookie_); + } + net::CanonicalCookie cookie_; +}; + +// Unary predicate to determine whether an expectation has been matched. +bool ExpectationIsMatched(const CookieExpectation& expectation) { + return expectation.matched_; +} + class BrowsingDataCookieHelperTest : public testing::Test { public: BrowsingDataCookieHelperTest() : testing_profile_(new TestingProfile()) { } + virtual void SetUp() OVERRIDE { cookie_expectations_.clear(); } + + // Adds an expectation for a cookie that satisfies the given parameters. + void AddCookieExpectation(const char* source, + const char* domain, + const char* path, + const char* name, + const char* value) { + CookieExpectation matcher; + if (source) + matcher.source_ = source; + if (domain) + matcher.domain_ = domain; + if (path) + matcher.path_ = path; + if (name) + matcher.name_ = name; + if (value) + matcher.value_ = value; + cookie_expectations_.push_back(matcher); + } + + // Checks the existing expectations, and then clears all existing + // expectations. + void CheckCookieExpectations() { + ASSERT_EQ(cookie_expectations_.size(), cookie_list_.size()); + + // For each cookie, look for a matching expectation. + for (net::CookieList::iterator it = cookie_list_.begin(); + it != cookie_list_.end(); + ++it) { + CookieMatcher matcher(*it); + std::vector::iterator match = std::find_if( + cookie_expectations_.begin(), cookie_expectations_.end(), matcher); + if (match != cookie_expectations_.end()) + match->matched_ = true; + } + + // Check that each expectation has been matched. + unsigned long match_count = std::count_if(cookie_expectations_.begin(), + cookie_expectations_.end(), + ExpectationIsMatched); + EXPECT_EQ(cookie_expectations_.size(), match_count); + + cookie_expectations_.clear(); + } + void CreateCookiesForTest() { scoped_refptr cookie_monster = testing_profile_->GetCookieMonster(); @@ -44,135 +134,76 @@ class BrowsingDataCookieHelperTest : public testing::Test { } void FetchCallback(const net::CookieList& cookies) { - ASSERT_EQ(2UL, cookies.size()); cookie_list_ = cookies; - net::CookieList::const_iterator it = cookies.begin(); - // Correct because fetching cookies will get a sorted cookie list. - ASSERT_TRUE(it != cookies.end()); - EXPECT_EQ("www.google.com", it->Domain()); - EXPECT_EQ("A", it->Name()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ("www.gmail.google.com", it->Domain()); - EXPECT_EQ("B", it->Name()); - - ASSERT_TRUE(++it == cookies.end()); + AddCookieExpectation(NULL, "www.google.com", NULL, "A", NULL); + AddCookieExpectation(NULL, "www.gmail.google.com", NULL, "B", NULL); + CheckCookieExpectations(); } void DomainCookieCallback(const net::CookieList& cookies) { - ASSERT_EQ(2UL, cookies.size()); cookie_list_ = cookies; - net::CookieList::const_iterator it = cookies.begin(); - - // Correct because fetching cookies will get a sorted cookie list. - ASSERT_TRUE(it != cookies.end()); - EXPECT_EQ("www.google.com", it->Domain()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("1", it->Value()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ(".www.google.com", it->Domain()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("2", it->Value()); - ASSERT_TRUE(++it == cookies.end()); + AddCookieExpectation(NULL, "www.google.com", NULL, "A", "1"); + AddCookieExpectation(NULL, ".www.google.com", NULL, "A", "2"); + CheckCookieExpectations(); } void DeleteCallback(const net::CookieList& cookies) { - ASSERT_EQ(1UL, cookies.size()); - net::CookieList::const_iterator it = cookies.begin(); - - ASSERT_TRUE(it != cookies.end()); - EXPECT_EQ("www.gmail.google.com", it->Domain()); - EXPECT_EQ("B", it->Name()); - - ASSERT_TRUE(++it == cookies.end()); + cookie_list_ = cookies; + AddCookieExpectation(NULL, "www.gmail.google.com", NULL, "B", NULL); + CheckCookieExpectations(); } void CannedUniqueCallback(const net::CookieList& cookies) { - EXPECT_EQ(1UL, cookies.size()); cookie_list_ = cookies; - net::CookieList::const_iterator it = cookies.begin(); - - ASSERT_TRUE(it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ("www.google.com", it->Domain()); - EXPECT_EQ("/", it->Path()); - EXPECT_EQ("A", it->Name()); - - ASSERT_TRUE(++it == cookies.end()); + AddCookieExpectation( + "http://www.google.com/", "www.google.com", "/", "A", NULL); + CheckCookieExpectations(); } void CannedReplaceCookieCallback(const net::CookieList& cookies) { - EXPECT_EQ(5UL, cookies.size()); cookie_list_ = cookies; - net::CookieList::const_iterator it = cookies.begin(); - - ASSERT_TRUE(it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ("www.google.com", it->Domain()); - EXPECT_EQ("/", it->Path()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("2", it->Value()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ("www.google.com", it->Domain()); - EXPECT_EQ("/example/0", it->Path()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("4", it->Value()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ(".google.com", it->Domain()); - EXPECT_EQ("/", it->Path()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("6", it->Value()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ(".google.com", it->Domain()); - EXPECT_EQ("/example/1", it->Path()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("8", it->Value()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ(".www.google.com", it->Domain()); - EXPECT_EQ("/", it->Path()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("10", it->Value()); - - ASSERT_TRUE(++it == cookies.end()); + AddCookieExpectation( + "http://www.google.com/", "www.google.com", "/", "A", "2"); + AddCookieExpectation( + "http://www.google.com/", "www.google.com", "/example/0", "A", "4"); + AddCookieExpectation( + "http://www.google.com/", ".google.com", "/", "A", "6"); + AddCookieExpectation( + "http://www.google.com/", ".google.com", "/example/1", "A", "8"); + AddCookieExpectation( + "http://www.google.com/", ".www.google.com", "/", "A", "10"); + CheckCookieExpectations(); } void CannedDomainCookieCallback(const net::CookieList& cookies) { - ASSERT_EQ(2UL, cookies.size()); cookie_list_ = cookies; - net::CookieList::const_iterator it = cookies.begin(); - - ASSERT_TRUE(it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ("www.google.com", it->Domain()); - - ASSERT_TRUE(++it != cookies.end()); - EXPECT_EQ("http://www.google.com/", it->Source()); - EXPECT_EQ("A", it->Name()); - EXPECT_EQ(".www.google.com", it->Domain()); - - ASSERT_TRUE(++it == cookies.end()); + AddCookieExpectation( + "http://www.google.com/", "www.google.com", NULL, "A", NULL); + AddCookieExpectation( + "http://www.google.com/", ".www.google.com", NULL, "A", NULL); + CheckCookieExpectations(); } void CannedDifferentFramesCallback(const net::CookieList& cookie_list) { ASSERT_EQ(3U, cookie_list.size()); } + void DeleteCookie(BrowsingDataCookieHelper* helper, const GURL origin) { + for (net::CookieList::iterator it = cookie_list_.begin(); + it != cookie_list_.end(); + ++it) { + if (it->Source() == net::CanonicalCookie::GetCookieSourceFromURL(origin)) + helper->DeleteCookie(*it); + } + } + protected: content::TestBrowserThreadBundle thread_bundle_; scoped_ptr testing_profile_; + std::vector cookie_expectations_; net::CookieList cookie_list_; }; @@ -237,7 +268,7 @@ TEST_F(BrowsingDataCookieHelperTest, CannedDeleteCookie) { EXPECT_EQ(2u, helper->GetCookieCount()); - helper->DeleteCookie(cookie_list_[0]); + DeleteCookie(helper.get(), origin1); EXPECT_EQ(1u, helper->GetCookieCount()); helper->StartFetching( -- cgit v1.1