Adding callbacks to `chrome.cookies.{set,remove}`.

chrome/browser/extensions/extension_cookies_api.cc

Index: chrome/browser/extensions/extension_cookies_api.cc
diff --git a/chrome/browser/extensions/extension_cookies_api.cc b/chrome/browser/extensions/extension_cookies_api.cc
index 04d5600815c345b951a31555c29150db7b769e77..52124829cf755a7c3f6dd427b8901bd737aaf076 100644
--- a/chrome/browser/extensions/extension_cookies_api.cc
+++ b/chrome/browser/extensions/extension_cookies_api.cc
@@ -274,6 +274,10 @@ SetCookieFunction::~SetCookieFunction() {
 }
 
 bool SetCookieFunction::RunImpl() {
+  // Start with `null` being returned to callback, in case things fail in

[jochen (gone - plz use gerrit), 2011/02/15 13:44:10]

please no backticks. I think double quotes are the most commonly used

[Mike West, 2011/02/15 14:26:58]

Hasn't everyone internalized Markdown yet?  :)

Changed, here and elsewhere.

+  // this syncronous bit of workflow.

[jochen (gone - plz use gerrit), 2011/02/15 13:44:10]

of the workflow?

[Mike West, 2011/02/15 14:26:58]

Done.

+  result_.reset(Value::CreateNullValue());
+
   // Return false if the arguments are malformed.
   DictionaryValue* details;
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
@@ -337,12 +341,16 @@ bool SetCookieFunction::RunImpl() {
 
 void SetCookieFunction::SetCookieOnIOThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  net::CookieMonster* cookie_monster =
-      store_context_->GetCookieStore()->GetCookieMonster();
+  net::CookieStore* cookie_store = store_context_->GetCookieStore();
+  net::CookieMonster* cookie_monster = cookie_store->GetCookieMonster();
   success_ = cookie_monster->SetCookieWithDetails(
       url_, name_, value_, domain_, path_, expiration_time_,
       secure_, http_only_);
 
+  // Pull the newly set cookie

[jochen (gone - plz use gerrit), 2011/02/15 13:44:10]

nit. dot at end of comments

[Mike West, 2011/02/15 14:26:58]

Done.

+  cookie_list_ =
+      extension_cookies_helpers::GetCookieListFromStore(cookie_store, url_);

[jochen (gone - plz use gerrit), 2011/02/15 13:44:10]

why don't you set result_ already here? you wouldn't need the cookie_list_ then

[Mike West, 2011/02/15 14:26:58]

This is the workflow used in `GetCookieFunction::GetCookieOnIOThread()` and
`GetCookieFunction::RespondOnUIThread()` (it's pretty much copy/pasted
verbatim).  I assumed there was a good reason for the split.  :)

If it's better to set `result_` on the IO thread, I'd suggest changing both
locations (which I'm happy to do).

[Aaron Boodman, 2011/02/15 22:26:04]

It seems OK to set the result on the IO thread. Doing that relies on knowledge
that nobody else is touching result_ at the same time, but that seems like an OK
assumption to make here, since the "result" of a function should not be accessed
until the function returns.

[Mike West, 2011/02/16 09:52:40]

Alright.  I've made changes to `GetCookieFunction`, `GetAllCookiesFunction`, and
`SetCookieFunction` to drop the `cookie_list_` member variable, and move the
processing to the IO thread.

I'm not actually sure that jumping back to the UI thread for `SendResponse` is
necessary, but I'm not entirely clear about what should be done where, so I've
left it.  It feels unnecessary, however.  What do you think?

+
   bool rv = BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &SetCookieFunction::RespondOnUIThread));
@@ -354,7 +362,20 @@ void SetCookieFunction::RespondOnUIThread() {
   if (!success_) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(
         keys::kCookieSetFailedError, name_);
+  } else {
+    net::CookieList::iterator it;
+    for (it = cookie_list_.begin(); it != cookie_list_.end(); ++it) {
+      // Return the first matching cookie. Relies on the fact that the
+      // CookieMonster returns them in canonical order (longest path, then
+      // earliest creation time).
+      if (it->Name() == name_) {
+        result_.reset(
+            extension_cookies_helpers::CreateCookieValue(*it, store_id_));
+        break;
+      }
+    }
   }
+
   SendResponse(success_);
 }
 
@@ -385,6 +406,10 @@ class RemoveCookieTask : public Task {
 }  // namespace
 
 bool RemoveCookieFunction::RunImpl() {
+  // Start with `null` being returned to callback, in case things fail in
+  // this syncronous bit of workflow.
+  result_.reset(Value::CreateNullValue());
+
   // Return false if the arguments are malformed.
   DictionaryValue* details;
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
@@ -412,6 +437,13 @@ bool RemoveCookieFunction::RunImpl() {
       new RemoveCookieTask(url, name, make_scoped_refptr(store_context)));
   DCHECK(rv);
 
+  DictionaryValue* test = new DictionaryValue();

[jochen (gone - plz use gerrit), 2011/02/15 13:44:10]

better variable name please

[Mike West, 2011/02/15 14:26:58]

*ahem*  Yeah.  That was not smart.

+  std::string url_string;
+  details->GetString(keys::kUrlKey, &url_string);
+  test->SetString(keys::kNameKey, name);
+  test->SetString(keys::kUrlKey, url_string);
+  result_.reset(test);

[jochen (gone - plz use gerrit), 2011/02/15 13:44:10]

hum, so you return name/url even if no cookie was deleted?

[Mike West, 2011/02/15 14:26:58]

I apparently do.  I'd somehow convinced myself that the validation macros above
stopped this from happening.

That said, I'm not sure what the best thing to do here would be.  Three things
can happen:

1.  `remove` removes a cookie.  In that case, returning the data is clear.
2.  `remove` is given invalid data, caught before it hits the CookieMonster.  In
that case, it's really an error, and passing `null` to the callback makes sense.
3.  `remove` is given valid data, but the cookie doesn't exist.  In that case,
it's not an _error_, is it?  It's something else.  Since the results of this
condition (the cookie never existed, so it _still_ doesn't exist) are
indistinguishable from #1, I'm not sure making the distinction is clearly
correct.

I'm not strongly committed to that position, though.  If you think we should do
something else, I'm happy to.  Perhaps a new `lastError` message ("The cookie
didn't exist...")?

[Aaron Boodman, 2011/02/15 22:26:04]

My vague preference is for the way you have it. Since extension developers are
dealing with this system out of process, things can change between when they
issue a request and when the request is processed. I feel like we should try to
be lenient in such cases. If the developer really cares to know that a cookie he
removed was there before, he can always get() it first, then remove() it, but
this seems like an unimportant use case to me.

[Mike West, 2011/02/16 09:52:40]

I think that makes sense.  I'll leave it this way.

+
   return true;
 }