Refactor chrome.cookies API to use JSON schema compiler.

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

Index: chrome/browser/extensions/api/cookies/cookies_api.cc
diff --git a/chrome/browser/extensions/api/cookies/cookies_api.cc b/chrome/browser/extensions/api/cookies/cookies_api.cc
index cb17f3096c346c74a6b0b309e59d40a4fd076692..3d352721308779f240d8e2c20da29df68077baa3 100644
--- a/chrome/browser/extensions/api/cookies/cookies_api.cc
+++ b/chrome/browser/extensions/api/cookies/cookies_api.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/api/cookies.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_error_utils.h"
 #include "content/public/browser/browser_thread.h"
@@ -25,6 +26,13 @@
 #include "net/url_request/url_request_context_getter.h"
 
 using content::BrowserThread;
+using extensions::api::cookies::CookieStore;
+
+namespace Get = extensions::api::cookies::Get;
+namespace GetAll = extensions::api::cookies::GetAll;
+namespace GetAllCookieStores = extensions::api::cookies::GetAllCookieStores;
+namespace Remove = extensions::api::cookies::Remove;
+namespace Set = extensions::api::cookies::Set;
 
 namespace extensions {
 namespace keys = cookies_api_constants;
@@ -119,12 +127,8 @@ void ExtensionCookiesEventRouter::DispatchEvent(Profile* profile,
   }
 }
 
-bool CookiesFunction::ParseUrl(const DictionaryValue* details, GURL* url,
+bool CookiesFunction::ParseUrl(std::string url_string, GURL* url,
                                bool check_host_permissions) {
-  DCHECK(details && url);
-  std::string url_string;
-  // Get the URL string or return false.
-  EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kUrlKey, &url_string));
   *url = GURL(url_string);
   if (!url->is_valid()) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(
@@ -141,23 +145,17 @@ bool CookiesFunction::ParseUrl(const DictionaryValue* details, GURL* url,
   return true;
 }
 
-bool CookiesFunction::ParseStoreContext(const DictionaryValue* details,
+bool CookiesFunction::ParseStoreContext(std::string* in_store_id,
                                         net::URLRequestContextGetter** context,
-                                        std::string* store_id) {
-  DCHECK(details && (context || store_id));
+                                        std::string* out_store_id) {
+  DCHECK((context || out_store_id));
   Profile* store_profile = NULL;
-  if (details->HasKey(keys::kStoreIdKey)) {
-    // The store ID was explicitly specified in the details dictionary.
-    // Retrieve its corresponding cookie store.
-    std::string store_id_value;
-    // Get the store ID string or return false.
-    EXTENSION_FUNCTION_VALIDATE(
-        details->GetString(keys::kStoreIdKey, &store_id_value));
+  if (in_store_id != NULL) {
     store_profile = cookies_helpers::ChooseProfileFromStoreId(
-        store_id_value, profile(), include_incognito());
+        *in_store_id, profile(), include_incognito());
     if (!store_profile) {
       error_ = ExtensionErrorUtils::FormatErrorMessage(
-          keys::kInvalidStoreIdError, store_id_value);
+          keys::kInvalidStoreIdError, *in_store_id);
       return false;
     }
   } else {
@@ -175,8 +173,8 @@ bool CookiesFunction::ParseStoreContext(const DictionaryValue* details,
 
   if (context)
     *context = store_profile->GetRequestContext();
-  if (store_id)
-    *store_id = cookies_helpers::GetStoreIdFromProfile(store_profile);
+  if (out_store_id)
+    *out_store_id = cookies_helpers::GetStoreIdFromProfile(store_profile);
 
   return true;
 }
@@ -186,20 +184,19 @@ GetCookieFunction::GetCookieFunction() {}
 GetCookieFunction::~GetCookieFunction() {}
 
 bool GetCookieFunction::RunImpl() {
-  // Return false if the arguments are malformed.
-  DictionaryValue* details;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
-  DCHECK(details);
+  scoped_ptr<Get::Params> params(Get::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
   // Read/validate input parameters.
-  if (!ParseUrl(details, &url_, true))
+  if (!ParseUrl(params->details.url, &url_, true))
     return false;
 
-  // Get the cookie name string or return false.
-  EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kNameKey, &name_));
+  name_ = params->details.name;
 
   net::URLRequestContextGetter* store_context = NULL;
-  if (!ParseStoreContext(details, &store_context, &store_id_))
+  if (!ParseStoreContext(params->details.store_id.get(),
+                         &store_context,
+                         &store_id_))

[Aaron Boodman, 2012/07/04 00:04:31]

This code was badly formatted before; it should have curly braces.

[mitchellwrosen, 2012/07/04 02:04:40]

Done.

     return false;
 
   DCHECK(store_context && !store_id_.empty());
@@ -261,11 +258,15 @@ bool GetAllCookiesFunction::RunImpl() {
   DCHECK(details_);
 
   // Read/validate input parameters.
-  if (details_->HasKey(keys::kUrlKey) && !ParseUrl(details_, &url_, false))
-    return false;
+  scoped_ptr<GetAll::Params> params(GetAll::Params::Create(*args_));
+  if (params->details.url.get())

[Aaron Boodman, 2012/07/04 00:04:31]

This if statement should have curly braces. The one below is OK w/o.

[mitchellwrosen, 2012/07/04 02:04:40]

Done.

+    if (!ParseUrl(*params->details.url, &url_, false))
+      return false;
 
   net::URLRequestContextGetter* store_context = NULL;
-  if (!ParseStoreContext(details_, &store_context, &store_id_))
+  if (!ParseStoreContext(params->details.store_id.get(),
+                         &store_context,
+                         &store_id_))

[Aaron Boodman, 2012/07/04 00:04:31]

This requires curlies

[mitchellwrosen, 2012/07/04 02:04:40]

Done.

     return false;
   DCHECK(store_context);
   store_context_ = store_context;
@@ -319,54 +320,35 @@ SetCookieFunction::~SetCookieFunction() {
 }
 
 bool SetCookieFunction::RunImpl() {
-  // Return false if the arguments are malformed.
-  DictionaryValue* details;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
-  DCHECK(details);
+  scoped_ptr<Set::Params> params(Set::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
   // Read/validate input parameters.
-  if (!ParseUrl(details, &url_, true))
+  if (!ParseUrl(params->details.url, &url_, true))
       return false;
-  // The macros below return false if argument types are not as expected.
-  if (details->HasKey(keys::kNameKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kNameKey, &name_));
-  if (details->HasKey(keys::kValueKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kValueKey, &value_));
-  if (details->HasKey(keys::kDomainKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kDomainKey, &domain_));
-  if (details->HasKey(keys::kPathKey))
-    EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kPathKey, &path_));
-
-  if (details->HasKey(keys::kSecureKey)) {
-    EXTENSION_FUNCTION_VALIDATE(
-        details->GetBoolean(keys::kSecureKey, &secure_));
-  }
-  if (details->HasKey(keys::kHttpOnlyKey)) {
-    EXTENSION_FUNCTION_VALIDATE(
-        details->GetBoolean(keys::kHttpOnlyKey, &http_only_));
-  }
-  if (details->HasKey(keys::kExpirationDateKey)) {
-    Value* expiration_date_value;
-    EXTENSION_FUNCTION_VALIDATE(details->Get(keys::kExpirationDateKey,
-                                             &expiration_date_value));
-    double expiration_date;
-    if (expiration_date_value->IsType(Value::TYPE_INTEGER)) {
-      int expiration_date_int;
-      EXTENSION_FUNCTION_VALIDATE(
-          expiration_date_value->GetAsInteger(&expiration_date_int));
-      expiration_date = static_cast<double>(expiration_date_int);
-    } else {
-      EXTENSION_FUNCTION_VALIDATE(
-          expiration_date_value->GetAsDouble(&expiration_date));
-    }
+
+  if (params->details.name.get())
+    name_ = *params->details.name;

[Aaron Boodman, 2012/07/04 00:04:31]

Do we need all these members now? Can we just store the parsed params and refer
to that?

[mitchellwrosen, 2012/07/04 02:04:40]

That would clean this code up a lot. I thought about that but I couldn't come up
with a good way to do it.

Here's on idea I had: give SetCookieFunction its own scoped_ptr<Set::Params>
member variable, and initialize it with params_.reset(params.Pass()).

Another idea: give SetCookieFunction its own Set::Params member variable,
although then I believe it would have to be initialized with a call to .Pass()
for each of the scoped_ptrs inside of params.

What do you think? Would my first solution work?

[Aaron Boodman, 2012/07/04 06:35:40]

Yes, that is what I had in mind.

Naming nit: how about parsed_args_ for consistency with args_.
This seems like more effort for no reason.
Seems like it...

[mitchellwrosen, 2012/07/05 17:43:38]

I'm still going to have to make a bunch of local variables, because
SetCookieWithDetailsAsync takes a bunch of const refs. Should I:

a) Leave it how it is
b) Only have a parsed_args_ member variable but then essentially copy all of
this code in SetCookieOnIOThread
c) Change SetCookieWithDetailsAsync to take ptrs instead of const refs

[Aaron Boodman, 2012/07/05 18:09:55]

Why can't you just do SetCookieWithDetailsAsync(*parsed_args_->details.url, ...)
?

+  if (params->details.value.get())
+    value_ = *params->details.value;
+  if (params->details.domain.get())
+    domain_ = *params->details.domain;
+  if (params->details.path.get())
+    path_ = *params->details.path;
+  if (params->details.secure.get())
+    secure_ = *params->details.secure;
+  if (params->details.http_only.get())
+    http_only_ = *params->details.http_only;
+  if (params->details.expiration_date.get()) {
     // Time::FromDoubleT converts double time 0 to empty Time object. So we need
     // to do special handling here.
-    expiration_time_ = (expiration_date == 0) ?
-        base::Time::UnixEpoch() : base::Time::FromDoubleT(expiration_date);
+    expiration_time_ = (*params->details.expiration_date == 0) ?
+        base::Time::UnixEpoch() :
+        base::Time::FromDoubleT(*params->details.expiration_date);
   }
 
   net::URLRequestContextGetter* store_context = NULL;
-  if (!ParseStoreContext(details, &store_context, NULL))
+  if (!ParseStoreContext(params->details.store_id.get(), &store_context, NULL))
     return false;
   DCHECK(store_context);
   store_context_ = store_context;
@@ -436,20 +418,19 @@ RemoveCookieFunction::~RemoveCookieFunction() {
 }
 
 bool RemoveCookieFunction::RunImpl() {
-  // Return false if the arguments are malformed.
-  DictionaryValue* details;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
-  DCHECK(details);
+  scoped_ptr<Remove::Params> params(Remove::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
   // Read/validate input parameters.
-  if (!ParseUrl(details, &url_, true))
+  if (!ParseUrl(params->details.url, &url_, true))
     return false;
 
-  // Get the cookie name string or return false.
-  EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kNameKey, &name_));
+  name_ = params->details.name;

[Aaron Boodman, 2012/07/04 00:04:31]

same question here about storing params.

 
   net::URLRequestContextGetter* store_context = NULL;
-  if (!ParseStoreContext(details, &store_context, &store_id_))
+  if (!ParseStoreContext(params->details.store_id.get(),
+                         &store_context,
+                         &store_id_))

[Aaron Boodman, 2012/07/04 00:04:31]

braces

[mitchellwrosen, 2012/07/04 02:04:40]

Done.

     return false;
   DCHECK(store_context);
   store_context_ = store_context;
@@ -477,11 +458,11 @@ void RemoveCookieFunction::RemoveCookieOnIOThread() {
 
 void RemoveCookieFunction::RemoveCookieCallback() {
   // Build the callback result
-  DictionaryValue* resultDictionary = new DictionaryValue();
-  resultDictionary->SetString(keys::kNameKey, name_);
-  resultDictionary->SetString(keys::kUrlKey, url_.spec());
-  resultDictionary->SetString(keys::kStoreIdKey, store_id_);
-  result_.reset(resultDictionary);
+  Remove::Result::Details details;
+  details.name = name_;
+  details.url = url_.spec();
+  details.store_id = store_id_;
+  result_.reset(Remove::Result::Create(details));
 
   // Return to UI thread
   bool rv = BrowserThread::PostTask(
@@ -515,28 +496,33 @@ bool GetAllCookieStoresFunction::RunImpl() {
        iter != BrowserList::end(); ++iter) {
     Browser* browser = *iter;
     if (browser->profile() == original_profile) {
-      cookies_helpers::AppendToTabIdList(browser,
-                                                   original_tab_ids.get());
+      cookies_helpers::AppendToTabIdList(browser, original_tab_ids.get());
     } else if (incognito_tab_ids.get() &&
                browser->profile() == incognito_profile) {
-      cookies_helpers::AppendToTabIdList(browser,
-                                                   incognito_tab_ids.get());
+      cookies_helpers::AppendToTabIdList(browser, incognito_tab_ids.get());
     }
   }
   // Return a list of all cookie stores with at least one open tab.
-  ListValue* cookie_store_list = new ListValue();
+  std::vector<linked_ptr<CookieStore> > cookie_stores;
   if (original_tab_ids->GetSize() > 0) {
-    cookie_store_list->Append(
+    scoped_ptr<DictionaryValue> dict(
         cookies_helpers::CreateCookieStoreValue(
             original_profile, original_tab_ids.release()));
+
+    linked_ptr<CookieStore> cookie_store(new CookieStore());
+    CookieStore::Populate(*dict, cookie_store.get());
+    cookie_stores.push_back(cookie_store);
   }
   if (incognito_tab_ids.get() && incognito_tab_ids->GetSize() > 0 &&
       incognito_profile) {
-    cookie_store_list->Append(
+    scoped_ptr<DictionaryValue> dict(
         cookies_helpers::CreateCookieStoreValue(
             incognito_profile, incognito_tab_ids.release()));
+    linked_ptr<CookieStore> cookie_store(new CookieStore());
+    CookieStore::Populate(*dict, cookie_store.get());
+    cookie_stores.push_back(cookie_store);
   }
-  result_.reset(cookie_store_list);
+  result_.reset(GetAllCookieStores::Result::Create(cookie_stores));
   return true;
 }