Cleanup the cookies code

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,139 +19,21 @@
 #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);
-
-  if (!error_message.empty()) {
-    v8::Local<v8::Value> error = mate::ConvertToV8(isolate, error_message);
-    callback.Run(error, v8::Null(isolate));
-    return;
-  }
-  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<v8::Value> error = mate::ConvertToV8(isolate, error_message);
-    callback.Run(error, v8::Null(isolate));
-    return;
-  }
-
-  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);
-
-  if (!error_message.empty()) {
-    v8::Local<v8::Value> error = mate::ConvertToV8(isolate, error_message);
-    callback.Run(error, v8::Null(isolate));
-    return;
-  }
-  if (!set_success) {
-    v8::Local<v8::Value> error = mate::ConvertToV8(
-        isolate, "Failed to set cookies");
-    callback.Run(error, v8::Null(isolate));
-  }
-
-  callback.Run(v8::Null(isolate), v8::Null(isolate));
-}
-
-bool MatchesDomain(const base::DictionaryValue* filter,
-                   const std::string& cookie_domain) {
-  std::string filter_domain;
-  if (!filter->GetString("domain", &filter_domain))
-    return true;
-
-  // Add a leading '.' character to the filter domain if it doesn't exist.
-  if (net::cookie_util::DomainIsHostOnly(filter_domain))
-    filter_domain.insert(0, ".");
-
-  std::string sub_domain(cookie_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) {
-      return true;
-    }
-    const size_t next_dot = sub_domain.find('.', 1);  // Skip over leading dot.
-    sub_domain.erase(0, next_dot);
-  }
-  return false;
-}
-
-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())
-    return false;
-  if (filter->GetString("path", &path) && path != cookie.Path())
-    return false;
-  if (!MatchesDomain(filter, cookie.Domain()))
-    return false;
-  if (filter->GetBoolean("secure", &is_secure) &&
-      is_secure != cookie.IsSecure())
-    return false;
-  if (filter->GetBoolean("session", &session) &&
-      session != cookie.IsPersistent())
-    return false;
-  return true;
-}
-
-}  // namespace
-
 namespace mate {
 
+template<>
+struct Converter<atom::api::Cookies::Error> {
+  static v8::Local<v8::Value> 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"));
+  }
+};
+
 template<>
 struct Converter<net::CanonicalCookie> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
@@ -161,11 +42,11 @@ struct Converter<net::CanonicalCookie> {
     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("hostOnly", net::cookie_util::DomainIsHostOnly(val.Domain()));
     dict.Set("path", val.Path());
     dict.Set("secure", val.IsSecure());
-    dict.Set("http_only", val.IsHttpOnly());
+    dict.Set("httpOnly", val.IsHttpOnly());
-    dict.Set("session", val.IsPersistent());
+    dict.Set("session", !val.IsPersistent());
     if (!val.IsPersistent())
       dict.Set("expirationDate", val.ExpiryDate().ToDoubleT());
     return dict.GetHandle();
@@ -178,121 +59,117 @@ namespace atom {
 
 namespace api {
 
-Cookies::Cookies(content::BrowserContext* browser_context)
+namespace {
-    : request_context_getter_(browser_context->GetRequestContext()) {
-}
 
-Cookies::~Cookies() {
+// 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))
+    filter.insert(0, ".");
 
-void Cookies::Get(const base::DictionaryValue& options,
+  std::string sub_domain(domain);
-                  const CookiesCallback& callback) {
+  // Strip any leading '.' character from the input cookie domain.
-  scoped_ptr<base::DictionaryValue> filter(
+  if (!net::cookie_util::DomainIsHostOnly(sub_domain))
-      options.DeepCopyWithoutEmptyChildren());
+    sub_domain = sub_domain.substr(1);
 
-  content::BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+  // Now check whether the domain argument is a subdomain of the filter domain.
-      base::Bind(&Cookies::GetCookiesOnIOThread, base::Unretained(this),
+  for (sub_domain.insert(0, "."); sub_domain.length() >= filter.length();) {
-          Passed(&filter), callback));
+    if (sub_domain == filter)
-}
+      return true;
-
+    const size_t next_dot = sub_domain.find('.', 1);  // Skip over leading dot.
-void Cookies::GetCookiesOnIOThread(scoped_ptr<base::DictionaryValue> filter,
+    sub_domain.erase(0, next_dot);
-                                   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));
   }
+  return false;
 }
 
-void Cookies::OnGetCookies(scoped_ptr<base::DictionaryValue> filter,
+// Returns whether |cookie| matches |filter|.
-                           const CookiesCallback& callback,
+bool MatchesCookie(const base::DictionaryValue* filter,
-                           const net::CookieList& cookie_list) {
+                   const net::CanonicalCookie& cookie) {
+  std::string str;
+  bool b;
+  if (filter->GetString("name", &str) && str != cookie.Name())
+    return false;
+  if (filter->GetString("path", &str) && str != cookie.Path())
+    return false;
+  if (filter->GetString("domain", &str) && !MatchesDomain(str, cookie.Domain()))
+    return false;
+  if (filter->GetBoolean("secure", &b) && b != cookie.IsSecure())
+    return false;
+  if (filter->GetBoolean("session", &b) && b != !cookie.IsPersistent())
+    return false;
+  return true;
+}
+
+// Helper to returns the CookieStore.
+inline net::CookieStore* GetCookieStore(
+    scoped_refptr<net::URLRequestContextGetter> getter) {
+  return getter->GetURLRequestContext()->cookie_store();
+}
+
+// Run |callback| on UI thread.
+void RunCallbackInUI(const base::Closure& callback) {
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+}
+
+// Remove cookies from |list| not matching |filter|, and pass it to |callback|.
+void FilterCookies(scoped_ptr<base::DictionaryValue> 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(
+  RunCallbackInUI(base::Bind(callback, Cookies::SUCCESS, result));
-      &RunGetCookiesCallbackOnUIThread, isolate(), "", result, callback));
 }
 
-void Cookies::Remove(const mate::Dictionary& details,
+// Receives cookies matching |filter| in IO thread.
-                     const CookiesCallback& callback) {
+void GetCookiesOnIO(scoped_refptr<net::URLRequestContextGetter> getter,
-  GURL url;
+                    scoped_ptr<base::DictionaryValue> filter,
-  std::string name;
+                    const Cookies::GetCallback& callback) {
-  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));
-}
-
-void Cookies::Set(const base::DictionaryValue& options,
-                  const CookiesCallback& callback) {
   std::string url;
-  std::string error_message;
+  filter->GetString("url", &url);
-  if (!options.GetString("url", &url)) {
-    error_message = "The url field is required.";
-  }
 
-  GURL gurl(url);
+  auto filtered_callback =
-  if (error_message.empty() && !gurl.is_valid()) {
+      base::Bind(FilterCookies, base::Passed(&filter), callback);
-    error_message = "URL is not valid.";
-  }
 
-  if (!error_message.empty()) {
+  net::CookieMonster* monster = GetCookieStore(getter)->GetCookieMonster();
-    RunSetCookiesCallbackOnUIThread(isolate(), error_message, false, callback);
+  // Empty url will match all url cookies.
-    return;
+  if (url.empty())
-  }
+    monster->GetAllCookiesAsync(filtered_callback);
-
+  else
-  scoped_ptr<base::DictionaryValue> details(
+    monster->GetAllCookiesForURLAsync(GURL(url), filtered_callback);
-      options.DeepCopyWithoutEmptyChildren());
-
-  content::BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-      base::Bind(&Cookies::SetCookiesOnIOThread, base::Unretained(this),
-          Passed(&details), gurl, callback));
 }
 
-void Cookies::SetCookiesOnIOThread(scoped_ptr<base::DictionaryValue> details,
+// Removes cookie with |url| and |name| in IO thread.
-                                   const GURL& url,
+void RemoveCookieOnIOThread(scoped_refptr<net::URLRequestContextGetter> getter,
-                                   const CookiesCallback& callback) {
+                            const GURL& url, const std::string& name,
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+                            const base::Closure& callback) {
+  GetCookieStore(getter)->DeleteCookieAsync(
+      url, name, base::Bind(RunCallbackInUI, callback));
+}
 
-  std::string name, value, domain, path;
+// Callback of SetCookie.
+void OnSetCookie(const Cookies::SetCallback& callback, bool success) {
+  RunCallbackInUI(
+      base::Bind(callback, success ? Cookies::SUCCESS : Cookies::FAILED));
+}
+
+// Sets cookie with |details| in IO thread.
+void SetCookieOnIO(scoped_refptr<net::URLRequestContextGetter> getter,
+                   scoped_ptr<base::DictionaryValue> 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<base::DictionaryValue> details,
         base::Time::FromDoubleT(expiration_date);
   }
 
-  GetCookieStore()->GetCookieMonster()->SetCookieWithDetailsAsync(
+  GetCookieStore(getter)->GetCookieMonster()->SetCookieWithDetailsAsync(
-      url,
+      GURL(url), name, value, domain, path, expiration_time, secure, http_only,
-      name,
+      false, net::COOKIE_PRIORITY_DEFAULT, base::Bind(OnSetCookie, callback));
-      value,
-      domain,
-      path,
-      expiration_time,
-      secure,
-      http_only,
-      false,
-      net::COOKIE_PRIORITY_DEFAULT,
-      base::Bind(&Cookies::OnSetCookies, base::Unretained(this), callback));
 }
 
-void Cookies::OnSetCookies(const CookiesCallback& callback,
+}  // namespace
-                           bool set_success) {
+
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+Cookies::Cookies(content::BrowserContext* browser_context)
-      base::Bind(&RunSetCookiesCallbackOnUIThread, isolate(), "", set_success,
+    : request_context_getter_(browser_context->GetRequestContext()) {
-                 callback));
 }
 
-net::CookieStore* Cookies::GetCookieStore() {
+Cookies::~Cookies() {
-  return request_context_getter_->GetURLRequestContext()->cookie_store();
+}
+
+void Cookies::Get(const base::DictionaryValue& filter,
+                  const GetCallback& callback) {
+  scoped_ptr<base::DictionaryValue> 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::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));
+}
+
+void Cookies::Set(const base::DictionaryValue& details,
+                  const SetCallback& callback) {
+  scoped_ptr<base::DictionaryValue> 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

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<Cookies> {
  public:
-  // node.js style callback function(error, result)
+  enum Error {
-  typedef base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>)>
+    SUCCESS,
-      CookiesCallback;
+    FAILED,
+  };
+
+  using GetCallback = base::Callback<void(Error, const net::CookieList&)>;
+  using SetCallback = base::Callback<void(Error)>;
 
   static mate::Handle<Cookies> Create(v8::Isolate* isolate,
                                       content::BrowserContext* browser_context);
@@ -50,34 +49,12 @@ class Cookies : public mate::TrackableObject<Cookies> {
   explicit Cookies(content::BrowserContext* browser_context);
   ~Cookies();
 
-  void Get(const base::DictionaryValue& options,
+  void Get(const base::DictionaryValue& filter, const GetCallback& callback);
-           const CookiesCallback& callback);
+  void Remove(const GURL& url, const std::string& name,
-  void Remove(const mate::Dictionary& details,
+              const base::Closure& callback);
-              const CookiesCallback& callback);
+  void Set(const base::DictionaryValue& details, const SetCallback& callback);
-  void Set(const base::DictionaryValue& details,
-           const CookiesCallback& callback);
-
-  void GetCookiesOnIOThread(scoped_ptr<base::DictionaryValue> filter,
-                            const CookiesCallback& callback);
-  void OnGetCookies(scoped_ptr<base::DictionaryValue> 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<base::DictionaryValue> details,
-                            const GURL& url,
-                            const CookiesCallback& callback);
-  void OnSetCookies(const CookiesCallback& callback,
-                    bool set_success);
 
  private:
-  // Must be called on IO thread.
-  net::CookieStore* GetCookieStore();
-
   net::URLRequestContextGetter* request_context_getter_;
 
   DISALLOW_COPY_AND_ASSIGN(Cookies);

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) ->
+      session.defaultSession.cookies.remove url, '2', ->
-        return done(error) if error
         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)', ->