Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10702088:
Refactor chrome.cookies API to use JSON schema compiler. (Closed)
|
Created:
8 years, 5 months ago by mitchellwrosen Modified:
8 years, 5 months ago Reviewers:
Aaron Boodman CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRefactor chrome.cookies API to use JSON schema compiler.
BUG=121174
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147954
Patch Set 1 #
Total comments: 19
Patch Set 2 : Changes per comments #Patch Set 3 : Cookies helpers, unit tests, other nits #
Total comments: 20
Patch Set 4 : Changes per comments #Patch Set 5 : Trunk sync #Patch Set 6 : Sync #Messages
Total messages: 21 (0 generated)
Can you move these APIs that are in some other directory back into chrome/browser/extensions/api/ while you are making these changes? For a little while we thought we wanted to move the APIs out of c/b/e, but we were wrong.
http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:199: &store_id_)) This code was badly formatted before; it should have curly braces. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:262: if (params->details.url.get()) This if statement should have curly braces. The one below is OK w/o. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:269: &store_id_)) This requires curlies http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:331: name_ = *params->details.name; Do we need all these members now? Can we just store the parsed params and refer to that? http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:428: name_ = params->details.name; same question here about storing params. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:433: &store_id_)) braces http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.h (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.h:83: bool ParseUrl(std::string url_string, GURL* url, bool check_host_permissions); Input params that are non-primitive should almost always be const ref. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.h:89: bool ParseStoreContext(std::string* in_store_id, Can you just have a single std::string* store_id at the end, and assign to it if NULL?
http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:199: &store_id_)) On 2012/07/04 00:04:31, Aaron Boodman wrote: > This code was badly formatted before; it should have curly braces. Done. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:262: if (params->details.url.get()) On 2012/07/04 00:04:31, Aaron Boodman wrote: > This if statement should have curly braces. The one below is OK w/o. Done. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:269: &store_id_)) On 2012/07/04 00:04:31, Aaron Boodman wrote: > This requires curlies Done. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:331: name_ = *params->details.name; On 2012/07/04 00:04:31, Aaron Boodman wrote: > Do we need all these members now? Can we just store the parsed params and refer > to that? 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? http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:433: &store_id_)) On 2012/07/04 00:04:31, Aaron Boodman wrote: > braces Done. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.h (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.h:83: bool ParseUrl(std::string url_string, GURL* url, bool check_host_permissions); On 2012/07/04 00:04:31, Aaron Boodman wrote: > Input params that are non-primitive should almost always be const ref. Done. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.h:89: bool ParseStoreContext(std::string* in_store_id, On 2012/07/04 00:04:31, Aaron Boodman wrote: > Can you just have a single std::string* store_id at the end, and assign to it if > NULL? Can you elaborate on this? How can you assign to something if it's NULL? I do agree the parameters are a little weird, though. If |in_store_id| is non-NULL, then *out_store_id is just going to be set to *in_store_id is, which we already have. However if |in_store_id| is NULL then we need an out-parameter to report the store ID of the current context.
http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:331: name_ = *params->details.name; On 2012/07/04 02:04:40, mitchellwrosen wrote: > On 2012/07/04 00:04:31, Aaron Boodman wrote: > > Do we need all these members now? Can we just store the parsed params and > refer > > to that? > 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()). Yes, that is what I had in mind. Naming nit: how about parsed_args_ for consistency with args_. > 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. This seems like more effort for no reason. > What do you think? Would my first solution work? Seems like it... http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.h (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.h:89: bool ParseStoreContext(std::string* in_store_id, On 2012/07/04 02:04:40, mitchellwrosen wrote: > On 2012/07/04 00:04:31, Aaron Boodman wrote: > > Can you just have a single std::string* store_id at the end, and assign to it > if > > NULL? > Can you elaborate on this? How can you assign to something if it's NULL? I do > agree the parameters are a little weird, though. If |in_store_id| is non-NULL, > then *out_store_id is just going to be set to *in_store_id is, which we already > have. However if |in_store_id| is NULL then we need an out-parameter to report > the store ID of the current context. Right you are. You could use empty string for the case NULL is being used for today and combine them that way.
http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:331: name_ = *params->details.name; On 2012/07/04 06:35:40, Aaron Boodman wrote: > On 2012/07/04 02:04:40, mitchellwrosen wrote: > > On 2012/07/04 00:04:31, Aaron Boodman wrote: > > > Do we need all these members now? Can we just store the parsed params and > > refer > > > to that? > > 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()). > > Yes, that is what I had in mind. > > Naming nit: how about parsed_args_ for consistency with args_. > > > 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. > > This seems like more effort for no reason. > > > What do you think? Would my first solution work? > > Seems like it... 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
Also, I can't remember if I mentioned this already, but can you also move any APIs that aren't following the convention chrome/browser/extensions/api/<name>/<name>.*? It could be a separate CL. http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/cookies/cookies_api.cc:331: name_ = *params->details.name; On 2012/07/05 17:43:38, mitchellwrosen wrote: > On 2012/07/04 06:35:40, Aaron Boodman wrote: > > On 2012/07/04 02:04:40, mitchellwrosen wrote: > > > On 2012/07/04 00:04:31, Aaron Boodman wrote: > > > > Do we need all these members now? Can we just store the parsed params and > > > refer > > > > to that? > > > 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()). > > > > Yes, that is what I had in mind. > > > > Naming nit: how about parsed_args_ for consistency with args_. > > > > > 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. > > > > This seems like more effort for no reason. > > > > > What do you think? Would my first solution work? > > > > Seems like it... > > 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 Why can't you just do SetCookieWithDetailsAsync(*parsed_args_->details.url, ...) ?
> Why can't you just do SetCookieWithDetailsAsync(*parsed_args_->details.url, ...) > ? Isn't that dereferencing null?
Rather, it *is* null, instead of a null string, for example
On 2012/07/05 18:13:11, mitchellwrosen wrote: > Rather, it *is* null, instead of a null string, for example Nope, I was wrong. I forgot that the scoped ptr has to be pointing at /something/, when it's initialized in the schema compiler. Carry on.
On Thu, Jul 5, 2012 at 11:16 AM, <mitchellwrosen@chromium.org> wrote: > On 2012/07/05 18:13:11, mitchellwrosen wrote: >> >> Rather, it *is* null, instead of a null string, for example > > > Nope, I was wrong. I forgot that the scoped ptr has to be pointing at > /something/, when it's initialized in the schema compiler. Carry on. > > http://codereview.chromium.org/10702088/ You've lost me. Are your questions answered now or not? :) - a
> You've lost me. Are your questions answered now or not? :) > > - a Ya
Whoops - let me resurrect this topic one last time. I'll try to be more clear. By the way, Devlin told me you're on vacation until Monday, so please feel free to wait until then to answer! Consider some string like url, that was a std::string but now you're suggesting I just keep a scoped_ptr to the entire args struct and reference its members. I need a const ref to the string to pass to a function, but all I have is a scoped_ptr to a string that may or may not have been initialized (optional parameter). Thus *parsed_args_->details.url is de-referencing NULL.
I see. OK, bummer. Then maybe just get rid of the members and make these guys
locals?
You could also just do it inline when calling SetCookieWithDetailsAsync, like:
SetCookieWithDetailsAsync(
parsed_args_->monkey.get() ? *parsed_args_->monkey : "",
parsed_args_->baz.get() ? *parsed_args_->baz : false,
...);
In any case: LGTM
On 2012/07/06 01:33:45, Aaron Boodman wrote: > I see. OK, bummer. Then maybe just get rid of the members and make these guys > locals? > > You could also just do it inline when calling SetCookieWithDetailsAsync, like: > > SetCookieWithDetailsAsync( > parsed_args_->monkey.get() ? *parsed_args_->monkey : "", > parsed_args_->baz.get() ? *parsed_args_->baz : false, > ...); > > In any case: LGTM Could I please bother you for another LGTM? I made some significant changes (compared to the LGTM patch), mostly in cookies_helpers and also cookies_unittest. If I shouldn't ask for two LGTMs please let me know =). Ideally I would have refactored all of the above for the first patch. Sorry about that.
http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:210: DCHECK(parsed_args_->details.store_id.get()); Looks like you always expect this value to get passed. In that case, all that's needed is: EXTENSION_FUNCTION_VALIDATE(parsed_args_->details.store_id.get()); before using. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:212: DCHECK(store_context && !parsed_args_->details.store_id->empty()); I don't think it's necessary to DCHECK either of these. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:273: scoped_ptr<GetAll::Params> params(GetAll::Params::Create(*args_)); Why are you creating another instance of this? Isn't this what's in parsed_args_? http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:284: return false; The new string leaks here. I think safer/easier to just create a copy of the string on the stack, like: std::string store_id = parsed_args_->details.store_id.get() ? ... : ""; http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_api.h (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.h:93: std::string* store_id); Reverse these two params because it is the identifier for the thingy being fetched. The comment should be something like: Gets the store identified by |store_id| and returns it in |context|. If |store_id| contains an empty string, retrieves the current execution context's store. In this case, |store_id| is populated with the found store. Also in this case, |context| can be NULL if the caller only wants |store_id|. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_helpers.cc (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.cc:71: dict.SetString(keys::kNameKey, UTF8ToUTF16(cookie.Name())); Is it possible to just populate new_cookie directly? Rather than populating a JSON struct, then parsing that into the strongly typed object? Same elsewhere in this file. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.cc:144: linked_ptr<Cookie> cookie(CreateCookie(*it, *details->store_id)); Since you don't need the local, you could use make_linked_ptr() to shorten this slightly. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.cc:168: bool matchesName = !details_->name.get() || *details_->name == cookie.Name(); Consider restructuring this logic like: if (details->name.get() && details->name != cookie.Name()) return false; if (!MatchesDomain(...)) return false; ... return true; http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_helpers.h (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.h:46: // by the cookies API. The caller is responsible for freeing it. Change this to return scoped_ptr. Then you don't have to document the ownership rules; they will be enforced by the compiler. Same below. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.h:79: std::vector<linked_ptr<extensions::api::cookies::Cookie> >* match_vector); Create a typedef for this.
http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_api.cc (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:210: DCHECK(parsed_args_->details.store_id.get()); On 2012/07/09 17:34:48, Aaron Boodman wrote: > Looks like you always expect this value to get passed. In that case, all that's > needed is: > > EXTENSION_FUNCTION_VALIDATE(parsed_args_->details.store_id.get()); > > before using. I realized there's definitely no way for this check to fail, so I just removed it entirely. Should I put an EXTENSION_FUNCTION_VALIDATE anyways? http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:212: DCHECK(store_context && !parsed_args_->details.store_id->empty()); On 2012/07/09 17:34:49, Aaron Boodman wrote: > I don't think it's necessary to DCHECK either of these. Done. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:273: scoped_ptr<GetAll::Params> params(GetAll::Params::Create(*args_)); On 2012/07/09 17:34:49, Aaron Boodman wrote: > Why are you creating another instance of this? Isn't this what's in > parsed_args_? Sorry, I re-factored a couple times and forgot to clean this method up. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.cc:284: return false; On 2012/07/09 17:34:49, Aaron Boodman wrote: > The new string leaks here. I think safer/easier to just create a copy of the > string on the stack, like: > > std::string store_id = parsed_args_->details.store_id.get() ? ... : ""; Done. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_api.h (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_api.h:93: std::string* store_id); On 2012/07/09 17:34:49, Aaron Boodman wrote: > Reverse these two params because it is the identifier for the thingy being > fetched. The comment should be something like: > > Gets the store identified by |store_id| and returns it in |context|. > > If |store_id| contains an empty string, retrieves the current execution > context's store. In this case, |store_id| is populated with the found store. > Also in this case, |context| can be NULL if the caller only wants |store_id|. Done. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_helpers.cc (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.cc:71: dict.SetString(keys::kNameKey, UTF8ToUTF16(cookie.Name())); On 2012/07/09 17:34:49, Aaron Boodman wrote: > Is it possible to just populate new_cookie directly? Rather than populating a > JSON struct, then parsing that into the strongly typed object? Same elsewhere in > this file. Done for populating a Cookie, but I think it may be more convenient to simply use CookieStore::Populate. Otherwise, I would have to populate a vector<int> from a ListValue, essentially duplicating json_schema_compiler::util::PopulateArrayFromList. What do you think? http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.cc:144: linked_ptr<Cookie> cookie(CreateCookie(*it, *details->store_id)); On 2012/07/09 17:34:49, Aaron Boodman wrote: > Since you don't need the local, you could use make_linked_ptr() to shorten this > slightly. Done. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.cc:168: bool matchesName = !details_->name.get() || *details_->name == cookie.Name(); On 2012/07/09 17:34:49, Aaron Boodman wrote: > Consider restructuring this logic like: > > if (details->name.get() && details->name != cookie.Name()) > return false; > > if (!MatchesDomain(...)) > return false; > > ... > > return true; Done. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... File chrome/browser/extensions/api/cookies/cookies_helpers.h (right): http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.h:46: // by the cookies API. The caller is responsible for freeing it. On 2012/07/09 17:34:49, Aaron Boodman wrote: > Change this to return scoped_ptr. Then you don't have to document the ownership > rules; they will be enforced by the compiler. Same below. Done. http://codereview.chromium.org/10702088/diff/3002/chrome/browser/extensions/a... chrome/browser/extensions/api/cookies/cookies_helpers.h:79: std::vector<linked_ptr<extensions::api::cookies::Cookie> >* match_vector); On 2012/07/09 17:34:49, Aaron Boodman wrote: > Create a typedef for this. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10702088/2...
Failed to apply patch for chrome/common/extensions/api/api.gyp: While running patch -p1 --forward --force; patching file chrome/common/extensions/api/api.gyp Hunk #1 FAILED at 19. 1 out of 1 hunk FAILED -- saving rejects to file chrome/common/extensions/api/api.gyp.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10702088/2...
Change committed as 147954 |
