Refactor chrome.cookies API to use JSON schema compiler.

chrome/browser/extensions/api/cookies/cookies_helpers.cc

Index: chrome/browser/extensions/api/cookies/cookies_helpers.cc
diff --git a/chrome/browser/extensions/api/cookies/cookies_helpers.cc b/chrome/browser/extensions/api/cookies/cookies_helpers.cc
index a139d5d1bde1c85ce5a8130d5bf7f45a00286a40..044ddf0b4e66e6380fa4029463674c6f0cbef031 100644
--- a/chrome/browser/extensions/api/cookies/cookies_helpers.cc
+++ b/chrome/browser/extensions/api/cookies/cookies_helpers.cc
@@ -6,7 +6,10 @@
 
 #include "chrome/browser/extensions/api/cookies/cookies_helpers.h"
 
+#include <vector>
+
 #include "base/logging.h"
+#include "base/memory/linked_ptr.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
@@ -16,12 +19,18 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/extensions/api/cookies.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "googleurl/src/gurl.h"
 #include "net/cookies/cookie_util.h"
 
+using extensions::api::cookies::Cookie;
+using extensions::api::cookies::CookieStore;
+
+namespace GetAll = extensions::api::cookies::GetAll;
+
 namespace extensions {
 
 namespace keys = cookies_api_constants;
@@ -51,43 +60,47 @@ const char* GetStoreIdFromProfile(Profile* profile) {
       kOffTheRecordProfileStoreId : kOriginalProfileStoreId;
 }
 
-DictionaryValue* CreateCookieValue(
+Cookie* CreateCookie(
     const net::CookieMonster::CanonicalCookie& cookie,
     const std::string& store_id) {
-  DictionaryValue* result = new DictionaryValue();
+  DictionaryValue dict;
 
   // A cookie is a raw byte sequence. By explicitly parsing it as UTF8, we
   // apply error correction, so the string can be safely passed to the
   // renderer.
-  result->SetString(keys::kNameKey, UTF8ToUTF16(cookie.Name()));
-  result->SetString(keys::kValueKey, UTF8ToUTF16(cookie.Value()));
-  result->SetString(keys::kDomainKey, cookie.Domain());
-  result->SetBoolean(keys::kHostOnlyKey,
-                     net::cookie_util::DomainIsHostOnly(cookie.Domain()));
+  dict.SetString(keys::kNameKey, UTF8ToUTF16(cookie.Name()));

[Aaron Boodman, 2012/07/09 17:34:49]

Is it possible to just populate new_cookie directly? Rather than populating a
JSON struct, then parsing that into the strongly typed object? Same elsewhere in
this file.

[mitchellwrosen, 2012/07/09 19:36:12]

Done for populating a Cookie, but I think it may be more convenient to simply
use CookieStore::Populate. Otherwise, I would have to populate a vector<int>
from a ListValue, essentially duplicating
json_schema_compiler::util::PopulateArrayFromList.

What do you think?

+  dict.SetString(keys::kValueKey, UTF8ToUTF16(cookie.Value()));
+  dict.SetString(keys::kDomainKey, cookie.Domain());
+  dict.SetBoolean(keys::kHostOnlyKey,
+                  net::cookie_util::DomainIsHostOnly(cookie.Domain()));
 
   // A non-UTF8 path is invalid, so we just replace it with an empty string.
-  result->SetString(keys::kPathKey,
-                    IsStringUTF8(cookie.Path()) ? cookie.Path() : "");
-  result->SetBoolean(keys::kSecureKey, cookie.IsSecure());
-  result->SetBoolean(keys::kHttpOnlyKey, cookie.IsHttpOnly());
-  result->SetBoolean(keys::kSessionKey, !cookie.DoesExpire());
-  if (cookie.DoesExpire()) {
-    result->SetDouble(keys::kExpirationDateKey,
-                      cookie.ExpiryDate().ToDoubleT());
-  }
-  result->SetString(keys::kStoreIdKey, store_id);
-
-  return result;
+  dict.SetString(keys::kPathKey,
+                 IsStringUTF8(cookie.Path()) ? cookie.Path() : "");
+  dict.SetBoolean(keys::kSecureKey, cookie.IsSecure());
+  dict.SetBoolean(keys::kHttpOnlyKey, cookie.IsHttpOnly());
+  dict.SetBoolean(keys::kSessionKey, !cookie.DoesExpire());
+  if (cookie.DoesExpire())
+    dict.SetDouble(keys::kExpirationDateKey, cookie.ExpiryDate().ToDoubleT());
+  dict.SetString(keys::kStoreIdKey, store_id);
+
+  Cookie* new_cookie = new Cookie();
+  bool rv = Cookie::Populate(dict, new_cookie);
+  CHECK(rv);
+  return new_cookie;
 }
 
-DictionaryValue* CreateCookieStoreValue(Profile* profile,
-                                        ListValue* tab_ids) {
+CookieStore* CreateCookieStore(Profile* profile, ListValue* tab_ids) {
   DCHECK(profile);
   DCHECK(tab_ids);
-  DictionaryValue* result = new DictionaryValue();
-  result->SetString(keys::kIdKey, GetStoreIdFromProfile(profile));
-  result->Set(keys::kTabIdsKey, tab_ids);
-  return result;
+  DictionaryValue dict;
+  dict.SetString(keys::kIdKey, GetStoreIdFromProfile(profile));
+  dict.Set(keys::kTabIdsKey, tab_ids);
+
+  CookieStore* cookie_store = new CookieStore();
+  bool rv = CookieStore::Populate(dict, cookie_store);
+  CHECK(rv);
+  return cookie_store;
 }
 
 void GetCookieListFromStore(
@@ -113,12 +126,11 @@ GURL GetURLFromCanonicalCookie(
   return GURL(scheme + content::kStandardSchemeSeparator + host + "/");
 }
 
-void AppendMatchingCookiesToList(
+void AppendMatchingCookiesToVector(
     const net::CookieList& all_cookies,
-    const std::string& store_id,
-    const GURL& url, const DictionaryValue* details,
+    const GURL& url, const GetAll::Params::Details* details,
     const Extension* extension,
-    ListValue* match_list) {
+    std::vector<linked_ptr<Cookie> >* match_vector) {
   net::CookieList::const_iterator it;
   for (it = all_cookies.begin(); it != all_cookies.end(); ++it) {
     // Ignore any cookie whose domain doesn't match the extension's
@@ -128,8 +140,10 @@ void AppendMatchingCookiesToList(
       continue;
     // Filter the cookie using the match filter.
     cookies_helpers::MatchFilter filter(details);
-    if (filter.MatchesCookie(*it))
-      match_list->Append(CreateCookieValue(*it, store_id));
+    if (filter.MatchesCookie(*it)) {
+      linked_ptr<Cookie> cookie(CreateCookie(*it, *details->store_id));

[Aaron Boodman, 2012/07/09 17:34:49]

Since you don't need the local, you could use make_linked_ptr() to shorten this
slightly.

[mitchellwrosen, 2012/07/09 19:36:12]

Done.

+      match_vector->push_back(cookie);
+    }
   }
 }
 
@@ -144,46 +158,32 @@ void AppendToTabIdList(Browser* browser, ListValue* tab_ids) {
   }
 }
 
-MatchFilter::MatchFilter(const DictionaryValue* details)
+MatchFilter::MatchFilter(const GetAll::Params::Details* details)
     : details_(details) {
   DCHECK(details_);
 }
 
 bool MatchFilter::MatchesCookie(
     const net::CookieMonster::CanonicalCookie& cookie) {
-  return MatchesString(keys::kNameKey, cookie.Name()) &&
-         MatchesDomain(cookie.Domain()) &&
-         MatchesString(keys::kPathKey, cookie.Path()) &&
-         MatchesBoolean(keys::kSecureKey, cookie.IsSecure()) &&
-         MatchesBoolean(keys::kSessionKey, !cookie.DoesExpire());
-}
-
-bool MatchFilter::MatchesString(const char* key, const std::string& value) {
-  if (!details_->HasKey(key))
-    return true;
-  std::string filter_value;
-  return (details_->GetString(key, &filter_value) &&
-          value == filter_value);
-}
-
-bool MatchFilter::MatchesBoolean(const char* key, bool value) {
-  if (!details_->HasKey(key))
-    return true;
-  bool filter_value = false;
-  return (details_->GetBoolean(key, &filter_value) &&
-          value == filter_value);
+  bool matchesName = !details_->name.get() || *details_->name == cookie.Name();

[Aaron Boodman, 2012/07/09 17:34:49]

Consider restructuring this logic like:

if (details->name.get() && details->name != cookie.Name())
  return false;

if (!MatchesDomain(...))
  return false;

...

return true;

[mitchellwrosen, 2012/07/09 19:36:12]

Done.

+  bool matchesDomain = MatchesDomain(cookie.Domain());
+  bool matchesPath = !details_->path.get() || *details_->path == cookie.Path();
+  bool matchesSecure = !details_->secure.get() ||
+      *details_->secure == cookie.IsSecure();
+  bool matchesSession = !details_->session.get() ||
+      *details_->session == !cookie.DoesExpire();
+
+  return matchesName && matchesDomain && matchesPath && matchesSecure &&
+      matchesSession;
 }
 
 bool MatchFilter::MatchesDomain(const std::string& domain) {
-  if (!details_->HasKey(keys::kDomainKey))
+  if (!details_->domain.get())
     return true;
 
-  std::string filter_value;
-  if (!details_->GetString(keys::kDomainKey, &filter_value))
-    return false;
   // Add a leading '.' character to the filter domain if it doesn't exist.
-  if (net::cookie_util::DomainIsHostOnly(filter_value))
-    filter_value.insert(0, ".");
+  if (net::cookie_util::DomainIsHostOnly(*details_->domain))
+    details_->domain->insert(0, ".");
 
   std::string sub_domain(domain);
   // Strip any leading '.' character from the input cookie domain.
@@ -192,8 +192,8 @@ bool MatchFilter::MatchesDomain(const std::string& domain) {
 
   // Now check whether the domain argument is a subdomain of the filter domain.
   for (sub_domain.insert(0, ".");
-       sub_domain.length() >= filter_value.length();) {
-    if (sub_domain == filter_value)
+       sub_domain.length() >= details_->domain->length();) {
+    if (sub_domain == *details_->domain)
       return true;
     const size_t next_dot = sub_domain.find('.', 1);  // Skip over leading dot.
     sub_domain.erase(0, next_dot);