Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
308053009:
Add contextual search to the template url system (Closed)
|
Created:
6 years, 6 months ago by jeremycho Modified:
6 years, 6 months ago Reviewers:
Donn Denman, David Trainor- moved to gerrit, Peter Kasting, pedro (no code reviews), donnd CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis includes a new URL path /_/contextualsearch and a /search parameter specifying the contextual search version, if any.
BUG=379196
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276874
Patch Set 1 : Patch 1 #
Total comments: 7
Patch Set 2 : Respond to comments. #
Total comments: 11
Patch Set 3 : Respond to comments and make contextual search /search requests accessible from Java. #
Total comments: 8
Patch Set 4 : Respond to comments #Patch Set 5 : Fix test. #Patch Set 6 : Add selection parameter. #
Total comments: 18
Patch Set 7 : Respond to comments. #Patch Set 8 : Rebase and fix try error. #Patch Set 9 : Rebase again. #Patch Set 10 : Rebase again! #Patch Set 11 : *sigh* another rebase! #Messages
Total messages: 43 (0 generated)
Hi Donn - can you do the initial review? Thanks!
Great job Jeremy! This is looking good. I just left you a comment. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:113: What about the page's URL?
Other than missing the base-page URL, this looks great to me too! I think we should refer to it as the base-page or host-page URL to keep it from being confusing with the contextual_search_url produced by this template. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/prepopulated_engines.json:533: "contextual_search_url": "{google:baseURL}_/contextualsearch?{google:contextualSearchStart}{google:contextualSearchEnd}{google:contextualSearchContent}{google:contextualSearchEncoding}", We need to include the {google:contextualSearchUrl} here too. I know we're not really using this for anything now, but I think we will be able to use it very soon and it will be very helpful. Also it looks like the contextualSearchVersion is not listed on this line. I think we probably want it here too, but not sure. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:110: const char kGoogleContextualSearchContent[] = "google:contextualSearchContent"; Add the contextualSearchUrl here too. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:113: On 2014/06/02 17:39:51, pedrosimonetti wrote: > What about the page's URL? +1
On 2014/06/02 18:09:46, Donn wrote: > Other than missing the base-page URL, this looks great to me too! I think we > should refer to it as the base-page or host-page URL to keep it from being > confusing with the contextual_search_url produced by this template. > > https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... > File chrome/browser/search_engines/prepopulated_engines.json (right): > > https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... > chrome/browser/search_engines/prepopulated_engines.json:533: > "contextual_search_url": > "{google:baseURL}_/contextualsearch?{google:contextualSearchStart}{google:contextualSearchEnd}{google:contextualSearchContent}{google:contextualSearchEncoding}", > We need to include the {google:contextualSearchUrl} here too. I know we're not > really using this for anything now, but I think we will be able to use it very > soon and it will be very helpful. > > Also it looks like the contextualSearchVersion is not listed on this line. I > think we probably want it here too, but not sure. > > https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... > File chrome/browser/search_engines/template_url.cc (right): > > https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... > chrome/browser/search_engines/template_url.cc:110: const char > kGoogleContextualSearchContent[] = "google:contextualSearchContent"; > Add the contextualSearchUrl here too. > > https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... > File chrome/browser/search_engines/template_url.h (right): > > https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... > chrome/browser/search_engines/template_url.h:113: > On 2014/06/02 17:39:51, pedrosimonetti wrote: > > What about the page's URL? > > +1 +1 for naming it "base page URL", since this is the same terminology used in the downstream codebase.
Thanks for the comments. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/prepopulated_engines.json:533: "contextual_search_url": "{google:baseURL}_/contextualsearch?{google:contextualSearchStart}{google:contextualSearchEnd}{google:contextualSearchContent}{google:contextualSearchEncoding}", Done. The version was primarily for indicating a /search is a contextual search request, but it's possible we might actually want it for versioning too, so including it here makes sense. On 2014/06/02 18:09:46, Donn wrote: > We need to include the {google:contextualSearchUrl} here too. I know we're not > really using this for anything now, but I think we will be able to use it very > soon and it will be very helpful. > > Also it looks like the contextualSearchVersion is not listed on this line. I > think we probably want it here too, but not sure. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:110: const char kGoogleContextualSearchContent[] = "google:contextualSearchContent"; On 2014/06/02 18:09:46, Donn wrote: > Add the contextualSearchUrl here too. Done. https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/40001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:113: On 2014/06/02 18:09:46, Donn wrote: > On 2014/06/02 17:39:51, pedrosimonetti wrote: > > What about the page's URL? > > +1 Done.
Hi Peter - can you please review this?
https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/prepopulated_engines.json:533: "contextual_search_url": "{google:baseURL}_/contextualsearch?{google:contextualSearchVersion}{google:contextualSearchStart}{google:contextualSearchEnd}{google:contextualSearchContent}{google:contextualSearchBasePageURL}{google:contextualSearchEncoding}", There are a couple of alternates to consider here: (1) If any of these args are mandatory, consider listing the param name explicitly here, and only using the substitution string to hold the value -- e.g. how the suggest_url above treats the "client" or "gs_ri" or "sugkey" params. (2) Alternately, consider simply making the URL here something like: {google:baseURL}_/contextualsearch?{google:contextualSearchVersion}{google:contextualSearchParams} ...and then having the TemplateURL expand that last into all the different params necessary. This makes for simpler code in TemplateURL. (We could probably make use of this trick for some of the other URLs above more than we currently do.) https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:965: DCHECK(!i->is_post_param); Why are you putting this on all these cases? It seems like there's nothing about any of your params that will break if they're instead sent via POST, so these DCHECKs seem wrong. (Some of the DCHECKs elsewhere in here look wrong too, in particular the ones in GOOGLE_CURRENT_PAGE_URL, GOOGLE_OMNIBOX_START_MARGIN, GOOGLE_ORIGINAL_QUERY_FOR_SUGGESTION, GOOGLE_RLZ, and GOOGLE_SEARCH_CLIENT). https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:116: Maybe all these should be in some sub-struct to group them. This would allow you to init all of these with a single constructor call instead of six separate assignments, in addition to hopefully improving readability a little. (We probably should do the same with the image search-specific params.) Really, this sort of bloating of SearchTermsArgs to contain the union of all options for all different kinds of substitutable URLs suggests that perhaps we should consider having multiple different kinds of SearchTermsArgs structs, which are supplied to separate API functions to expand different types of search URLs. Maybe those APIs should be exposed only on subclasses of TemplateURLRef, e.g. ContextualSearchTemplateURLRef. This hoists some of the semantic differences between these kinds of URLs into the type system, which might make code easier to understand or safer (?). I'm not sure, I just don't like what's happening with the current route. https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:1384: "ctxs=1" Nit: Indent even
Appreciate the comments. We'll need to construct a contextual search request from our Java code, so I added that change here. https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/prepopulated_engines.json:533: "contextual_search_url": "{google:baseURL}_/contextualsearch?{google:contextualSearchVersion}{google:contextualSearchStart}{google:contextualSearchEnd}{google:contextualSearchContent}{google:contextualSearchBasePageURL}{google:contextualSearchEncoding}", Went with (1), since several are mandatory and this makes that explicit. On 2014/06/03 21:47:09, Peter Kasting wrote: > There are a couple of alternates to consider here: > > (1) If any of these args are mandatory, consider listing the param name > explicitly here, and only using the substitution string to hold the value -- > e.g. how the suggest_url above treats the "client" or "gs_ri" or "sugkey" > params. > (2) Alternately, consider simply making the URL here something like: > > {google:baseURL}_/contextualsearch?{google:contextualSearchVersion}{google:contextualSearchParams} > > ...and then having the TemplateURL expand that last into all the different > params necessary. This makes for simpler code in TemplateURL. (We could > probably make use of this trick for some of the other URLs above more than we > currently do.) https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:965: DCHECK(!i->is_post_param); Done. In fact, we may ultimately send this as a POST request. On 2014/06/03 21:47:09, Peter Kasting wrote: > Why are you putting this on all these cases? It seems like there's nothing > about any of your params that will break if they're instead sent via POST, so > these DCHECKs seem wrong. > > (Some of the DCHECKs elsewhere in here look wrong too, in particular the ones in > GOOGLE_CURRENT_PAGE_URL, GOOGLE_OMNIBOX_START_MARGIN, > GOOGLE_ORIGINAL_QUERY_FOR_SUGGESTION, GOOGLE_RLZ, and GOOGLE_SEARCH_CLIENT). https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:116: Done. On 2014/06/03 21:47:09, Peter Kasting wrote: > Maybe all these should be in some sub-struct to group them. This would allow > you to init all of these with a single constructor call instead of six separate > assignments, in addition to hopefully improving readability a little. (We > probably should do the same with the image search-specific params.) > > Really, this sort of bloating of SearchTermsArgs to contain the union of all > options for all different kinds of substitutable URLs suggests that perhaps we > should consider having multiple different kinds of SearchTermsArgs structs, > which are supplied to separate API functions to expand different types of search > URLs. Maybe those APIs should be exposed only on subclasses of TemplateURLRef, > e.g. ContextualSearchTemplateURLRef. This hoists some of the semantic > differences between these kinds of URLs into the type system, which might make > code easier to understand or safer (?). I'm not sure, I just don't like what's > happening with the current route. https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:1384: "ctxs=1" On 2014/06/03 21:47:09, Peter Kasting wrote: > Nit: Indent even Done.
https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:965: DCHECK(!i->is_post_param); On 2014/06/04 00:22:11, jeremycho wrote: > Done. In fact, we may ultimately send this as a POST request. Unfortunately, you changed several of these to only replacing the value here, and listing the param name inside the URL in prepopulated_engines.json. That's not compatible with having these be POST params, and you need to restore the DCHECKs on anything where you call HandleReplacement() with std::string() as the first arg. If you really want to support POST for all this stuff, you need to restore the previous code, or better yet use my suggestion that you simply have a single replacement string that inserts the value of almost all these. Then it can be coded here as a set of several sequential HandleReplacement() calls, saving code as well as not exploding the replacement type enum quite as much. https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/prepopulated_engines.json:533: "contextual_search_url": "{google:baseURL}_/contextualsearch?{google:contextualSearchVersion}&ctxs_start={google:contextualSearchStart}&ctxs_end={google:contextualSearchEnd}&ctxs_content={google:contextualSearchContent}{google:contextualSearchBasePageURL}{google:contextualSearchEncoding}", I don't think you want an ampersand after {google:contextualSearchVersion}, since I believe it puts in its own. Whereas you do want an ampersand before {google:contextualSearchBasePageURL}. In order to avoid ending the URL with an ampersand, we usually choose to list one of the named params (that won't expand to something with a trailing ampersand) last. https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:75: ContextualSearchParams() : version(-1), start(-1), end(-1) { It's surprising you don't have a 6-arg variant to set all these at once. Isn't that what most callers who change any of these will want? See e.g. template_url_unittest.cc. This can also enforce that all the args that you want set are set to valid/nonempty values. Also, define the constructor in the .cc file for structs/classes that contain non-basic-type members. https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:82: int start; These should probably be size_ts and use npos to represent "no value". https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:1386: "&ctxs_start=12" Nit: Place ampersands on the ends of previous lines, as that's how template_url.cc expands them, so it's clearer to read that way.
https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:965: DCHECK(!i->is_post_param); Not sure I follow how the single replacement string would work. Using HandleReplacement on each post parameter requires them to have indices into post_params_. So don't these still need to be added to the big else if block in TemplateURLRef::ParseParameter? On 2014/06/04 00:40:49, Peter Kasting wrote: > On 2014/06/04 00:22:11, jeremycho wrote: > > Done. In fact, we may ultimately send this as a POST request. > > Unfortunately, you changed several of these to only replacing the value here, > and listing the param name inside the URL in prepopulated_engines.json. That's > not compatible with having these be POST params, and you need to restore the > DCHECKs on anything where you call HandleReplacement() with std::string() as the > first arg. > > If you really want to support POST for all this stuff, you need to restore the > previous code, or better yet use my suggestion that you simply have a single > replacement string that inserts the value of almost all these. Then it can be > coded here as a set of several sequential HandleReplacement() calls, saving code > as well as not exploding the replacement type enum quite as much.
https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/60001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:965: DCHECK(!i->is_post_param); On 2014/06/04 03:33:04, jeremycho wrote: > Not sure I follow how the single replacement string would work. Using > HandleReplacement on each post parameter requires them to have indices into > post_params_. So don't these still need to be added to the big else if block in > TemplateURLRef::ParseParameter? Frankly, I don't know. I didn't write any of the POST stuff and AFAIK it doesn't really work right anyway. Certainly jnd@ (who wrote it) never bothered to get it working properly for anything other than some of the image stuff he was trying to implement. Now that I'm staring at it again, I'm becoming less and less certain that I actually understand how any of it works. You wouldn't know I reviewed all this once upon a time... It looks from inspection like you'd first have to add a contextual search case to GetPostParamsString(), then probably modify ParseURL() a bit to understand how to handle this kind of thing. What I would do for now is probably drop the idea of POST entirely (get GET working first), and aim for the simplest possible code. IMO having one replacement param instead of n is simpler, but if you really want to do it the other way, I don't care too much. I am no longer certain whether or not the DCHECKs about things not being post params are correct.
On 2014/06/04 06:56:21, Peter Kasting wrote: > What I would do for now is probably drop the idea of POST entirely (get GET > working first), and aim for the simplest possible code. IMO having one > replacement param instead of n is simpler, but if you really want to do it the > other way, I don't care too much. I am no longer certain whether or not the > DCHECKs about things not being post params are correct. Jeremy, I agree with Peter about using GET and now worrying about POST: Even though we have a bug open to use POST, things have changed recently and now we want to use the HTTP headers to pass the context just like Genie does. I'll update that bug.
On 2014/06/04 17:43:37, Donn wrote: > On 2014/06/04 06:56:21, Peter Kasting wrote: > > What I would do for now is probably drop the idea of POST entirely (get GET > > working first), and aim for the simplest possible code. IMO having one > > replacement param instead of n is simpler, but if you really want to do it the > > other way, I don't care too much. I am no longer certain whether or not the > > DCHECKs about things not being post params are correct. > Jeremy, I agree with Peter about using GET and now worrying about POST: Even > though we have a bug open to use POST, things have changed recently and now we > want to use the HTTP headers to pass the context just like Genie does. I'll > update that bug. Switched to supporting GET only and using a single replacement param. PTAL.
Hi David, can you please review for TemplateUrlService.java? https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/prepopulated_engines.json:533: "contextual_search_url": "{google:baseURL}_/contextualsearch?{google:contextualSearchVersion}&ctxs_start={google:contextualSearchStart}&ctxs_end={google:contextualSearchEnd}&ctxs_content={google:contextualSearchContent}{google:contextualSearchBasePageURL}{google:contextualSearchEncoding}", Removed named params. On 2014/06/04 00:40:49, Peter Kasting wrote: > I don't think you want an ampersand after {google:contextualSearchVersion}, > since I believe it puts in its own. Whereas you do want an ampersand before > {google:contextualSearchBasePageURL}. In order to avoid ending the URL with an > ampersand, we usually choose to list one of the named params (that won't expand > to something with a trailing ampersand) last. https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:75: ContextualSearchParams() : version(-1), start(-1), end(-1) { If it's being used in a /search, then only version should be set. I'm not sure now which will remain mandatory, so I've chosen to default to invalid values and checking them when building the URL. On 2014/06/04 00:40:49, Peter Kasting wrote: > It's surprising you don't have a 6-arg variant to set all these at once. Isn't > that what most callers who change any of these will want? See e.g. > template_url_unittest.cc. This can also enforce that all the args that you want > set are set to valid/nonempty values. > > Also, define the constructor in the .cc file for structs/classes that contain > non-basic-type members. https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/template_url.h:82: int start; On 2014/06/04 00:40:49, Peter Kasting wrote: > These should probably be size_ts and use npos to represent "no value". Done. https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/308053009/diff/80001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:1386: "&ctxs_start=12" On 2014/06/04 00:40:49, Peter Kasting wrote: > Nit: Place ampersands on the ends of previous lines, as that's how > template_url.cc expands them, so it's clearer to read that way. Done.
lgtm
TemplateUrlService.java lgtm
Friendly ping.
Sorry, I've had a lot of review requests. I'll try to re-review tomorrow.
LGTM https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.cc:105: // Contextual search parameters. Nit: It seems like these and the image search constants just above should be rolled into the larger alphabetized list above them. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.cc:965: if (search_terms_args.contextual_search_params.start != Nit: If above all these you do something like this: const ContextualSearchParams& params = search_terms_args.contextual_search_params; Then you can use "params.<member>" everywhere below, reducing wordiness and allowing you to eliminate the "const std::string" temps that you create just to shorten the names. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.h:80: // Offset into content of the start of the user selection. Nit: Should "content" be "the page content" everywhere? https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.h:303: GOOGLE_CONTEXTUAL_SEARCH_CONTEXT_DATA, Nit: Should this just be named GOOGLE_CONTEXTUAL_SEARCH_PARAMS (and similarly everywhere else)? That would be both shorter and matching the struct name. Or maybe everything should be named "contextual search data" (i.e. drop "context")? https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:206: GURL gurl = GetDefaultSearchURLForSearchTerms(GetOriginalProfile(), query); Nit: Consider using () instead of = to construct non-basic types (just to emphasize to the reader that they're of class type) https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1370: search_terms_args.contextual_search_params.version = 1; Nit: I still think the presence of these long lists of args in this unittest suggests that you should add a six-arg constructor to ContextualSearchParams. You could even make it #ifdef UNIT_TEST if you really wanted non-test code to not use it. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1380: EXPECT_EQ("http://bar/_/contextualsearch?" Nit: Consider indenting like this: EXPECT_EQ("http://bar/_/contextualsearch?" "ctxs=1&" ..., result); ...to emphasize that the strings continued the first arg, and then |result| is a separate arg. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1390: search_terms_args.contextual_search_params.version = -1; Nit: Should we do this test first? Then there'd be no need to set all these members, they'd be in this state already from construction. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1393: search_terms_args.contextual_search_params.selection = ""; Nit: Use .clear(), or in the limit "= std::string()", rather than "".
Thanks for the reviews. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.cc:105: // Contextual search parameters. On 2014/06/06 17:51:33, Peter Kasting wrote: > Nit: It seems like these and the image search constants just above should be > rolled into the larger alphabetized list above them. Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.cc:965: if (search_terms_args.contextual_search_params.start != On 2014/06/06 17:51:33, Peter Kasting wrote: > Nit: If above all these you do something like this: > > const ContextualSearchParams& params = > search_terms_args.contextual_search_params; > > Then you can use "params.<member>" everywhere below, reducing wordiness and > allowing you to eliminate the "const std::string" temps that you create just to > shorten the names. Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url.h (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.h:80: // Offset into content of the start of the user selection. On 2014/06/06 17:51:33, Peter Kasting wrote: > Nit: Should "content" be "the page content" everywhere? Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url.h:303: GOOGLE_CONTEXTUAL_SEARCH_CONTEXT_DATA, On 2014/06/06 17:51:33, Peter Kasting wrote: > Nit: Should this just be named GOOGLE_CONTEXTUAL_SEARCH_PARAMS (and similarly > everywhere else)? That would be both shorter and matching the struct name. Or > maybe everything should be named "contextual search data" (i.e. drop "context")? The struct also contains version, which is why I named this something else to avoid associating the two. "Context" was used since this data does relate to the user's context (what they were looking at, etc). https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:206: GURL gurl = GetDefaultSearchURLForSearchTerms(GetOriginalProfile(), query); On 2014/06/06 17:51:33, Peter Kasting wrote: > Nit: Consider using () instead of = to construct non-basic types (just to > emphasize to the reader that they're of class type) Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1370: search_terms_args.contextual_search_params.version = 1; On 2014/06/06 17:51:34, Peter Kasting wrote: > Nit: I still think the presence of these long lists of args in this unittest > suggests that you should add a six-arg constructor to ContextualSearchParams. > You could even make it #ifdef UNIT_TEST if you really wanted non-test code to > not use it. Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1380: EXPECT_EQ("http://bar/_/contextualsearch?" On 2014/06/06 17:51:34, Peter Kasting wrote: > Nit: Consider indenting like this: > > EXPECT_EQ("http://bar/_/contextualsearch?" > "ctxs=1&" > ..., > result); > > ...to emphasize that the strings continued the first arg, and then |result| is a > separate arg. Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1390: search_terms_args.contextual_search_params.version = -1; On 2014/06/06 17:51:34, Peter Kasting wrote: > Nit: Should we do this test first? Then there'd be no need to set all these > members, they'd be in this state already from construction. Done. https://codereview.chromium.org/308053009/diff/160001/chrome/browser/search_e... chrome/browser/search_engines/template_url_unittest.cc:1393: search_terms_args.contextual_search_params.selection = ""; On 2014/06/06 17:51:33, Peter Kasting wrote: > Nit: Use .clear(), or in the limit "= std::string()", rather than "". Removed.
The CQ bit was checked by jeremycho@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/308053009/130011
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/14149)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
The CQ bit was checked by jeremycho@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/308053009/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15621) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18759)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by jeremycho@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/308053009/210001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15686) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18820)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15723) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18856)
The CQ bit was checked by jeremycho@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/308053009/230001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/19096)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was checked by jeremycho@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/308053009/250001
Message was sent while issue was closed.
Change committed as 276874 |
