Refactor chrome.cookies API to use JSON schema compiler.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Implements the Chrome Extensions Cookies API.
 
 #include "chrome/browser/extensions/api/cookies/cookies_api.h"
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/cookies/cookies_api_constants.h"
 #include "chrome/browser/extensions/api/cookies/cookies_helpers.h"
 #include "chrome/browser/extensions/extension_event_router.h"
 #include "chrome/browser/profiles/profile.h"
 #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"
 #include "content/public/browser/notification_service.h"
 #include "net/cookies/cookie_monster.h"
 #include "net/url_request/url_request_context.h"
 #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;
 
 ExtensionCookiesEventRouter::ExtensionCookiesEventRouter(Profile* profile)
     : profile_(profile) {}
 
 ExtensionCookiesEventRouter::~ExtensionCookiesEventRouter() {}
 
 void ExtensionCookiesEventRouter::Init() {
@@ skipping 74 matching lines @@
 void ExtensionCookiesEventRouter::DispatchEvent(Profile* profile,
                                                 const char* event_name,
                                                 const std::string& json_args,
                                                 GURL& cookie_domain) {
   if (profile && profile->GetExtensionEventRouter()) {
     profile->GetExtensionEventRouter()->DispatchEventToRenderers(
         event_name, json_args, profile, cookie_domain, EventFilteringInfo());
   }
 }
 
-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(
         keys::kInvalidUrlError, url_string);
     return false;
   }
   // Check against host permissions if needed.
   if (check_host_permissions &&
       !GetExtension()->HasHostPermission(*url)) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(
         keys::kNoHostPermissionsError, url->spec());
     return false;
   }
   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 {
     // The store ID was not specified; use the current execution context's
     // cookie store by default.
     // GetCurrentBrowser() already takes into account incognito settings.
     Browser* current_browser = GetCurrentBrowser();
     if (!current_browser) {
       error_ = keys::kNoCookieStoreFoundError;
       return false;
     }
     store_profile = current_browser->profile();
   }
   DCHECK(store_profile);
 
   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;
 }
 
 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());
   store_context_ = store_context;
 
   bool rv = BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&GetCookieFunction::GetCookieOnIOThread, this));
   DCHECK(rv);
 
@@ skipping 41 matching lines @@
 GetAllCookiesFunction::GetAllCookiesFunction() : details_(NULL) {}
 
 GetAllCookiesFunction::~GetAllCookiesFunction() {}
 
 bool GetAllCookiesFunction::RunImpl() {
   // Return false if the arguments are malformed.
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details_));
   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;
 
   bool rv = BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&GetAllCookiesFunction::GetAllCookiesOnIOThread, this));
   DCHECK(rv);
 
   // Will finish asynchronously.
@@ skipping 33 matching lines @@
 SetCookieFunction::SetCookieFunction()
     : secure_(false),
       http_only_(false),
       success_(false) {
 }
 
 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;
 
   bool rv = BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SetCookieFunction::SetCookieOnIOThread, this));
   DCHECK(rv);
 
   // Will finish asynchronously.
@@ skipping 49 matching lines @@
   SendResponse(success_);
 }
 
 RemoveCookieFunction::RemoveCookieFunction() : success_(false) {
 }
 
 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;
 
   // Pass the work off to the IO thread.
   bool rv = BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&RemoveCookieFunction::RemoveCookieOnIOThread, this));
   DCHECK(rv);
 
   // Will return asynchronously.
   return true;
 }
 
 void RemoveCookieFunction::RemoveCookieOnIOThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // Remove the cookie
   net::CookieStore* cookie_store =
       store_context_->GetURLRequestContext()->cookie_store();
   cookie_store->DeleteCookieAsync(
       url_, name_,
       base::Bind(&RemoveCookieFunction::RemoveCookieCallback, this));
 }
 
 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(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&RemoveCookieFunction::RespondOnUIThread, this));
   DCHECK(rv);
 }
 
 void RemoveCookieFunction::RespondOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ skipping 13 matching lines @@
   }
   DCHECK(original_profile != incognito_profile);
 
   // Iterate through all browser instances, and for each browser,
   // add its tab IDs to either the regular or incognito tab ID list depending
   // whether the browser is regular or incognito.
   for (BrowserList::const_iterator iter = BrowserList::begin();
        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;
 }
 
 void GetAllCookieStoresFunction::Run() {
   SendResponse(RunImpl());
 }
 
 }  // namespace extensions