Skip to content

GitLab

T
tangled
Commit 0e84cea6 authored by rdsmith's avatar rdsmith Committed by Commit Bot
Browse files
Options

Simplify CookieMonster::SetAllCookies{,Async}() implementation. 

Given that for now the call must remain present but the precise semantics
of the call aren't important (see
https://codereview.chromium.org/2882063002/#msg64), it's worthwhile having
as clean and simple an implementation of reasonable semantics as possible.

BUG=None
R=mmenke@chromium.org

Review-Url: https://codereview.chromium.org/2974363002
Cr-Commit-Position: refs/heads/master@{#486248}
parent fa66128a
Show whitespace changes
Inline Side-by-side
Showing with 8 additions and 216 deletions
net/cookies/cookie_monster.cc
View file @ 0e84cea6
...@@ -196,19 +196,6 @@ bool LRACookieSorter(const CookieMonster::CookieMap::iterator& it1, ...@@ -196,19 +196,6 @@ bool LRACookieSorter(const CookieMonster::CookieMap::iterator& it1,
return it1->second->CreationDate() < it2->second->CreationDate(); return it1->second->CreationDate() < it2->second->CreationDate();
} }
// Compare cookies using name, domain and path, so that "equivalent" cookies
// (per RFC 2965) are equal to each other.
bool PartialDiffCookieSorter(const CanonicalCookie& a,
const CanonicalCookie& b) {
return a.PartialCompare(b);
}
// This is a stricter ordering than PartialDiffCookieOrdering, where all fields
// are used.
bool FullDiffCookieSorter(const CanonicalCookie& a, const CanonicalCookie& b) {
return a.FullCompare(b);
}
// Our strategy to find duplicates is: // Our strategy to find duplicates is:
// (1) Build a map from (cookiename, cookiepath) to // (1) Build a map from (cookiename, cookiepath) to
// {list of cookies with this signature, sorted by creation time}. // {list of cookies with this signature, sorted by creation time}.
...@@ -1482,46 +1469,18 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc, ...@@ -1482,46 +1469,18 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
void CookieMonster::SetAllCookies(CookieList list, void CookieMonster::SetAllCookies(CookieList list,
SetCookiesCallback callback) { SetCookiesCallback callback) {
DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(thread_checker_.CalledOnValidThread());
CookieList positive_diff;
CookieList negative_diff;
CookieList old_cookies;
old_cookies.reserve(cookies_.size());
for (const auto& cookie : cookies_)
old_cookies.push_back(*cookie.second.get());
ComputeCookieDiff(&old_cookies, &list, &positive_diff, &negative_diff); // Nuke the existing store.
while (!cookies_.empty()) {
for (const auto& cookie_to_delete : negative_diff) { // TODO(rdsmith): The CANONICAL is a lie.
for (CookieMapItPair its = InternalDeleteCookie(cookies_.begin(), true, DELETE_COOKIE_CANONICAL);
cookies_.equal_range(GetKey(cookie_to_delete.Domain()));
its.first != its.second; ++its.first) {
// The creation date acts as the unique index...
if (its.first->second->CreationDate() ==
cookie_to_delete.CreationDate()) {
// TODO(rdsmith): DELETE_COOKIE_CANONICAL is incorrect and should
// be changed.
InternalDeleteCookie(its.first, true, DELETE_COOKIE_CANONICAL);
break;
}
}
}
if (positive_diff.size() == 0) {
MaybeRunCookieCallback(std::move(callback), true);
return;
} }
// Set all passed in cookies.
for (const auto& cookie : list) { for (const auto& cookie : list) {
const std::string key(GetKey(cookie.Domain())); const std::string key(GetKey(cookie.Domain()));
Time creation_time = cookie.CreationDate(); Time creation_time = cookie.CreationDate();
bool already_expired = cookie.IsExpired(creation_time); if (cookie.IsExpired(creation_time))
bool result =
DeleteAnyEquivalentCookie(key, cookie, true, false, already_expired);
DCHECK(!result);
if (already_expired)
continue; continue;
if (cookie.IsPersistent()) { if (cookie.IsPersistent()) {
...@@ -2054,41 +2013,6 @@ void CookieMonster::DoCookieCallbackForURL(base::OnceClosure callback, ...@@ -2054,41 +2013,6 @@ void CookieMonster::DoCookieCallbackForURL(base::OnceClosure callback,
std::move(callback).Run(); std::move(callback).Run();
} }
void CookieMonster::ComputeCookieDiff(CookieList* old_cookies,
CookieList* new_cookies,
CookieList* cookies_to_add,
CookieList* cookies_to_delete) {
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(old_cookies);
DCHECK(new_cookies);
DCHECK(cookies_to_add);
DCHECK(cookies_to_delete);
DCHECK(cookies_to_add->empty());
DCHECK(cookies_to_delete->empty());
// Sort both lists.
// A set ordered by FullDiffCookieSorter is also ordered by
// PartialDiffCookieSorter.
std::sort(old_cookies->begin(), old_cookies->end(), FullDiffCookieSorter);
std::sort(new_cookies->begin(), new_cookies->end(), FullDiffCookieSorter);
// Select any old cookie for deletion if no new cookie has the same name,
// domain, and path.
std::set_difference(
old_cookies->begin(), old_cookies->end(), new_cookies->begin(),
new_cookies->end(),
std::inserter(*cookies_to_delete, cookies_to_delete->begin()),
PartialDiffCookieSorter);
// Select any new cookie for addition (or update) if no old cookie is exactly
// equivalent.
std::set_difference(new_cookies->begin(), new_cookies->end(),
old_cookies->begin(), old_cookies->end(),
std::inserter(*cookies_to_add, cookies_to_add->begin()),
FullDiffCookieSorter);
}
void CookieMonster::RunCookieChangedCallbacks(const CanonicalCookie& cookie, void CookieMonster::RunCookieChangedCallbacks(const CanonicalCookie& cookie,
ChangeCause cause) { ChangeCause cause) {
DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(thread_checker_.CalledOnValidThread());
......
net/cookies/cookie_monster.h
View file @ 0e84cea6
...@@ -149,8 +149,8 @@ class NET_EXPORT CookieMonster : public CookieStore { ...@@ -149,8 +149,8 @@ class NET_EXPORT CookieMonster : public CookieStore {
~CookieMonster() override; ~CookieMonster() override;
// Writes all the cookies in |list| into the store, replacing existing // Writes all the cookies in |list| into the store, replacing all cookies
// cookies that collide. Does not affect cookies not listed in |list|. // currently present in store.
// This method does not flush the backend. // This method does not flush the backend.
// TODO(rdsmith, mmenke): Do not use this function; it is deprecated // TODO(rdsmith, mmenke): Do not use this function; it is deprecated
// and should be removed. // and should be removed.
...@@ -255,9 +255,6 @@ class NET_EXPORT CookieMonster : public CookieStore { ...@@ -255,9 +255,6 @@ class NET_EXPORT CookieMonster : public CookieStore {
// For FindCookiesForKey. // For FindCookiesForKey.
FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, ShortLivedSessionCookies); FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, ShortLivedSessionCookies);
// For ComputeCookieDiff.
FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, ComputeCookieDiff);
// For CookieSource histogram enum. // For CookieSource histogram enum.
FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, CookieSourceHistogram); FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, CookieSourceHistogram);
...@@ -638,16 +635,6 @@ class NET_EXPORT CookieMonster : public CookieStore { ...@@ -638,16 +635,6 @@ class NET_EXPORT CookieMonster : public CookieStore {
// given URL are loaded. // given URL are loaded.
void DoCookieCallbackForURL(base::OnceClosure callback, const GURL& url); void DoCookieCallbackForURL(base::OnceClosure callback, const GURL& url);
// Computes the difference between |old_cookies| and |new_cookies|, and writes
// the result in |cookies_to_add| and |cookies_to_delete|.
// This function has the side effect of changing the order of |old_cookies|
// and |new_cookies|. |cookies_to_add| and |cookies_to_delete| must be empty,
// and none of the arguments can be null.
void ComputeCookieDiff(CookieList* old_cookies,
CookieList* new_cookies,
CookieList* cookies_to_add,
CookieList* cookies_to_delete);
// Run all cookie changed callbacks that are monitoring |cookie|. // Run all cookie changed callbacks that are monitoring |cookie|.
// |removed| is true if the cookie was deleted. // |removed| is true if the cookie was deleted.
void RunCookieChangedCallbacks(const CanonicalCookie& cookie, void RunCookieChangedCallbacks(const CanonicalCookie& cookie,
......
net/cookies/cookie_monster_unittest.cc
View file @ 0e84cea6
...@@ -2660,125 +2660,6 @@ TEST_F(CookieMonsterTest, SetAllCookies) { ...@@ -2660,125 +2660,6 @@ TEST_F(CookieMonsterTest, SetAllCookies) {
EXPECT_EQ("Z", it->Value()); EXPECT_EQ("Z", it->Value());
} }
TEST_F(CookieMonsterTest, ComputeCookieDiff) {
std::unique_ptr<CookieMonster> cm(new CookieMonster(nullptr, nullptr));
base::Time now = base::Time::Now();
base::Time creation_time = now - base::TimeDelta::FromSeconds(1);
std::unique_ptr<CanonicalCookie> cookie1(base::MakeUnique<CanonicalCookie>(
"A", "B", "." + http_www_foo_.url().host(), "/", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie2(base::MakeUnique<CanonicalCookie>(
"C", "D", "." + http_www_foo_.url().host(), "/", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie3(base::MakeUnique<CanonicalCookie>(
"E", "F", "." + http_www_foo_.url().host(), "/", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie4(base::MakeUnique<CanonicalCookie>(
"G", "H", "." + http_www_foo_.url().host(), "/", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie4_with_new_value(
base::MakeUnique<CanonicalCookie>(
"G", "iamnew", "." + http_www_foo_.url().host(), "/", creation_time,
base::Time(), base::Time(), false, false,
CookieSameSite::DEFAULT_MODE, COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie5(base::MakeUnique<CanonicalCookie>(
"I", "J", "." + http_www_foo_.url().host(), "/", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie5_with_new_creation_time(
base::MakeUnique<CanonicalCookie>(
"I", "J", "." + http_www_foo_.url().host(), "/", now, base::Time(),
base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie6(base::MakeUnique<CanonicalCookie>(
"K", "L", "." + http_www_foo_.url().host(), "/foo", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie6_with_new_path(
base::MakeUnique<CanonicalCookie>(
"K", "L", "." + http_www_foo_.url().host(), "/bar", creation_time,
base::Time(), base::Time(), false, false,
CookieSameSite::DEFAULT_MODE, COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie7(base::MakeUnique<CanonicalCookie>(
"M", "N", "." + http_www_foo_.url().host(), "/foo", creation_time,
base::Time(), base::Time(), false, false, CookieSameSite::DEFAULT_MODE,
COOKIE_PRIORITY_DEFAULT));
std::unique_ptr<CanonicalCookie> cookie7_with_new_path(
base::MakeUnique<CanonicalCookie>(
"M", "N", "." + http_www_foo_.url().host(), "/bar", creation_time,
base::Time(), base::Time(), false, false,
CookieSameSite::DEFAULT_MODE, COOKIE_PRIORITY_DEFAULT));
CookieList old_cookies;
old_cookies.push_back(*cookie1);
old_cookies.push_back(*cookie2);
old_cookies.push_back(*cookie4);
old_cookies.push_back(*cookie5);
old_cookies.push_back(*cookie6);
old_cookies.push_back(*cookie7);
CookieList new_cookies;
new_cookies.push_back(*cookie1);
new_cookies.push_back(*cookie3);
new_cookies.push_back(*cookie4_with_new_value);
new_cookies.push_back(*cookie5_with_new_creation_time);
new_cookies.push_back(*cookie6_with_new_path);
new_cookies.push_back(*cookie7);
new_cookies.push_back(*cookie7_with_new_path);
CookieList cookies_to_add;
CookieList cookies_to_delete;
cm->ComputeCookieDiff(&old_cookies, &new_cookies, &cookies_to_add,
&cookies_to_delete);
// |cookie1| has not changed.
EXPECT_FALSE(IsCookieInList(*cookie1, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie1, cookies_to_delete));
// |cookie2| has been deleted.
EXPECT_FALSE(IsCookieInList(*cookie2, cookies_to_add));
EXPECT_TRUE(IsCookieInList(*cookie2, cookies_to_delete));
// |cookie3| has been added.
EXPECT_TRUE(IsCookieInList(*cookie3, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie3, cookies_to_delete));
// |cookie4| has a new value: new cookie overrides the old one (which does not
// need to be explicitly removed).
EXPECT_FALSE(IsCookieInList(*cookie4, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie4, cookies_to_delete));
EXPECT_TRUE(IsCookieInList(*cookie4_with_new_value, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie4_with_new_value, cookies_to_delete));
// |cookie5| has a new creation time: new cookie overrides the old one (which
// does not need to be explicitly removed).
EXPECT_FALSE(IsCookieInList(*cookie5, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie5, cookies_to_delete));
EXPECT_TRUE(IsCookieInList(*cookie5_with_new_creation_time, cookies_to_add));
EXPECT_FALSE(
IsCookieInList(*cookie5_with_new_creation_time, cookies_to_delete));
// |cookie6| has a new path: the new cookie does not overrides the old one,
// which needs to be explicitly removed.
EXPECT_FALSE(IsCookieInList(*cookie6, cookies_to_add));
EXPECT_TRUE(IsCookieInList(*cookie6, cookies_to_delete));
EXPECT_TRUE(IsCookieInList(*cookie6_with_new_path, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie6_with_new_path, cookies_to_delete));
// |cookie7| is kept and |cookie7_with_new_path| is added as a new cookie.
EXPECT_FALSE(IsCookieInList(*cookie7, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie7, cookies_to_delete));
EXPECT_TRUE(IsCookieInList(*cookie7_with_new_path, cookies_to_add));
EXPECT_FALSE(IsCookieInList(*cookie7_with_new_path, cookies_to_delete));
}
// Check that DeleteAll does flush (as a sanity check that flush_count() // Check that DeleteAll does flush (as a sanity check that flush_count()
// works). // works).
TEST_F(CookieMonsterTest, DeleteAll) { TEST_F(CookieMonsterTest, DeleteAll) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment