Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6525016:
Adding callbacks to `chrome.cookies.{set,remove}`. (Closed)
|
Created:
9 years, 10 months ago by Mike West Modified:
9 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org, Paweł Hajdan Jr., Cindy Lau, Aaron Boodman, Erik does not do reviews Visibility:
Public. |
DescriptionAdding callbacks to `chrome.cookies.{set,remove}`.
Currently, neither `.set` nor `.remove` give the user the
ability to take action after the asynchronous cookie
monster does its thing in the background. One can listen
for events, but can't distinguish between events generated
from one's own code, and events generated through
normal interaction with the web. This makes infinite
loops much easier to write than they should be. Callbacks
make that class of problem simpler to deal with.
BUG=70102
TEST=Check that `.set` and `.remove` callbacks are triggered.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75242
Patch Set 1 #
Total comments: 23
Patch Set 2 : Fixes based on Jochen's comments. #
Total comments: 1
Patch Set 3 : Dropping 'cookie_list_' member var, setting 'result_' on IO thread. #
Total comments: 20
Patch Set 4 : Dropping the explicit setting of 'null'; failures will pass no arguments to the callback. #Patch Set 5 : Fixing variable names, clarifying a test, and restoring accidentally deleted comments. #
Total comments: 10
Patch Set 6 : Dropping samples.json, addressing Jochen's comments." #
Total comments: 2
Patch Set 7 : Whitespace. #
Messages
Total messages: 13 (0 generated)
Hi all. This is the first CL I'm submitting for review, so please do review harshly. I'm not entirely sure about the implementation (in particular the `result_` population in `RemoveCookieFunction::RunImpl()`. I had a chat with Jochen about the general approach, and he basically said "submit a CL and see what Aaron and Erik say." So. Hi Aaron and Erik (and Jochen). :) -Mike
http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:277: // Start with `null` being returned to callback, in case things fail in please no backticks. I think double quotes are the most commonly used http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:278: // this syncronous bit of workflow. of the workflow? http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:350: // Pull the newly set cookie nit. dot at end of comments http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:352: extension_cookies_helpers::GetCookieListFromStore(cookie_store, url_); why don't you set result_ already here? you wouldn't need the cookie_list_ then http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:440: DictionaryValue* test = new DictionaryValue(); better variable name please http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:445: result_.reset(test); hum, so you return name/url even if no cookie was deleted? http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. nit. put 2011 here http://codereview.chromium.org/6525016/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6525016/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:3305: "name": "cookie", "$ref": "Cookie", "optional": true, "description": "Contains details about the cookie that's been set. If setting failed for any reason, this will be `null`, and `chrome.extension.lastError` will be set." no backticks also, why not return the storeId here as well? http://codereview.chromium.org/6525016/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:3339: "storeId": {"type": "string", "optional": true, "description": "The ID of the cookie store from which the cookie was removed."} why would storeId be optional? http://codereview.chromium.org/6525016/diff/1/chrome/test/data/extensions/api... File chrome/test/data/extensions/api_test/cookies/api/tab.html (right): http://codereview.chromium.org/6525016/diff/1/chrome/test/data/extensions/api... chrome/test/data/extensions/api_test/cookies/api/tab.html:267: no empty lines
Two outstanding issues after I've taken care of the clear fixes Jochen noted: http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... and http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... I'd like input on solutions to both of those; I'm happy to implement whatever we decide makes sense. Thanks! -Mike http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:277: // Start with `null` being returned to callback, in case things fail in On 2011/02/15 13:44:10, jochen wrote: > please no backticks. I think double quotes are the most commonly used Hasn't everyone internalized Markdown yet? :) Changed, here and elsewhere. http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:278: // this syncronous bit of workflow. On 2011/02/15 13:44:10, jochen wrote: > of the workflow? Done. http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:350: // Pull the newly set cookie On 2011/02/15 13:44:10, jochen wrote: > nit. dot at end of comments Done. http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:352: extension_cookies_helpers::GetCookieListFromStore(cookie_store, url_); On 2011/02/15 13:44:10, jochen wrote: > why don't you set result_ already here? you wouldn't need the cookie_list_ then 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). http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:440: DictionaryValue* test = new DictionaryValue(); On 2011/02/15 13:44:10, jochen wrote: > better variable name please *ahem* Yeah. That was not smart. http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:445: result_.reset(test); On 2011/02/15 13:44:10, jochen wrote: > hum, so you return name/url even if no cookie was deleted? 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...")? http://codereview.chromium.org/6525016/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6525016/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:3305: "name": "cookie", "$ref": "Cookie", "optional": true, "description": "Contains details about the cookie that's been set. If setting failed for any reason, this will be `null`, and `chrome.extension.lastError` will be set." On 2011/02/15 13:44:10, jochen wrote: > no backticks Done. > also, why not return the storeId here as well? The storeId is returned as a property of the cookie that's been set. That's probably enough. http://codereview.chromium.org/6525016/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:3339: "storeId": {"type": "string", "optional": true, "description": "The ID of the cookie store from which the cookie was removed."} On 2011/02/15 13:44:10, jochen wrote: > why would storeId be optional? Because I thought it would be tough to grab. As it turns out, it isn't, since we need it for CookieMonster anyway. Removing the optional flag. http://codereview.chromium.org/6525016/diff/1/chrome/test/data/extensions/api... File chrome/test/data/extensions/api_test/cookies/api/tab.html (right): http://codereview.chromium.org/6525016/diff/1/chrome/test/data/extensions/api... chrome/test/data/extensions/api_test/cookies/api/tab.html:267: On 2011/02/15 13:44:10, jochen wrote: > no empty lines Done.
http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:352: extension_cookies_helpers::GetCookieListFromStore(cookie_store, url_); On 2011/02/15 14:26:58, Mike West wrote: > On 2011/02/15 13:44:10, jochen wrote: > > why don't you set result_ already here? you wouldn't need the cookie_list_ > then > > 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). 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. http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:445: result_.reset(test); On 2011/02/15 14:26:58, Mike West wrote: > On 2011/02/15 13:44:10, jochen wrote: > > hum, so you return name/url even if no cookie was deleted? > > 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...")? 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. http://codereview.chromium.org/6525016/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:277: // Start with `null` being returned to callback, in case things fail in There is no need to explicitly set the result to null. The system will send no arguments to the callback in the case of error, and will set the lastError property so that the error is detectable.
http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:352: extension_cookies_helpers::GetCookieListFromStore(cookie_store, url_); > It seems OK to set the result on the IO thread. 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? http://codereview.chromium.org/6525016/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:445: result_.reset(test); On 2011/02/15 22:26:04, Aaron Boodman wrote: > My vague preference is for the way you have it. I think that makes sense. I'll leave it this way.
you can send the response only from the UI thread http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (left): http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:196: // Return the first matching cookie. Relies on the fact that the why did you remove the comment? http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:153: EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details)); so if this fails, you return with undefined result instead of null result. is this intended? http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:186: net::CookieList cookie_list_ = no trailing underscore for local variables http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:239: net::CookieList cookie_list_ = no underscore http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:242: const Extension* extension = GetExtension(); not sure whether Extension is thread safe? http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:345: net::CookieList cookie_list_ = no underscore http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:418: std::string store_id_string; just store_id http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:432: std::string url_string; just url http://codereview.chromium.org/6525016/diff/9/chrome/test/data/extensions/api... File chrome/test/data/extensions/api_test/cookies/api/tab.html (right): http://codereview.chromium.org/6525016/diff/9/chrome/test/data/extensions/api... chrome/test/data/extensions/api_test/cookies/api/tab.html:311: chrome.test.assertTrue(typeof(data.storeId) === "string"); why not assertEq?
also, you should be able to send tryjobs now :)
More fixes based on Jochen's comments. Only outstanding issue I see is whether or not `Extension` can be safely used on the IO thread, or whether it must to be restricted to the UI thread. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (left): http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:196: // Return the first matching cookie. Relies on the fact that the On 2011/02/16 10:02:09, jochen wrote: > why did you remove the comment? Copy/paste error. Fixed now. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:153: EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details)); On 2011/02/16 10:02:09, jochen wrote: > so if this fails, you return with undefined result instead of null result. is > this intended? Based on what Aaron said in his comments, it seems to be the expected result. It's consistent with the "error" behavior of the other API functions, though I think it's arguable that that behavior isn't desirable. :) http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:186: net::CookieList cookie_list_ = On 2011/02/16 10:02:09, jochen wrote: > no trailing underscore for local variables Done. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:239: net::CookieList cookie_list_ = On 2011/02/16 10:02:09, jochen wrote: > no underscore Done. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:242: const Extension* extension = GetExtension(); On 2011/02/16 10:02:09, jochen wrote: > not sure whether Extension is thread safe? No idea. I was copy/pasting here for consistency with the cookie_list-less `GetCookieFunction`. :) http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:345: net::CookieList cookie_list_ = On 2011/02/16 10:02:09, jochen wrote: > no underscore Done. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:418: std::string store_id_string; On 2011/02/16 10:02:09, jochen wrote: > just store_id Done. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:432: std::string url_string; On 2011/02/16 10:02:09, jochen wrote: > just url `url` is already a variable in this scope (line 409). That's why I appended `_string` to it, and `store_id`. http://codereview.chromium.org/6525016/diff/9/chrome/test/data/extensions/api... File chrome/test/data/extensions/api_test/cookies/api/tab.html (right): http://codereview.chromium.org/6525016/diff/9/chrome/test/data/extensions/api... chrome/test/data/extensions/api_test/cookies/api/tab.html:311: chrome.test.assertTrue(typeof(data.storeId) === "string"); On 2011/02/16 10:02:09, jochen wrote: > why not assertEq? I assumed that the current execution context's storeId wasn't fixed. Will it always be the same value on every execution? Looking through the code again, there's a similar test on line 227 that does more or less the same test I'm doing here (testing `typeof` rather than an explicit value. I'd rather copy that test down here than hard-code a fixed value.
http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:153: EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details)); On 2011/02/16 11:07:28, Mike West wrote: > On 2011/02/16 10:02:09, jochen wrote: > > so if this fails, you return with undefined result instead of null result. is > > this intended? > > Based on what Aaron said in his comments, it seems to be the expected result. > It's consistent with the "error" behavior of the other API functions, though I > think it's arguable that that behavior isn't desirable. :) Agree the current mechanism is bad, but I'd rather be consistent in this case. http://codereview.chromium.org/6525016/diff/9/chrome/browser/extensions/exten... chrome/browser/extensions/extension_cookies_api.cc:242: const Extension* extension = GetExtension(); On 2011/02/16 11:07:28, Mike West wrote: > On 2011/02/16 10:02:09, jochen wrote: > > not sure whether Extension is thread safe? > > No idea. I was copy/pasting here for consistency with the cookie_list-less > `GetCookieFunction`. :) It is. The intention is that the const Extension* instances in the browser process are immutable and can be used from any thread.
On 2011/02/16 19:45:10, Aaron Boodman wrote: > Agree the current mechanism is bad, but I'd rather be consistent in this case. > ... > It is. The intention is that the const Extension* instances in the browser > process are immutable and can be used from any thread. Thanks Aaron! In that case, I don't see any unaddressed comments. Is there anything I can do to further clean up the code? I'm submitting it to the trybots again, as I have the impression that the windows and mac bots were flakey when I submitted earlier. Both have errors that shouldn't have anything to do with my code. :)
http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:181: result_.reset(Value::CreateNullValue()); based on your comments, shouldn't get also return undefined on a parsing error? http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:338: net::CookieMonster* cookie_monster = cookie_store->GetCookieMonster(); cookie monster implements cookie store, so you can use cookie monster everywhere http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:370: no empty line http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:430: std::string url_string; Then use url.spec() instead http://codereview.chromium.org/6525016/diff/4003/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/6525016/diff/4003/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:951: "source_hash": "c746d9114348f4b414c1ec05e988e2807feb963a", i think you should remove this file from the CL
Thanks, Jochen! http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:181: result_.reset(Value::CreateNullValue()); On 2011/02/17 08:18:26, jochen wrote: > based on your comments, shouldn't get also return undefined on a parsing error? You're right. Though I still don't like it. :) Moving this to after the `for` loop... more or less where it was before I touched the code at all. ;) http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:338: net::CookieMonster* cookie_monster = cookie_store->GetCookieMonster(); On 2011/02/17 08:18:26, jochen wrote: > cookie monster implements cookie store, so you can use cookie monster everywhere Done. http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:370: On 2011/02/17 08:18:26, jochen wrote: > no empty line Done. http://codereview.chromium.org/6525016/diff/4003/chrome/browser/extensions/ex... chrome/browser/extensions/extension_cookies_api.cc:430: std::string url_string; On 2011/02/17 08:18:26, jochen wrote: > Then use url.spec() instead Done. http://codereview.chromium.org/6525016/diff/4003/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/6525016/diff/4003/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:951: "source_hash": "c746d9114348f4b414c1ec05e988e2807feb963a", On 2011/02/17 08:18:26, jochen wrote: > i think you should remove this file from the CL Done.
LGTM http://codereview.chromium.org/6525016/diff/11002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/6525016/diff/11002/chrome/browser/extensions/e... chrome/browser/extensions/extension_cookies_api.cc:180: no empty line http://codereview.chromium.org/6525016/diff/11002/chrome/browser/extensions/e... chrome/browser/extensions/extension_cookies_api.cc:183: no empty line |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
