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

chrome/browser/extensions/extension_cookies_api.cc

 // Copyright (c) 2010 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/extension_cookies_api.h"
 
 #include "base/json/json_writer.h"
 #include "base/task.h"
@@ skipping 256 matching lines @@
 SetCookieFunction::SetCookieFunction()
     : secure_(false),
       http_only_(false),
       success_(false) {
 }
 
 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));
   DCHECK(details);
 
   // Read/validate input parameters.
   if (!ParseUrl(details, &url_, true))
       return false;
   // The macros below return false if argument types are not as expected.
   if (details->HasKey(keys::kNameKey))
@@ skipping 43 matching lines @@
       BrowserThread::IO, FROM_HERE,
       NewRunnableMethod(this, &SetCookieFunction::SetCookieOnIOThread));
   DCHECK(rv);
 
   // Will finish asynchronously.
   return true;
 }
 
 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));
   DCHECK(rv);
 }
 
 void SetCookieFunction::RespondOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   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_);
 }
 
 namespace {
 
 class RemoveCookieTask : public Task {
  public:
   RemoveCookieTask(const GURL& url,
                    const std::string& name,
                    const scoped_refptr<URLRequestContextGetter>& context_getter)
@@ skipping 10 matching lines @@
   const GURL url_;
   const std::string name_;
   const scoped_refptr<URLRequestContextGetter> context_getter_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoveCookieTask);
 };
 
 }  // 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));
   DCHECK(details);
 
   // Read/validate input parameters.
   GURL url;
   if (!ParseUrl(details, &url, true))
     return false;
 
   std::string name;
   // Get the cookie name string or return false.
   EXTENSION_FUNCTION_VALIDATE(details->GetString(keys::kNameKey, &name));
 
   URLRequestContextGetter* store_context = NULL;
   if (!ParseStoreContext(details, &store_context, NULL))
     return false;
   DCHECK(store_context);
 
   // We don't bother to synchronously wait for the result here, because
   // CookieMonster is only ever accessed on the IO thread, so any other accesses
   // should happen after this.
   bool rv = BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       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;
 }
 
 void RemoveCookieFunction::Run() {
   SendResponse(RunImpl());
 }
 
 bool GetAllCookieStoresFunction::RunImpl() {
   Profile* original_profile = profile();
   DCHECK(original_profile);
@@ skipping 34 matching lines @@
         extension_cookies_helpers::CreateCookieStoreValue(
             incognito_profile, incognito_tab_ids.release()));
   }
   result_.reset(cookie_store_list);
   return true;
 }
 
 void GetAllCookieStoresFunction::Run() {
   SendResponse(RunImpl());
 }