From 4d1e2230445b8ca8200db50bc2aad11b0ab8ed41 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sat, 12 Dec 2015 15:33:51 +0800 Subject: [PATCH] Cleanup the cookies code --- atom/browser/api/atom_api_cookies.cc | 358 ++++++++++++----------------------- atom/browser/api/atom_api_cookies.h | 45 ++--- spec/api-session-spec.coffee | 5 +- 3 files changed, 138 insertions(+), 270 deletions(-) diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc index 3b1bd49..6323e51 100644 --- a/atom/browser/api/atom_api_cookies.cc +++ b/atom/browser/api/atom_api_cookies.cc @@ -7,7 +7,6 @@ #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/native_mate_converters/value_converter.h" -#include "base/bind.h" #include "base/time/time.h" #include "base/values.h" #include "content/public/browser/browser_context.h" @@ -20,279 +19,157 @@ #include "net/url_request/url_request_context.h" #include "net/url_request/url_request_context_getter.h" -using atom::api::Cookies; using content::BrowserThread; -namespace { - -bool GetCookieListFromStore( - net::CookieStore* cookie_store, - const std::string& url, - const net::CookieMonster::GetCookieListCallback& callback) { - DCHECK(cookie_store); - GURL gurl(url); - net::CookieMonster* monster = cookie_store->GetCookieMonster(); - // Empty url will match all url cookies. - if (url.empty()) { - monster->GetAllCookiesAsync(callback); - return true; - } - - if (!gurl.is_valid()) - return false; - - monster->GetAllCookiesForURLAsync(gurl, callback); - return true; -} - -void RunGetCookiesCallbackOnUIThread(v8::Isolate* isolate, - const std::string& error_message, - const net::CookieList& cookie_list, - const Cookies::CookiesCallback& callback) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); +namespace mate { - if (!error_message.empty()) { - v8::Local error = mate::ConvertToV8(isolate, error_message); - callback.Run(error, v8::Null(isolate)); - return; +template<> +struct Converter { + static v8::Local ToV8(v8::Isolate* isolate, + atom::api::Cookies::Error val) { + if (val == atom::api::Cookies::SUCCESS) + return v8::Null(isolate); + else + return v8::Exception::Error(StringToV8(isolate, "failed")); } - callback.Run(v8::Null(isolate), mate::ConvertToV8(isolate, cookie_list)); -} - -void RunRemoveCookiesCallbackOnUIThread( - v8::Isolate* isolate, - const std::string& error_message, - const Cookies::CookiesCallback& callback) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); +}; - if (!error_message.empty()) { - v8::Local error = mate::ConvertToV8(isolate, error_message); - callback.Run(error, v8::Null(isolate)); - return; +template<> +struct Converter { + static v8::Local ToV8(v8::Isolate* isolate, + const net::CanonicalCookie& val) { + mate::Dictionary dict(isolate, v8::Object::New(isolate)); + dict.Set("name", val.Name()); + dict.Set("value", val.Value()); + dict.Set("domain", val.Domain()); + dict.Set("hostOnly", net::cookie_util::DomainIsHostOnly(val.Domain())); + dict.Set("path", val.Path()); + dict.Set("secure", val.IsSecure()); + dict.Set("httpOnly", val.IsHttpOnly()); + dict.Set("session", !val.IsPersistent()); + if (!val.IsPersistent()) + dict.Set("expirationDate", val.ExpiryDate().ToDoubleT()); + return dict.GetHandle(); } +}; - callback.Run(v8::Null(isolate), v8::Null(isolate)); -} - -void RunSetCookiesCallbackOnUIThread(v8::Isolate* isolate, - const std::string& error_message, - bool set_success, - const Cookies::CookiesCallback& callback) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); +} // namespace mate - if (!error_message.empty()) { - v8::Local error = mate::ConvertToV8(isolate, error_message); - callback.Run(error, v8::Null(isolate)); - return; - } - if (!set_success) { - v8::Local error = mate::ConvertToV8( - isolate, "Failed to set cookies"); - callback.Run(error, v8::Null(isolate)); - } +namespace atom { - callback.Run(v8::Null(isolate), v8::Null(isolate)); -} +namespace api { -bool MatchesDomain(const base::DictionaryValue* filter, - const std::string& cookie_domain) { - std::string filter_domain; - if (!filter->GetString("domain", &filter_domain)) - return true; +namespace { +// Returns whether |domain| matches |filter|. +bool MatchesDomain(std::string filter, const std::string& domain) { // Add a leading '.' character to the filter domain if it doesn't exist. - if (net::cookie_util::DomainIsHostOnly(filter_domain)) - filter_domain.insert(0, "."); + if (net::cookie_util::DomainIsHostOnly(filter)) + filter.insert(0, "."); - std::string sub_domain(cookie_domain); + std::string sub_domain(domain); // Strip any leading '.' character from the input cookie domain. if (!net::cookie_util::DomainIsHostOnly(sub_domain)) sub_domain = sub_domain.substr(1); // Now check whether the domain argument is a subdomain of the filter domain. - for (sub_domain.insert(0, "."); - sub_domain.length() >= filter_domain.length();) { - if (sub_domain == filter_domain) { + for (sub_domain.insert(0, "."); sub_domain.length() >= filter.length();) { + if (sub_domain == filter) return true; - } const size_t next_dot = sub_domain.find('.', 1); // Skip over leading dot. sub_domain.erase(0, next_dot); } return false; } +// Returns whether |cookie| matches |filter|. bool MatchesCookie(const base::DictionaryValue* filter, const net::CanonicalCookie& cookie) { - std::string name, domain, path; - bool is_secure, session; - if (filter->GetString("name", &name) && name != cookie.Name()) + std::string str; + bool b; + if (filter->GetString("name", &str) && str != cookie.Name()) return false; - if (filter->GetString("path", &path) && path != cookie.Path()) + if (filter->GetString("path", &str) && str != cookie.Path()) return false; - if (!MatchesDomain(filter, cookie.Domain())) + if (filter->GetString("domain", &str) && !MatchesDomain(str, cookie.Domain())) return false; - if (filter->GetBoolean("secure", &is_secure) && - is_secure != cookie.IsSecure()) + if (filter->GetBoolean("secure", &b) && b != cookie.IsSecure()) return false; - if (filter->GetBoolean("session", &session) && - session != cookie.IsPersistent()) + if (filter->GetBoolean("session", &b) && b != !cookie.IsPersistent()) return false; return true; } -} // namespace - -namespace mate { - -template<> -struct Converter { - static v8::Local ToV8(v8::Isolate* isolate, - const net::CanonicalCookie& val) { - mate::Dictionary dict(isolate, v8::Object::New(isolate)); - dict.Set("name", val.Name()); - dict.Set("value", val.Value()); - dict.Set("domain", val.Domain()); - dict.Set("host_only", net::cookie_util::DomainIsHostOnly(val.Domain())); - dict.Set("path", val.Path()); - dict.Set("secure", val.IsSecure()); - dict.Set("http_only", val.IsHttpOnly()); - dict.Set("session", val.IsPersistent()); - if (!val.IsPersistent()) - dict.Set("expirationDate", val.ExpiryDate().ToDoubleT()); - return dict.GetHandle(); - } -}; - -} // namespace mate - -namespace atom { - -namespace api { - -Cookies::Cookies(content::BrowserContext* browser_context) - : request_context_getter_(browser_context->GetRequestContext()) { +// Helper to returns the CookieStore. +inline net::CookieStore* GetCookieStore( + scoped_refptr getter) { + return getter->GetURLRequestContext()->cookie_store(); } -Cookies::~Cookies() { +// Run |callback| on UI thread. +void RunCallbackInUI(const base::Closure& callback) { + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback); } -void Cookies::Get(const base::DictionaryValue& options, - const CookiesCallback& callback) { - scoped_ptr filter( - options.DeepCopyWithoutEmptyChildren()); - - content::BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&Cookies::GetCookiesOnIOThread, base::Unretained(this), - Passed(&filter), callback)); -} - -void Cookies::GetCookiesOnIOThread(scoped_ptr filter, - const CookiesCallback& callback) { - std::string url; - filter->GetString("url", &url); - if (!GetCookieListFromStore(GetCookieStore(), url, - base::Bind(&Cookies::OnGetCookies, base::Unretained(this), - Passed(&filter), callback))) { - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, - base::Bind(&RunGetCookiesCallbackOnUIThread, isolate(), - "URL is not valid", net::CookieList(), callback)); - } -} - -void Cookies::OnGetCookies(scoped_ptr filter, - const CookiesCallback& callback, - const net::CookieList& cookie_list) { +// Remove cookies from |list| not matching |filter|, and pass it to |callback|. +void FilterCookies(scoped_ptr filter, + const Cookies::GetCallback& callback, + const net::CookieList& list) { net::CookieList result; - for (const auto& cookie : cookie_list) { + for (const auto& cookie : list) { if (MatchesCookie(filter.get(), cookie)) result.push_back(cookie); } - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind( - &RunGetCookiesCallbackOnUIThread, isolate(), "", result, callback)); -} - -void Cookies::Remove(const mate::Dictionary& details, - const CookiesCallback& callback) { - GURL url; - std::string name; - std::string error_message; - if (!details.Get("url", &url) || !details.Get("name", &name)) { - error_message = "Details(url, name) of removing cookie are required."; - } - if (error_message.empty() && !url.is_valid()) { - error_message = "URL is not valid."; - } - if (!error_message.empty()) { - RunRemoveCookiesCallbackOnUIThread(isolate(), error_message, callback); - return; - } - content::BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&Cookies::RemoveCookiesOnIOThread, base::Unretained(this), - url, name, callback)); -} - -void Cookies::RemoveCookiesOnIOThread(const GURL& url, const std::string& name, - const CookiesCallback& callback) { - GetCookieStore()->DeleteCookieAsync(url, name, - base::Bind(&Cookies::OnRemoveCookies, base::Unretained(this), callback)); -} - -void Cookies::OnRemoveCookies(const CookiesCallback& callback) { - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, - base::Bind(&RunRemoveCookiesCallbackOnUIThread, isolate(), "", callback)); + RunCallbackInUI(base::Bind(callback, Cookies::SUCCESS, result)); } -void Cookies::Set(const base::DictionaryValue& options, - const CookiesCallback& callback) { +// Receives cookies matching |filter| in IO thread. +void GetCookiesOnIO(scoped_refptr getter, + scoped_ptr filter, + const Cookies::GetCallback& callback) { std::string url; - std::string error_message; - if (!options.GetString("url", &url)) { - error_message = "The url field is required."; - } + filter->GetString("url", &url); - GURL gurl(url); - if (error_message.empty() && !gurl.is_valid()) { - error_message = "URL is not valid."; - } + auto filtered_callback = + base::Bind(FilterCookies, base::Passed(&filter), callback); - if (!error_message.empty()) { - RunSetCookiesCallbackOnUIThread(isolate(), error_message, false, callback); - return; - } - - scoped_ptr details( - options.DeepCopyWithoutEmptyChildren()); + net::CookieMonster* monster = GetCookieStore(getter)->GetCookieMonster(); + // Empty url will match all url cookies. + if (url.empty()) + monster->GetAllCookiesAsync(filtered_callback); + else + monster->GetAllCookiesForURLAsync(GURL(url), filtered_callback); +} - content::BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&Cookies::SetCookiesOnIOThread, base::Unretained(this), - Passed(&details), gurl, callback)); +// Removes cookie with |url| and |name| in IO thread. +void RemoveCookieOnIOThread(scoped_refptr getter, + const GURL& url, const std::string& name, + const base::Closure& callback) { + GetCookieStore(getter)->DeleteCookieAsync( + url, name, base::Bind(RunCallbackInUI, callback)); } -void Cookies::SetCookiesOnIOThread(scoped_ptr details, - const GURL& url, - const CookiesCallback& callback) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); +// Callback of SetCookie. +void OnSetCookie(const Cookies::SetCallback& callback, bool success) { + RunCallbackInUI( + base::Bind(callback, success ? Cookies::SUCCESS : Cookies::FAILED)); +} - std::string name, value, domain, path; +// Sets cookie with |details| in IO thread. +void SetCookieOnIO(scoped_refptr getter, + scoped_ptr details, + const Cookies::SetCallback& callback) { + std::string url, name, value, domain, path; bool secure = false; bool http_only = false; double expiration_date; - + details->GetString("url", &url); details->GetString("name", &name); details->GetString("value", &value); details->GetString("domain", &domain); details->GetString("path", &path); details->GetBoolean("secure", &secure); - details->GetBoolean("http_only", &http_only); + details->GetBoolean("httpOnly", &http_only); base::Time expiration_time; if (details->GetDouble("expirationDate", &expiration_date)) { @@ -301,29 +178,44 @@ void Cookies::SetCookiesOnIOThread(scoped_ptr details, base::Time::FromDoubleT(expiration_date); } - GetCookieStore()->GetCookieMonster()->SetCookieWithDetailsAsync( - url, - name, - value, - domain, - path, - expiration_time, - secure, - http_only, - false, - net::COOKIE_PRIORITY_DEFAULT, - base::Bind(&Cookies::OnSetCookies, base::Unretained(this), callback)); + GetCookieStore(getter)->GetCookieMonster()->SetCookieWithDetailsAsync( + GURL(url), name, value, domain, path, expiration_time, secure, http_only, + false, net::COOKIE_PRIORITY_DEFAULT, base::Bind(OnSetCookie, callback)); +} + +} // namespace + +Cookies::Cookies(content::BrowserContext* browser_context) + : request_context_getter_(browser_context->GetRequestContext()) { +} + +Cookies::~Cookies() { +} + +void Cookies::Get(const base::DictionaryValue& filter, + const GetCallback& callback) { + scoped_ptr copied(filter.CreateDeepCopy()); + auto getter = make_scoped_refptr(request_context_getter_); + content::BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(GetCookiesOnIO, getter, Passed(&copied), callback)); } -void Cookies::OnSetCookies(const CookiesCallback& callback, - bool set_success) { - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, - base::Bind(&RunSetCookiesCallbackOnUIThread, isolate(), "", set_success, - callback)); +void Cookies::Remove(const GURL& url, const std::string& name, + const base::Closure& callback) { + auto getter = make_scoped_refptr(request_context_getter_); + content::BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(RemoveCookieOnIOThread, getter, url, name, callback)); } -net::CookieStore* Cookies::GetCookieStore() { - return request_context_getter_->GetURLRequestContext()->cookie_store(); +void Cookies::Set(const base::DictionaryValue& details, + const SetCallback& callback) { + scoped_ptr copied(details.CreateDeepCopy()); + auto getter = make_scoped_refptr(request_context_getter_); + content::BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(SetCookieOnIO, getter, Passed(&copied), callback)); } // static diff --git a/atom/browser/api/atom_api_cookies.h b/atom/browser/api/atom_api_cookies.h index 5afa1bd..302fd1b 100644 --- a/atom/browser/api/atom_api_cookies.h +++ b/atom/browser/api/atom_api_cookies.h @@ -20,12 +20,7 @@ namespace content { class BrowserContext; } -namespace mate { -class Dictionary; -} - namespace net { -class CookieStore; class URLRequestContextGetter; } @@ -35,9 +30,13 @@ namespace api { class Cookies : public mate::TrackableObject { public: - // node.js style callback function(error, result) - typedef base::Callback, v8::Local)> - CookiesCallback; + enum Error { + SUCCESS, + FAILED, + }; + + using GetCallback = base::Callback; + using SetCallback = base::Callback; static mate::Handle Create(v8::Isolate* isolate, content::BrowserContext* browser_context); @@ -50,34 +49,12 @@ class Cookies : public mate::TrackableObject { explicit Cookies(content::BrowserContext* browser_context); ~Cookies(); - void Get(const base::DictionaryValue& options, - const CookiesCallback& callback); - void Remove(const mate::Dictionary& details, - const CookiesCallback& callback); - void Set(const base::DictionaryValue& details, - const CookiesCallback& callback); - - void GetCookiesOnIOThread(scoped_ptr filter, - const CookiesCallback& callback); - void OnGetCookies(scoped_ptr filter, - const CookiesCallback& callback, - const net::CookieList& cookie_list); - - void RemoveCookiesOnIOThread(const GURL& url, - const std::string& name, - const CookiesCallback& callback); - void OnRemoveCookies(const CookiesCallback& callback); - - void SetCookiesOnIOThread(scoped_ptr details, - const GURL& url, - const CookiesCallback& callback); - void OnSetCookies(const CookiesCallback& callback, - bool set_success); + void Get(const base::DictionaryValue& filter, const GetCallback& callback); + void Remove(const GURL& url, const std::string& name, + const base::Closure& callback); + void Set(const base::DictionaryValue& details, const SetCallback& callback); private: - // Must be called on IO thread. - net::CookieStore* GetCookieStore(); - net::URLRequestContextGetter* request_context_getter_; DISALLOW_COPY_AND_ASSIGN(Cookies); diff --git a/spec/api-session-spec.coffee b/spec/api-session-spec.coffee index 98e47e3..03c62c1 100644 --- a/spec/api-session-spec.coffee +++ b/spec/api-session-spec.coffee @@ -49,12 +49,11 @@ describe 'session module', -> it 'should remove cookies', (done) -> session.defaultSession.cookies.set {url: url, name: '2', value: '2'}, (error) -> return done(error) if error - session.defaultSession.cookies.remove {url: url, name: '2'}, (error) -> - return done(error) if error + session.defaultSession.cookies.remove url, '2', -> session.defaultSession.cookies.get {url: url}, (error, list) -> return done(error) if error for cookie in list when cookie.name is '2' - return done('Cookie not deleted') + return done('Cookie not deleted') done() describe 'session.clearStorageData(options)', -> -- 2.7.4