Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10537154:
A working implementation of AQS (Assisted Query Stats). (Closed)
|
Created:
8 years, 6 months ago by Bart N Modified:
8 years, 6 months ago CC:
chromium-reviews, James Su, Abhi D, tfarina Visibility:
Public. |
DescriptionA working implementation of AQS (Assisted Query Stats).
BUG=132667
TEST=AutocompleteProviderTest::AssistedQueryStats,AutocompleteProviderTest::UpdateAssistedQueryStats, TemplateURLTest::ReplaceAssistedQueryStats, manual testing.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143827
Patch Set 1 #
Total comments: 27
Patch Set 2 : Addressed review feedback. #
Total comments: 1
Patch Set 3 : Add missing change to google engine. #
Total comments: 12
Patch Set 4 : Introduced SearchTermsArgs, cleaned up code and addressed all comments. #Patch Set 5 : Removed SupportsAssistedQueryStats. #
Total comments: 41
Patch Set 6 : Addressed Peter's comments. #Patch Set 7 : Added Autocomplete unit tests. #
Total comments: 25
Patch Set 8 : Addressed comments. #
Total comments: 2
Patch Set 9 : Moved kMaxRelevance inside the function which uses it. #
Total comments: 12
Patch Set 10 : Addressed comments. #
Total comments: 2
Patch Set 11 : Fix compilation errors. #Patch Set 12 : Addressed remaining comments. #Patch Set 13 : Fixed Win compilation error. #
Total comments: 2
Patch Set 14 : Fixed InstantTest.OnChangeEvent failure; addressed remaining commments. #
Total comments: 2
Patch Set 15 : Added HTTPS checks, as requested by privacy folks. #
Total comments: 5
Patch Set 16 : Addressed comments. #
Total comments: 4
Patch Set 17 : Addressed comments and added more docs. #Messages
Total messages: 45 (0 generated)
Some preliminary feedback. You'll ultimately need Peter's approval. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1105: namespace { anon namespaces and contents belong at the top of the file; ~line 57 https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1107: // Converts the given type to an integer based on the AQS specification. Are these magic numbers really necessary? Why not use the existing AutocompleteMatch::Type values? https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1108: int AutocompletMatchToAssistedQueryType(const AutocompleteMatch::Type& type) { Spelling: Autocomplet*e*MatchToAssistedQueryType https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1150: std::string aqs; Use string16 if possible. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1153: for (ACMatches::iterator match(result->begin()); match != result->end(); Is there a reason for combining consecutive matches by type here? Does it make sense to just list each match type here and analyze them as needed offline? https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1164: AppendQueryStats(last_type, count, &aqs); Should this be done even if there were no matches? https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1168: for (AutocompleteResult::iterator i = result->begin(); i != result->end(); nit: consistency is nice, consider matching your loop above. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1170: if (i->provider != search_provider_) It'd be nice if we could put this logic in SearchProvider match generation itself, but I guess that doesn't work if we need to know the final aggregated ordering of all the match types from all the providers. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1178: url.append(base::StringPrintf("&aqs=%d", index)).append(".").append(aqs); Whoa, we're sending the ordered list of match types to the search provider? Has that gone through privacy review? https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1181: VLOG(1) << "[" << i->provider->name() << "] URL = " << i->destination_url; Will this LOG help end users? if not, please remove. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/search_... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/search_... chrome/browser/search_engines/template_url_prepopulate_data.cc:3445: nit: remove blank line at the end.
PTAL https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1105: namespace { On 2012/06/13 22:39:33, msw wrote: > anon namespaces and contents belong at the top of the file; ~line 57 Done. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1107: // Converts the given type to an integer based on the AQS specification. On 2012/06/13 22:39:33, msw wrote: > Are these magic numbers really necessary? Why not use the existing > AutocompleteMatch::Type values? These values must map to existing enum on the server side. I can't reuse existing values. One question though - should I create constants for these values and put it somewhere in anon namespace? https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1108: int AutocompletMatchToAssistedQueryType(const AutocompleteMatch::Type& type) { On 2012/06/13 22:39:33, msw wrote: > Spelling: Autocomplet*e*MatchToAssistedQueryType Done. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1150: std::string aqs; On 2012/06/13 22:39:33, msw wrote: > Use string16 if possible. Hmmm.. a few methods broke when I switched to string16. For instance, I'm using StringAppendF which doesn't support string16, is there any alternative for that? https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1153: for (ACMatches::iterator match(result->begin()); match != result->end(); On 2012/06/13 22:39:33, msw wrote: > Is there a reason for combining consecutive matches by type here? Does it make > sense to just list each match type here and analyze them as needed offline? Yes, it's required by the spec. For instance, instead of 0j0j0, we have to send 0l3 (length=3). https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1164: AppendQueryStats(last_type, count, &aqs); On 2012/06/13 22:39:33, msw wrote: > Should this be done even if there were no matches? There is always at least one. See the check at the top of this function. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1168: for (AutocompleteResult::iterator i = result->begin(); i != result->end(); On 2012/06/13 22:39:33, msw wrote: > nit: consistency is nice, consider matching your loop above. Ah yes, done. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1170: if (i->provider != search_provider_) On 2012/06/13 22:39:33, msw wrote: > It'd be nice if we could put this logic in SearchProvider match generation > itself, but I guess that doesn't work if we need to know the final aggregated > ordering of all the match types from all the providers. Currently, I rely on the fact that we check for SupportsAssistedQueryStats. Actually, this check may be entirely unnecessary, because only Google Provider, which is a SearchProvider, will have it set. Anyway, let me know. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1178: url.append(base::StringPrintf("&aqs=%d", index)).append(".").append(aqs); On 2012/06/13 22:39:33, msw wrote: > Whoa, we're sending the ordered list of match types to the search provider? Has > that gone through privacy review? Yes. It was approved today, I can forward an email if you are interested in. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1181: VLOG(1) << "[" << i->provider->name() << "] URL = " << i->destination_url; On 2012/06/13 22:39:33, msw wrote: > Will this LOG help end users? if not, please remove. Removed.
https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1107: // Converts the given type to an integer based on the AQS specification. >should I create constants I wouldn't bother, but maybe add a vague statement about where they map from. (can't link to server code, but you can say "These numbers match those in server code, see <enum name>." or similar. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1150: std::string aqs; On 2012/06/13 23:17:13, Bart N wrote: > On 2012/06/13 22:39:33, msw wrote: > > Use string16 if possible. > Hmmm.. a few methods broke when I switched to string16. For instance, I'm using > StringAppendF which doesn't support string16, is there any alternative for that? Use + and += operators, IIRC. See base/string16.h and base/string_util.h, etc. for more as needed. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1153: for (ACMatches::iterator match(result->begin()); match != result->end(); On 2012/06/13 23:17:13, Bart N wrote: > On 2012/06/13 22:39:33, msw wrote: > > Is there a reason for combining consecutive matches by type here? Does it make > > sense to just list each match type here and analyze them as needed offline? > Yes, it's required by the spec. For instance, instead of 0j0j0, we have to send > 0l3 (length=3). Fair enough. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1164: AppendQueryStats(last_type, count, &aqs); On 2012/06/13 23:17:13, Bart N wrote: > On 2012/06/13 22:39:33, msw wrote: > > Should this be done even if there were no matches? > There is always at least one. See the check at the top of this function. Doh! Thanks for the heads up on the empty check. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1170: if (i->provider != search_provider_) On 2012/06/13 23:17:13, Bart N wrote: > On 2012/06/13 22:39:33, msw wrote: > > It'd be nice if we could put this logic in SearchProvider match generation > > itself, but I guess that doesn't work if we need to know the final aggregated > > ordering of all the match types from all the providers. > Currently, I rely on the fact that we check for SupportsAssistedQueryStats. > Actually, this check may be entirely unnecessary, because only Google Provider, > which is a SearchProvider, will have it set. Anyway, let me know. > So, I'm not terribly familiar with AQS or what you're trying to achieve. It looks like you need data about all the aggregated matches from various providers, right? If so, then this seems to be the right place for this code. If you only need to know about the matches that the SearchProvider creates (and not know for example, that a history match listed above/between them, or some search matches weren't included in the displayed set, etc.), then move this code to SearchProvider. https://chromiumcodereview.appspot.com/10537154/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/autocomplete.cc:1178: url.append(base::StringPrintf("&aqs=%d", index)).append(".").append(aqs); On 2012/06/13 23:17:13, Bart N wrote: > On 2012/06/13 22:39:33, msw wrote: > > Whoa, we're sending the ordered list of match types to the search provider? > Has > > that gone through privacy review? > Yes. It was approved today, I can forward an email if you are interested in. > You should link this CL to a chromium bug (set BUG=###### in the CL description). In my experience, there's security/privacy review tags there (or in a corresponding bug) as needed; that'd bring peace of mind to your reviewers :) https://chromiumcodereview.appspot.com/10537154/diff/5001/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/5001/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:1178: url.append(base::StringPrintf("&aqs=%d", index)).append(".").append(aqs); Won't this append a second "&aqs=", since one was already found in the URL? Should you assign a value to the already-present parameter?
https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:59: // Converts the given type to an integer based on the AQS specification. We either need a copy of or link to the spec, or a copy of the actual table of values as constants, or something. These magic numbers are too magic. https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:62: case AutocompleteMatch::SEARCH_SUGGEST: Nit: I suggest putting the return statements on the same lines as the case statements, all aligned https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:1166: // Go over all matches and set AQS if the match supports it. Don't do things this way. Instead let TemplateURLRef substitute the correct string into the URL, and pass the data you need to it (by which I mean, a const AutocompleteResult*) through ReplaceSearchTerms(), the way we currently do for the existing suggest data. Now, implementing this will be a bit of a trick. I suggest adding a normally-empty string16 member on AutocompleteMatch called |query_for_later_replace|, and where SearchProvider() currently calls ReplaceSearchTerms(), instead omit the call and set this member. Then this function (UpdateAssistedQueryStats) can simply loop through all matches, and for each one with a non-empty value here, call ReplaceSearchTerms(match.query_for_later_replace, result). This will also eliminate the need for the SupportsAssistedQueryStats() method you've added. Note that this comment assumes we're replacing the existing aq/oq stuff with aqs, see comment elsewhere. https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... chrome/browser/search_engines/template_url.cc:130: default: Don't muck with this, please https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... chrome/browser/search_engines/template_url_prepopulate_data.cc:1093: "{google:acceptedSuggestion}{google:originalQueryForSuggestion}" Why do we still have acceptedSuggestion and originalQueryForSuggestion? Doesn't this replace them?
Thanks for the review, Peter. Just replied with high level comment, as I think everything else is non-questionable and I can address it quickly. Also, general question - do we want to expose Google specific internal protos, where we currently store the ID mappings? https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:1166: // Go over all matches and set AQS if the match supports it. On 2012/06/14 01:04:23, Peter Kasting wrote: > Don't do things this way. Instead let TemplateURLRef substitute the correct > string into the URL, and pass the data you need to it (by which I mean, a const > AutocompleteResult*) through ReplaceSearchTerms(), the way we currently do for > the existing suggest data. I really wanted to do it that way, but I ran into several issues. > > Now, implementing this will be a bit of a trick. I suggest adding a > normally-empty string16 member on AutocompleteMatch called > |query_for_later_replace|, and where SearchProvider() currently calls > ReplaceSearchTerms(), instead omit the call and set this member. Then this > function (UpdateAssistedQueryStats) can simply loop through all matches, and for > each one with a non-empty value here, call > ReplaceSearchTerms(match.query_for_later_replace, result). I don't think this would work. The problem is that AQS string can only be computed in this method, as it is order sensitive and needs global access to all providers. On the other hand, we still need to replace all other params (e.g. oq, search terms etc.), which requires knowing the substitutes, currently only known locally (at the time when ReplaceSearchTerms is called). Probably a better approach would be to decouple setting replacements from actually applying them. For instance, we could set template params instead of calling ReplaceSearchTerms in two places: (1) as they are currently (in search provider) (2) in this method (aqs) Finally, we could call something like "ApplyReplacements" (or similar) which would essentially go over all params and map them onto the final URL. This would also imply that we don't set destination_url until the very last moment, which would be here, but I can't tell if there are any consequences of doing so... > > This will also eliminate the need for the SupportsAssistedQueryStats() method > you've added. > > Note that this comment assumes we're replacing the existing aq/oq stuff with > aqs, see comment elsewhere. Unfortunately, we still need oq stuff and aqs alone is not sufficient. See my proposal above.
Regarding Google-specific protos: I don't personally have a problem with copying pieces of them into the public tree, but it's usually not my call to make. Make sure you OK whatever you bring in with some appropriate authority :) https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:1166: // Go over all matches and set AQS if the match supports it. On 2012/06/14 01:22:13, Bart N wrote: > I don't think this would work. The problem is that AQS string can only be > computed in this method, as it is order sensitive and needs global access to all > providers. On the other hand, we still need to replace all other params (e.g. > oq, search terms etc.), which requires knowing the substitutes, currently only > known locally (at the time when ReplaceSearchTerms is called). Right; I was attempting to address that, but assumed only the query string needed to be preserved of the three existing args. It sounds like the "oq" needs to be preserved too (see bottom of this comment). There are two ways to handle this. One is to use a route like I've suggested and simply preserve the "oq" on the AutocompleteMatch as well. The other is to do something like you've suggested and cache the "query" and "oq" values in the TemplateURLRef, making it stateful, then allow a caller to get back a URL with those values substituted in later. Practically speaking, these are really almost the same, they just differ in where the state is preserved. Because TemplateURLRef doesn't currently have any state like this, and it has a lot of different users all over the codebase (while AutocompleteMatch is restricted to just this subsystem), I'd feel a little better if the state was preserved on AutocompleteMatch. Now, if keeping two members instead of one feels inelegant, there's another change we could make; we could define a struct (SearchTermsArgs) that ReplaceSearchTerms() can take, and that AutocompleteMatch can keep as a scoped_ptr (so NULL means no need to call ReplaceSearchTerms() here). Then if we give the struct a one-arg constructor, we can replace almost all the calls in the rest of the codebase, which currently do something akin to "ReplaceSearchTerms(terms, NO_SUGGESTIONS_AVAILABLE, std::string())" with the somewhat-more-readable-and-less-confusing "ReplaceSearchTerms(SearchTermsArgs(terms))". This makes the overall change a bit less hacky, at the cost of requiring you to touch more code :) > > Note that this comment assumes we're replacing the existing aq/oq stuff with > > aqs, see comment elsewhere. > Unfortunately, we still need oq stuff and aqs alone is not sufficient. See my > proposal above. Can we at least eliminate the stuff dealing with aq, leaving oq and aqs?
On Wed, Jun 13, 2012 at 8:18 PM, <pkasting@chromium.org> wrote: > Regarding Google-specific protos: I don't personally have a problem with > copying > pieces of them into the public tree, but it's usually not my call to make. > Make > sure you OK whatever you bring in with some appropriate authority :) > > > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 1006/chrome/browser/**autocomplete/autocomplete.cc<https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/autocomplete/autocomplete.cc> > File chrome/browser/autocomplete/**autocomplete.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 1006/chrome/browser/**autocomplete/autocomplete.cc#**newcode1166<https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/autocomplete/autocomplete.cc#newcode1166> > chrome/browser/autocomplete/**autocomplete.cc:1166: // Go over all matches > and set AQS if the match supports it. > On 2012/06/14 01:22:13, Bart N wrote: > >> I don't think this would work. The problem is that AQS string can only >> > be > >> computed in this method, as it is order sensitive and needs global >> > access to all > >> providers. On the other hand, we still need to replace all other >> > params (e.g. > >> oq, search terms etc.), which requires knowing the substitutes, >> > currently only > >> known locally (at the time when ReplaceSearchTerms is called). >> > > Right; I was attempting to address that, but assumed only the query > string needed to be preserved of the three existing args. It sounds > like the "oq" needs to be preserved too (see bottom of this comment). > > There are two ways to handle this. One is to use a route like I've > suggested and simply preserve the "oq" on the AutocompleteMatch as well. > The other is to do something like you've suggested and cache the > "query" and "oq" values in the TemplateURLRef, making it stateful, then > allow a caller to get back a URL with those values substituted in later. > > Practically speaking, these are really almost the same, they just differ > in where the state is preserved. Because TemplateURLRef doesn't > currently have any state like this, and it has a lot of different users > all over the codebase (while AutocompleteMatch is restricted to just > this subsystem), I'd feel a little better if the state was preserved on > AutocompleteMatch. > > Now, if keeping two members instead of one feels inelegant, there's > another change we could make; we could define a struct (SearchTermsArgs) > that ReplaceSearchTerms() can take, and that AutocompleteMatch can keep > as a scoped_ptr (so NULL means no need to call ReplaceSearchTerms() > here). Then if we give the struct a one-arg constructor, we can replace > almost all the calls in the rest of the codebase, which currently do > something akin to "ReplaceSearchTerms(terms, NO_SUGGESTIONS_AVAILABLE, > std::string())" with the somewhat-more-readable-and-**less-confusing > "ReplaceSearchTerms(**SearchTermsArgs(terms))". This makes the overall > change a bit less hacky, at the cost of requiring you to touch more code > :) I like it to be less hacky and more readable, so I think I'll try this approach and ping back when done. Just please let me know if you believe that this may significantly jeopardize chances of having this change submitted before the deadline. Thanks for the quick reviews! > > > > Note that this comment assumes we're replacing the existing aq/oq >> > stuff with > >> > aqs, see comment elsewhere. >> Unfortunately, we still need oq stuff and aqs alone is not sufficient. >> > See my > >> proposal above. >> > > Can we at least eliminate the stuff dealing with aq, leaving oq and aqs? > The one and only reason why I want to keep aq is to make sure we don't have to update all logging related libraries. I was thinking of keeping it for a while and then deprecate aq in M22. What do you think? I don't think too strong about it, since aq is broken anyway... > > https://chromiumcodereview.**appspot.com/10537154/<https://chromiumcodereview... >
On 2012/06/14 03:51:32, Bart N wrote: > The one and only reason why I want to keep aq is to make sure we don't have > to update all logging related libraries. I was thinking of keeping it for a > while and then deprecate aq in M22. What do you think? If we go the struct route, then keeping an extra member in it won't have much impact on the rest of the world. I'm fine with leaving aq for now if you want, although please do file bugs etc. to make sure we clean this up eventually :) I wouldn't worry _too_ hard about the deadline, since I'll be continuing to do code reviews through then, and even if somehow we slip this, we can merge it across to the branch if it becomes important -- this change is plenty safe to do so.
Hi Peter, As we discussed I added a new struct, SearchTermsArgs and simplified the API. A few things: (1) I'm still use std::string in some places, let me know if it's OK (2) As we decided, I'll keep AQ for now, and it will be removed later (M22). (3) destination_url is instantiated twice - because it's needed for SortAndCull; once w/o AQS and finally with it (4) There are no unit tests, please let me know if you are OK with the code as is, and if so, I'll start adding them (5) The final AQS spec has been finalized and you will see a minor change to the format PTAL https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:59: // Converts the given type to an integer based on the AQS specification. On 2012/06/14 01:04:23, Peter Kasting wrote: > We either need a copy of or link to the spec, or a copy of the actual table of > values as constants, or something. These magic numbers are too magic. Done. https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:62: case AutocompleteMatch::SEARCH_SUGGEST: On 2012/06/14 01:04:23, Peter Kasting wrote: > Nit: I suggest putting the return statements on the same lines as the case > statements, all aligned Done. https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/auto... chrome/browser/autocomplete/autocomplete.cc:1166: // Go over all matches and set AQS if the match supports it. Done, as we discussed. On 2012/06/14 03:18:17, Peter Kasting wrote: > On 2012/06/14 01:22:13, Bart N wrote: > > I don't think this would work. The problem is that AQS string can only be > > computed in this method, as it is order sensitive and needs global access to > all > > providers. On the other hand, we still need to replace all other params (e.g. > > oq, search terms etc.), which requires knowing the substitutes, currently only > > known locally (at the time when ReplaceSearchTerms is called). > > Right; I was attempting to address that, but assumed only the query string > needed to be preserved of the three existing args. It sounds like the "oq" > needs to be preserved too (see bottom of this comment). > > There are two ways to handle this. One is to use a route like I've suggested > and simply preserve the "oq" on the AutocompleteMatch as well. The other is to > do something like you've suggested and cache the "query" and "oq" values in the > TemplateURLRef, making it stateful, then allow a caller to get back a URL with > those values substituted in later. > > Practically speaking, these are really almost the same, they just differ in > where the state is preserved. Because TemplateURLRef doesn't currently have any > state like this, and it has a lot of different users all over the codebase > (while AutocompleteMatch is restricted to just this subsystem), I'd feel a > little better if the state was preserved on AutocompleteMatch. > > Now, if keeping two members instead of one feels inelegant, there's another > change we could make; we could define a struct (SearchTermsArgs) that > ReplaceSearchTerms() can take, and that AutocompleteMatch can keep as a > scoped_ptr (so NULL means no need to call ReplaceSearchTerms() here). Then if > we give the struct a one-arg constructor, we can replace almost all the calls in > the rest of the codebase, which currently do something akin to > "ReplaceSearchTerms(terms, NO_SUGGESTIONS_AVAILABLE, std::string())" with the > somewhat-more-readable-and-less-confusing > "ReplaceSearchTerms(SearchTermsArgs(terms))". This makes the overall change a > bit less hacky, at the cost of requiring you to touch more code :) > > > > Note that this comment assumes we're replacing the existing aq/oq stuff with > > > aqs, see comment elsewhere. > > Unfortunately, we still need oq stuff and aqs alone is not sufficient. See my > > proposal above. > > Can we at least eliminate the stuff dealing with aq, leaving oq and aqs? https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... chrome/browser/search_engines/template_url.cc:130: default: On 2012/06/14 01:04:23, Peter Kasting wrote: > Don't muck with this, please I think I got some warning and fixed that. Anyway, reverted. The warning is: /usr/local/google/home/bartn/chrome/src/chrome/browser/search_engines/template_url.cc:137: More than one command on the same line [whitespace/newline] [4] https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/1006/chrome/browser/sear... chrome/browser/search_engines/template_url_prepopulate_data.cc:1093: "{google:acceptedSuggestion}{google:originalQueryForSuggestion}" On 2012/06/14 01:04:23, Peter Kasting wrote: > Why do we still have acceptedSuggestion and originalQueryForSuggestion? Doesn't > this replace them? As discussed, we need OQ, and we will keep AQ until we clean up on the backend side and then remove all remaining leftovers. Filed http://b/6669208.
The general idea here is good. You can go ahead with adding tests. I didn't see any strings I'd convert to string16s (did you have any at all?), but I did see a string16 I'd convert to a string -- the assisted_query_stats member itself. See below. Thanks for your flexibility here. The way this change is shaping up looks really nice to me. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:57: namespace { Nit: Because these functions are only used inside AutocompleteController, I suggest putting this whole anonymous namespace just below the "// AutocompleteController --" divider comment. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:60: // For more details, See http://go/binary-clients-logging. Nit: See->see; go->goto.google.com; I suggest a space before the trailing period just to be clear. Is it possible to eventually (maybe not before this lands) produce a sanitized version of the doc that can be made public, and then link that here instead? https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:78: void AppendAvailableAutocompletion(int type, int count, Nit: One arg per line (2 places) https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:92: aqs.append(".").append(base::StringPrintf("%d", query_index)).append(".") Nit: Why not just something like return base::StringPrintf("%s.%d.%s", client, query_index, available_autocompletions); (Note, I don't remember exactly how to do string args, like if there's a format type that means "std::string" or you have to pass .c_str() or whatever.) At this point, you could then just inline this into the caller side. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1167: // It's OK to call it, because there is always at least one match. Nit: I'd remove this comment (it seems clear enough from the top-of-function check) https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1172: for (ACMatches::iterator match(result->begin()); match != result->end(); Nit: Since you need to use an index anyway, just omit the iterator and use result->match_at(index). (Note this will mean you should switch |index| and the corresponding arg of ConstructAssistedQueryStatsString() to use size_t.) https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1174: if (match->provider != search_provider_) Nit: Omit this check; there's no theoretical reason why only the search provider can produce these. Checking for |search_terms_args| is good enough. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1179: SearchTermsArgs& search_terms_args = *match->search_terms_args.get(); Nit: We generally avoid non-const refs; I suggest either accessing match->search_terms_args directly or using a pointer as the temp. (I'd do the former.) https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_match.h (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_match.h:278: // Optional search terms arguments used to initialize destination_url above. Nit: How about: "Optional search terms args. If present, AutocompleteController::UpdateAssistedQueryStats() will incorporate this data with additional data it calculates and pass the completed struct to TemplateURLRef::ReplaceSearchTerms() to reset the match's |destination_url| after the complete set of matches in the AutocompleteResult has been chosen and sorted. Most providers will leave this as NULL, which will cause the AutocompleteController to do no additional transformations." I think this avoids the need for the second paragraph as well, but let me know if you think it's unclear. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/keyword_provider.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/keyword_provider.cc:398: match->destination_url = GURL(element_ref.ReplaceSearchTerms( Nit: Can this now be wrapped after '=' instead? https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/search_provider.cc:1064: // This is the initial replacement, which is necessary for URL dedupping Nit: How about: "This is the destination URL sans assisted query stats. This must be set so the AutocompleteController can properly de-dupe; the controller will eventually overwrite it before it reaches the user." https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/search_provider.cc:1066: match.destination_url = GURL(search_url.ReplaceSearchTerms( Nit: Can this be wrapped after '=' instead? https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/imp... File chrome/browser/importer/profile_writer.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/imp... chrome/browser/importer/profile_writer.cc:265: t_url->url_ref().ReplaceSearchTerms( Nit: This may fit on the prior line https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:216: if (!search_terms_args.assisted_query_stats.empty()) Nit: Needs {} https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:222: if (search_terms_args.accepted_suggestion == NO_SUGGESTION_CHOSEN) Nit: Needs {} https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:245: !search_terms_args.assisted_query_stats.empty()) Nit: Needs {} https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url.h (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.h:22: // SearchTermsArgs ------------------------------------------------------------ Nit: While I'm loathe to spoil the simplicity of the "SearchTermsArgs(...)" calls you've changed to everywhere else, I think this should be a member class of TemplateURLRef. This would serve to namespace it, which seems good (the name "SearchTermsArgs" is a bit generic to be floating in the global namespace, not that we don't have the same problem many other places...) It also emphasizes that it's sort of an implementation detail of how TemplateURLRef does search term construction. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.h:34: // Teh optional query stats, aka AQS, used for logging purposes. Nit: Teh->The https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.h:35: string16 assisted_query_stats; Use a std::string here. This isn't a UI string, and it's both created and ultimately used as a UTF-8 string, so converting to and from UTF-16 is just a waste. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url_prepopulate_data.cc:1094: "{google:assistedQueryStats}" Nit: I'd probably wrap after the "search field trial" block if it fits
Thanks. Addressed your comments and I'm working on unit tests now, should have them ready by the EOD. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:57: namespace { On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Because these functions are only used inside AutocompleteController, I > suggest putting this whole anonymous namespace just below the "// > AutocompleteController --" divider comment. Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:60: // For more details, See http://go/binary-clients-logging. Yes, we can have a sanitized version later on. Done. On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: See->see; go->goto.google.com; I suggest a space before the trailing period > just to be clear. > > Is it possible to eventually (maybe not before this lands) produce a sanitized > version of the doc that can be made public, and then link that here instead? https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:78: void AppendAvailableAutocompletion(int type, int count, On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: One arg per line (2 places) Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:92: aqs.append(".").append(base::StringPrintf("%d", query_index)).append(".") On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Why not just something like > > return base::StringPrintf("%s.%d.%s", client, query_index, > available_autocompletions); > > (Note, I don't remember exactly how to do string args, like if there's a format > type that means "std::string" or you have to pass .c_str() or whatever.) > > At this point, you could then just inline this into the caller side. Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1167: // It's OK to call it, because there is always at least one match. On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: I'd remove this comment (it seems clear enough from the top-of-function > check It wasn't clear to another reviewer, so I added it. Removed. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1172: for (ACMatches::iterator match(result->begin()); match != result->end(); On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Since you need to use an index anyway, just omit the iterator and use > result->match_at(index). (Note this will mean you should switch |index| and the > corresponding arg of ConstructAssistedQueryStatsString() to use size_t.) Done. Except size_t which is a pain to get proper % in printf. I also added another access to Result, since match_at returns const& https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1174: if (match->provider != search_provider_) On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Omit this check; there's no theoretical reason why only the search provider > can produce these. Checking for |search_terms_args| is good enough. Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:1179: SearchTermsArgs& search_terms_args = *match->search_terms_args.get(); On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: We generally avoid non-const refs; I suggest either accessing > match->search_terms_args directly or using a pointer as the temp. (I'd do the > former.) Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_match.h (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_match.h:278: // Optional search terms arguments used to initialize destination_url above. On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: How about: > > "Optional search terms args. If present, > AutocompleteController::UpdateAssistedQueryStats() will incorporate this data > with additional data it calculates and pass the completed struct to > TemplateURLRef::ReplaceSearchTerms() to reset the match's |destination_url| > after the complete set of matches in the AutocompleteResult has been chosen and > sorted. Most providers will leave this as NULL, which will cause the > AutocompleteController to do no additional transformations." > > I think this avoids the need for the second paragraph as well, but let me know > if you think it's unclear. Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/keyword_provider.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/keyword_provider.cc:398: match->destination_url = GURL(element_ref.ReplaceSearchTerms( On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Can this now be wrapped after '=' instead? Not after I moved Args inside URLRef. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/search_provider.cc:1064: // This is the initial replacement, which is necessary for URL dedupping On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: How about: > > "This is the destination URL sans assisted query stats. This must be set so the > AutocompleteController can properly de-dupe; the controller will eventually > overwrite it before it reaches the user." Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/aut... chrome/browser/autocomplete/search_provider.cc:1066: match.destination_url = GURL(search_url.ReplaceSearchTerms( On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Can this be wrapped after '=' instead? Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/imp... File chrome/browser/importer/profile_writer.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/imp... chrome/browser/importer/profile_writer.cc:265: t_url->url_ref().ReplaceSearchTerms( On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: This may fit on the prior line Not anymore. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:216: if (!search_terms_args.assisted_query_stats.empty()) On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Needs {} I'm not sure who wrote this code, but the style is pretty inconsistent. Check if/else flows elsewhere. Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:222: if (search_terms_args.accepted_suggestion == NO_SUGGESTION_CHOSEN) On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Needs {} Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:245: !search_terms_args.assisted_query_stats.empty()) On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Needs {} Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url.h (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.h:22: // SearchTermsArgs ------------------------------------------------------------ On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: While I'm loathe to spoil the simplicity of the "SearchTermsArgs(...)" > calls you've changed to everywhere else, I think this should be a member class > of TemplateURLRef. This would serve to namespace it, which seems good (the name > "SearchTermsArgs" is a bit generic to be floating in the global namespace, not > that we don't have the same problem many other places...) > > It also emphasizes that it's sort of an implementation detail of how > TemplateURLRef does search term construction. Yes. I also think it makes more sense. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.h:34: // Teh optional query stats, aka AQS, used for logging purposes. On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: Teh->The Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.h:35: string16 assisted_query_stats; On 2012/06/16 03:14:17, Peter Kasting wrote: > Use a std::string here. This isn't a UI string, and it's both created and > ultimately used as a UTF-8 string, so converting to and from UTF-16 is just a > waste. Done. https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url_prepopulate_data.cc:1094: "{google:assistedQueryStats}" On 2012/06/16 03:14:17, Peter Kasting wrote: > Nit: I'd probably wrap after the "search field trial" block if it fits Done.
Added tests to autocomplete_unittest.cc. It took my substantial time to figure out why template URL service wasn't returning my TemplateURL... it tuned out I was using templte_url->id() as a keyword... oh well.
On 2012/06/17 17:55:17, Bart N wrote: > Added tests to autocomplete_unittest.cc. It took my substantial time to figure > out why template URL service wasn't returning my TemplateURL... it tuned out I > was using templte_url->id() as a keyword... oh well. Ping? Is there any hope for this change to make into M21?
This looks like it's about ready. Have you run things through any trybots yet? Do you need me to do so? https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:216: if (!search_terms_args.assisted_query_stats.empty()) On 2012/06/16 23:38:10, Bart N wrote: > On 2012/06/16 03:14:17, Peter Kasting wrote: > > Nit: Needs {} > I'm not sure who wrote this code, but the style is pretty inconsistent. Check > if/else flows elsewhere. Yeah, the existing code was wrong. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:896: int query_index, Taking an int here results in a typecast (bad anyway + may warn on other OSes). Take a size_t, and make the StringPrintf() format string be: "%s.%" PRIuS ".%s" If this macro isn't defined you can get it from base/format_macros.h. Note: I still think it'd be just as clear -- and shorter overall -- to inline this StringPrintf() call in the caller and aliminate this helper function. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:41: const string16 match_keyword) Nit: One arg per line https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:79: // Generate one result synchronously, the rest later. This comment is now wrong? https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:197: // Register a TemplateURL for test keyword "t". Can we do this in the UpdateAssistedQueryStats test instead? https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:354: AutocompleteMatch match(NULL, -i, false, aqs_test_data[i].match_type); Nit: "-i" for a size_t is pretty weird, and risks overrunning any signal value that might be present at (-1). Can we just pick a constant for the highest relevance and then do (kMaxRelevance - i) instead? https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:433: EXPECT_TRUE(result_.match_at(i)->search_terms_args->assisted_query_stats Nit: Break after '(' instead (2 places) https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:508: RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data)); Nit: I think you can use arraysize() here instead since the struct type is named? (3 places) https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:509: SCOPED_TRACE("No matches"); Nit: Doesn't this need to be above the RunAssistedQueryStatsTest() call? (3 places) https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:110: // TemplateURLRef ------------------------------------------------------------- Nit: Move this below and put a "// TemplateURLRef::SearchTermsArgs ---" divider here. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... File chrome/browser/search_engines/template_url_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... chrome/browser/search_engines/template_url_unittest.cc:230: search_terms_data)); Nit: If this doesn't fit on the previous line, it should be indented the same as the line above, since it's not a second arg to the SearchTermsArgs constructor.
OK thanks, I'm updating the cl to incorporate your feedback. On Mon, Jun 18, 2012 at 12:45 PM, <pkasting@chromium.org> wrote: > This looks like it's about ready. Have you run things through any trybots > yet? > Do you need me to do so? Meanwhile, I'm getting troubles running git try: Sorry, Tryserver is not available. I guess I need it to set it up. Do you mind running it this time for me? Thanks! > > > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 12002/chrome/browser/search_**engines/template_url.cc<https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/search_engines/template_url.cc> > File chrome/browser/search_engines/**template_url.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 12002/chrome/browser/search_**engines/template_url.cc#**newcode216<https://chromiumcodereview.appspot.com/10537154/diff/12002/chrome/browser/search_engines/template_url.cc#newcode216> > chrome/browser/search_engines/**template_url.cc:216: if > (!search_terms_args.assisted_**query_stats.empty()) > On 2012/06/16 23:38:10, Bart N wrote: > >> On 2012/06/16 03:14:17, Peter Kasting wrote: >> > Nit: Needs {} >> I'm not sure who wrote this code, but the style is pretty >> > inconsistent. Check > >> if/else flows elsewhere. >> > > Yeah, the existing code was wrong. > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete.cc<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete.cc> > File chrome/browser/autocomplete/**autocomplete.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete.cc#**newcode896<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete.cc#newcode896> > chrome/browser/autocomplete/**autocomplete.cc:896: int query_index, > Taking an int here results in a typecast (bad anyway + may warn on other > OSes). Take a size_t, and make the StringPrintf() format string be: > > "%s.%" PRIuS ".%s" > > If this macro isn't defined you can get it from base/format_macros.h. > > Note: I still think it'd be just as clear -- and shorter overall -- to > inline this StringPrintf() call in the caller and aliminate this helper > function. > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc> > File chrome/browser/autocomplete/**autocomplete_unittest.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode41<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode41> > chrome/browser/autocomplete/**autocomplete_unittest.cc:41: const string16 > match_keyword) > > Nit: One arg per line > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode79<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode79> > chrome/browser/autocomplete/**autocomplete_unittest.cc:79: // Generate one > result synchronously, the rest later. > This comment is now wrong? > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode197<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode197> > chrome/browser/autocomplete/**autocomplete_unittest.cc:197: // Register a > TemplateURL for test keyword "t". > Can we do this in the UpdateAssistedQueryStats test instead? > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode354<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode354> > chrome/browser/autocomplete/**autocomplete_unittest.cc:354: > AutocompleteMatch match(NULL, -i, false, aqs_test_data[i].match_type); > Nit: "-i" for a size_t is pretty weird, and risks overrunning any signal > value that might be present at (-1). Can we just pick a constant for > the highest relevance and then do (kMaxRelevance - i) instead? > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode433<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode433> > chrome/browser/autocomplete/**autocomplete_unittest.cc:433: > EXPECT_TRUE(result_.match_at(**i)->search_terms_args->** > assisted_query_stats > Nit: Break after '(' instead (2 places) > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode508<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode508> > chrome/browser/autocomplete/**autocomplete_unittest.cc:508: > RunAssistedQueryStatsTest(**test_data, ARRAYSIZE_UNSAFE(test_data)); > Nit: I think you can use arraysize() here instead since the struct type > is named? (3 places) > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode509<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode509> > chrome/browser/autocomplete/**autocomplete_unittest.cc:509: > SCOPED_TRACE("No matches"); > Nit: Doesn't this need to be above the RunAssistedQueryStatsTest() call? > (3 places) > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/search_**engines/template_url.cc<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/search_engines/template_url.cc> > File chrome/browser/search_engines/**template_url.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/search_**engines/template_url.cc#**newcode110<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/search_engines/template_url.cc#newcode110> > chrome/browser/search_engines/**template_url.cc:110: // TemplateURLRef > ------------------------------**------------------------------**- > Nit: Move this below and put a "// TemplateURLRef::**SearchTermsArgs ---" > divider here. > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/search_**engines/template_url_unittest.**cc<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/search_engines/template_url_unittest.cc> > File chrome/browser/search_engines/**template_url_unittest.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 19001/chrome/browser/search_**engines/template_url_unittest.** > cc#newcode230<https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/search_engines/template_url_unittest.cc#newcode230> > chrome/browser/search_engines/**template_url_unittest.cc:230: > search_terms_data)); > Nit: If this doesn't fit on the previous line, it should be indented the > same as the line above, since it's not a second arg to the > SearchTermsArgs constructor. > > https://chromiumcodereview.**appspot.com/10537154/<https://chromiumcodereview... >
On 2012/06/18 19:56:17, Bart N wrote: > Meanwhile, I'm getting troubles running git try: > Sorry, Tryserver is not available. > > I guess I need it to set it up. Do you mind running it this time for me? > Thanks! Not a problem. Ping me once you upload a new patch and I'll do it. You may be having problems because tryservers are only available to committers. I don't know if you have a committer account yet (or were planning to get one?).
On Mon, Jun 18, 2012 at 1:00 PM, <pkasting@chromium.org> wrote: > On 2012/06/18 19:56:17, Bart N wrote: > >> Meanwhile, I'm getting troubles running git try: >> Sorry, Tryserver is not available. >> > > I guess I need it to set it up. Do you mind running it this time for me? >> Thanks! >> > > Not a problem. Ping me once you upload a new patch and I'll do it. > > You may be having problems because tryservers are only available to > committers. > I don't know if you have a committer account yet (or were planning to get > one?). > Yes. I'd like to get a chromium.org account + committer rights. Is there a process I should follow to get the account? > > https://chromiumcodereview.**appspot.com/10537154/<https://chromiumcodereview... >
Thanks. PTAL https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete.cc:896: int query_index, On 2012/06/18 19:45:52, Peter Kasting wrote: > Taking an int here results in a typecast (bad anyway + may warn on other OSes). > Take a size_t, and make the StringPrintf() format string be: > > "%s.%" PRIuS ".%s" > > If this macro isn't defined you can get it from base/format_macros.h. > > Note: I still think it'd be just as clear -- and shorter overall -- to inline > this StringPrintf() call in the caller and aliminate this helper function. Ah thanks, I was searching on the web and couldn't find a good way. Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:41: const string16 match_keyword) On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: One arg per line Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:79: // Generate one result synchronously, the rest later. On 2012/06/18 19:45:52, Peter Kasting wrote: > This comment is now wrong? Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:197: // Register a TemplateURL for test keyword "t". On 2012/06/18 19:45:52, Peter Kasting wrote: > Can we do this in the UpdateAssistedQueryStats test instead? I still wanted a full end-to-end test rather than UpdateAssistedQueryStatsTest only (which is testing a small fraction of the whole flow). https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:354: AutocompleteMatch match(NULL, -i, false, aqs_test_data[i].match_type); On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: "-i" for a size_t is pretty weird, and risks overrunning any signal value > that might be present at (-1). Can we just pick a constant for the highest > relevance and then do (kMaxRelevance - i) instead? Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:354: AutocompleteMatch match(NULL, -i, false, aqs_test_data[i].match_type); On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: "-i" for a size_t is pretty weird, and risks overrunning any signal value > that might be present at (-1). Can we just pick a constant for the highest > relevance and then do (kMaxRelevance - i) instead? Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:433: EXPECT_TRUE(result_.match_at(i)->search_terms_args->assisted_query_stats On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: Break after '(' instead (2 places) Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:509: SCOPED_TRACE("No matches"); On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: Doesn't this need to be above the RunAssistedQueryStatsTest() call? (3 > places) Ah yes. Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:110: // TemplateURLRef ------------------------------------------------------------- On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: Move this below and put a "// TemplateURLRef::SearchTermsArgs ---" divider > here. Done. https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... File chrome/browser/search_engines/template_url_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/19001/chrome/browser/sea... chrome/browser/search_engines/template_url_unittest.cc:230: search_terms_data)); On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: If this doesn't fit on the previous line, it should be indented the same as > the line above, since it's not a second arg to the SearchTermsArgs constructor. Done.
I'll get the tryserver running this. http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:197: // Register a TemplateURL for test keyword "t". On 2012/06/18 20:34:18, Bart N wrote: > On 2012/06/18 19:45:52, Peter Kasting wrote: > > Can we do this in the UpdateAssistedQueryStats test instead? > I still wanted a full end-to-end test rather than UpdateAssistedQueryStatsTest > only (which is testing a small fraction of the whole flow). Sure, but can't we do that by just moving this setup code down to the test that actually references this TemplateURL? It's not critical that we setup this TemplateURL in this function, is it? http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:508: RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data)); On 2012/06/18 19:45:52, Peter Kasting wrote: > Nit: I think you can use arraysize() here instead since the struct type is > named? (3 places) Was this false or did you miss it? http://codereview.chromium.org/10537154/diff/27001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/27001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:34: const size_t kMaxRelevance = 1000; Nit: Since we only use this one place, just define it in the function where it's used.
Working on the last nit: TemplateURL setup. http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:508: RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data)); On 2012/06/18 20:49:50, Peter Kasting wrote: > On 2012/06/18 19:45:52, Peter Kasting wrote: > > Nit: I think you can use arraysize() here instead since the struct type is > > named? (3 places) > > Was this false or did you miss it? I missed my reply. Here it goes: // One caveat is that, for C++03, arraysize() doesn't accept any array of // an anonymous type or a type defined inside a function. In these rare // cases, you have to use the unsafe ARRAYSIZE() macro below. This is // due to a limitation in C++03's template system. The limitation has // been removed in C++11. In short - no, I can't use it here as it doesn't compile. http://codereview.chromium.org/10537154/diff/27001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/27001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:34: const size_t kMaxRelevance = 1000; On 2012/06/18 20:49:50, Peter Kasting wrote: > Nit: Since we only use this one place, just define it in the function where it's > used. Done.
http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:508: RunAssistedQueryStatsTest(test_data, ARRAYSIZE_UNSAFE(test_data)); On 2012/06/18 21:20:52, Bart N wrote: > On 2012/06/18 20:49:50, Peter Kasting wrote: > > On 2012/06/18 19:45:52, Peter Kasting wrote: > > > Nit: I think you can use arraysize() here instead since the struct type is > > > named? (3 places) > > > > Was this false or did you miss it? > I missed my reply. Here it goes: > > // One caveat is that, for C++03, arraysize() doesn't accept any array of > // an anonymous type or a type defined inside a function. In these rare > // cases, you have to use the unsafe ARRAYSIZE() macro below. This is > // due to a limitation in C++03's template system. The limitation has > // been removed in C++11. > > In short - no, I can't use it here as it doesn't compile. Huh. I thought AssistedQueryStatsTestData was defined in the class instead of in the function, so it would compile here. But if you tested and it doesn't, I guess I'm wrong :)
http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:57: nit: why add this extra blank line here? http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:793: DCHECK(index < matches_.size()); nit: DCHECK_LT(...) http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:863: namespace { this usually goes into the top of the source file after the header includes.
Addressed all comments. PTAL http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/19001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:197: // Register a TemplateURL for test keyword "t". On 2012/06/18 20:49:50, Peter Kasting wrote: > On 2012/06/18 20:34:18, Bart N wrote: > > On 2012/06/18 19:45:52, Peter Kasting wrote: > > > Can we do this in the UpdateAssistedQueryStats test instead? > > I still wanted a full end-to-end test rather than UpdateAssistedQueryStatsTest > > only (which is testing a small fraction of the whole flow). > > Sure, but can't we do that by just moving this setup code down to the test that > actually references this TemplateURL? It's not critical that we setup this > TemplateURL in this function, is it? I tried, but there are some major implications, e.g. a match can't set keyword unless it's registered, otherwise GetTemplateURL crashes. I left a TODO and will address it in AQ cleanup, if you don't mind... http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:57: On 2012/06/18 22:01:57, tfarina wrote: > nit: why add this extra blank line here? Done. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:793: DCHECK(index < matches_.size()); On 2012/06/18 22:01:57, tfarina wrote: > nit: DCHECK_LT(...) Done. Here and below. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:863: namespace { On 2012/06/18 22:01:57, tfarina wrote: > this usually goes into the top of the source file after the header includes. I put it originally at the top but my reviewer asked my to move it here.
Looks from the trybots like there are compile errors in at least startup_browser_creator_win.cc, browser_frame_win.cc, and instant_browsertest.cc due to needing to use SearchTermsArgs in a few more places. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:57: On 2012/06/18 22:26:24, Bart N wrote: > On 2012/06/18 22:01:57, tfarina wrote: > > nit: why add this extra blank line here? > > Done. Actually I preferred it as it was, which is why I didn't call this out. I've been trying to put two blank lines before each class divider in general, though this file doesn't follow that at the moment. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:863: namespace { On 2012/06/18 22:26:24, Bart N wrote: > On 2012/06/18 22:01:57, tfarina wrote: > > this usually goes into the top of the source file after the header includes. > I put it originally at the top but my reviewer asked my to move it here. Indeed, please leave it. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.cc (right): http://codereview.chromium.org/10537154/diff/32002/chrome/browser/search_engi... chrome/browser/search_engines/template_url.cc:156: search_terms_args, UIThreadSearchTermsData(owner_->profile())); To fix a clang compile error, you'll need to move this last arg to be declared above the ReplaceSearchTermsUsingTermsData() call; see for example SupportsReplacement() above. http://codereview.chromium.org/10537154/diff/28002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/28002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:220: // TODO: Move it outside this method, after refactoring test flows. Nit: Can you say more about what needs to happen in order to move this? This is kind of vague.
I believe I've addressed all comments. On 2012/06/19 00:38:08, Peter Kasting wrote: > Looks from the trybots like there are compile errors in at least > startup_browser_creator_win.cc, browser_frame_win.cc, and instant_browsertest.cc > due to needing to use SearchTermsArgs in a few more places. > > http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... > File chrome/browser/autocomplete/autocomplete.cc (right): > > http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... > chrome/browser/autocomplete/autocomplete.cc:57: > On 2012/06/18 22:26:24, Bart N wrote: > > On 2012/06/18 22:01:57, tfarina wrote: > > > nit: why add this extra blank line here? > > > > Done. > > Actually I preferred it as it was, which is why I didn't call this out. > > I've been trying to put two blank lines before each class divider in general, > though this file doesn't follow that at the moment. > > http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... > chrome/browser/autocomplete/autocomplete.cc:863: namespace { > On 2012/06/18 22:26:24, Bart N wrote: > > On 2012/06/18 22:01:57, tfarina wrote: > > > this usually goes into the top of the source file after the header includes. > > I put it originally at the top but my reviewer asked my to move it here. > > Indeed, please leave it. > > http://codereview.chromium.org/10537154/diff/32002/chrome/browser/search_engi... > File chrome/browser/search_engines/template_url.cc (right): > > http://codereview.chromium.org/10537154/diff/32002/chrome/browser/search_engi... > chrome/browser/search_engines/template_url.cc:156: search_terms_args, > UIThreadSearchTermsData(owner_->profile())); > To fix a clang compile error, you'll need to move this last arg to be declared > above the ReplaceSearchTermsUsingTermsData() call; see for example > SupportsReplacement() above. > > http://codereview.chromium.org/10537154/diff/28002/chrome/browser/autocomplet... > File chrome/browser/autocomplete/autocomplete_unittest.cc (right): > > http://codereview.chromium.org/10537154/diff/28002/chrome/browser/autocomplet... > chrome/browser/autocomplete/autocomplete_unittest.cc:220: // TODO: Move it > outside this method, after refactoring test flows. > Nit: Can you say more about what needs to happen in order to move this? This is > kind of vague.
http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:57: On 2012/06/19 00:38:08, Peter Kasting wrote: > On 2012/06/18 22:26:24, Bart N wrote: > > On 2012/06/18 22:01:57, tfarina wrote: > > > nit: why add this extra blank line here? > > > > Done. > > Actually I preferred it as it was, which is why I didn't call this out. > > I've been trying to put two blank lines before each class divider in general, > though this file doesn't follow that at the moment. Done. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:863: namespace { ACK-ed. On 2012/06/19 00:38:08, Peter Kasting wrote: > On 2012/06/18 22:26:24, Bart N wrote: > > On 2012/06/18 22:01:57, tfarina wrote: > > > this usually goes into the top of the source file after the header includes. > > I put it originally at the top but my reviewer asked my to move it here. > > Indeed, please leave it. http://codereview.chromium.org/10537154/diff/32002/chrome/browser/search_engi... File chrome/browser/search_engines/template_url.cc (right): http://codereview.chromium.org/10537154/diff/32002/chrome/browser/search_engi... chrome/browser/search_engines/template_url.cc:156: search_terms_args, UIThreadSearchTermsData(owner_->profile())); On 2012/06/19 00:38:08, Peter Kasting wrote: > To fix a clang compile error, you'll need to move this last arg to be declared > above the ReplaceSearchTermsUsingTermsData() call; see for example > SupportsReplacement() above. Done. http://codereview.chromium.org/10537154/diff/28002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/10537154/diff/28002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:220: // TODO: Move it outside this method, after refactoring test flows. On 2012/06/19 00:38:08, Peter Kasting wrote: > Nit: Can you say more about what needs to happen in order to move this? This is > kind of vague. Done.
Fixing 1 remaining compilation issue:
Win:
E:\b\build\slave\win\build\src\chrome\browser\autocomplete\autocomplete_unittest.cc(522)
: error C2466: cannot allocate an array of constant size 0
E:\b\build\slave\win\build\src\chrome\browser\autocomplete\autocomplete_unittest.cc(522)
: error C2440: 'initializing' : cannot convert from 'int' to 'const
AutocompleteMatch::Type'
Conversion to enumeration type requires an explicit cast (static_cast,
C-style cast or function-style cast)
Mac:
The failing tests seem to be unrelated to this change.
Added a workaround for broken Win compiler. Not sure if it's the best solution, but at least you can give another tryserver kick. Thanks and sorry for still not being able to run tryservers by myself...
On 2012/06/19 02:24:43, Bart N wrote:
> Fixing 1 remaining compilation issue:
>
> Win:
>
>
E:\b\build\slave\win\build\src\chrome\browser\autocomplete\autocomplete_unittest.cc(522)
> : error C2466: cannot allocate an array of constant size 0
>
E:\b\build\slave\win\build\src\chrome\browser\autocomplete\autocomplete_unittest.cc(522)
> : error C2440: 'initializing' : cannot convert from 'int' to 'const
> AutocompleteMatch::Type'
> Conversion to enumeration type requires an explicit cast (static_cast,
> C-style cast or function-style cast)
>
> Mac:
> The failing tests seem to be unrelated to this change.
Actually, this failure looks suspicious:
../../chrome/browser/instant/instant_browsertest.cc:294: Failure
Value of: loader()->url().spec()
Actual: "http://127.0.0.1:61227/files/instant.html?q=def"
Expected: default_turl->url_ref().ReplaceSearchTerms(
TemplateURLRef::SearchTermsArgs(ASCIIToUTF16("defghi")))
Which is: "http://127.0.0.1:61227/files/instant.html?q=defghi"
[ FAILED ] InstantTest.OnChangeEvent, where TypeParam = and GetParam() =
(1112 ms)
but I don't see how it can be broken on Mac only. Looking into it...
On 2012/06/19 02:51:11, Bart N wrote:
> On 2012/06/19 02:24:43, Bart N wrote:
> > Fixing 1 remaining compilation issue:
> >
> > Win:
> >
> >
>
E:\b\build\slave\win\build\src\chrome\browser\autocomplete\autocomplete_unittest.cc(522)
> > : error C2466: cannot allocate an array of constant size 0
> >
>
E:\b\build\slave\win\build\src\chrome\browser\autocomplete\autocomplete_unittest.cc(522)
> > : error C2440: 'initializing' : cannot convert from 'int' to 'const
> > AutocompleteMatch::Type'
> > Conversion to enumeration type requires an explicit cast
(static_cast,
> > C-style cast or function-style cast)
> >
> > Mac:
> > The failing tests seem to be unrelated to this change.
> Actually, this failure looks suspicious:
>
> ../../chrome/browser/instant/instant_browsertest.cc:294: Failure
> Value of: loader()->url().spec()
> Actual: "http://127.0.0.1:61227/files/instant.html?q=def"
> Expected: default_turl->url_ref().ReplaceSearchTerms(
> TemplateURLRef::SearchTermsArgs(ASCIIToUTF16("defghi")))
> Which is: "http://127.0.0.1:61227/files/instant.html?q=defghi"
> [ FAILED ] InstantTest.OnChangeEvent, where TypeParam = and GetParam() =
> (1112 ms)
>
> but I don't see how it can be broken on Mac only. Looking into it...
This is most likely a real breakage. I guess that instant browser tests (part of
interactive UI tests) run on Mac only (Linux ran 108 tests vs 183 on Mac).
My guess is that destination_url in this change may be set twice which may have
some side effect in case of Instant. I still don't understand the issue, because
we make a copy of all params into SearchTermsArgs (I assume that string16 is not
like a StringPiece and its copy constructor performs a deep copy; if not then
this may be a problem if search_terms gets mutated after it was copied to
SearchTermsArgs).
I think the Instant failure is a real failure too. You might try building and debugging locally, perhaps using --single_process (or --single-process, I always forget which is which). If you can't figure it out, ping sky; he can perhaps help you track down what's going on. Also fill in at least the BUG= line for this change, as well as either some explicit manual testing steps or "none" for the TEST= line. Don't worry too much about not making the branch point. We can merge back if this needs to make M21. Let's focus on making the change correct first. https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:532: // Note: We use dummy data to work around a compilation error Nit: Slightly more informative: // MSVC doesn't support zero-length arrays, so supply some dummy data.
On Mon, Jun 18, 2012 at 10:53 PM, <pkasting@chromium.org> wrote: > I think the Instant failure is a real failure too. > > You might try building and debugging locally, perhaps using > --single_process (or > --single-process, I always forget which is which). If you can't figure it > out, > ping sky; he can perhaps help you track down what's going on. > Yes, thanks. I will try to do it tomorrow. > > Also fill in at least the BUG= line for this change, as well as either some > explicit manual testing steps or "none" for the TEST= line. > Don't worry too much about not making the branch point. We can merge back > if > this needs to make M21. Let's focus on making the change correct first. > > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 30007/chrome/browser/**autocomplete/autocomplete_**unittest.cc<https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/autocomplete/autocomplete_unittest.cc> > File chrome/browser/autocomplete/**autocomplete_unittest.cc (right): > > https://chromiumcodereview.**appspot.com/10537154/diff/** > 30007/chrome/browser/**autocomplete/autocomplete_**unittest.cc#newcode532<https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/autocomplete/autocomplete_unittest.cc#newcode532> > chrome/browser/autocomplete/**autocomplete_unittest.cc:532: // Note: We > use dummy data to work around a compilation error > Nit: Slightly more informative: > > // MSVC doesn't support zero-length arrays, so supply some dummy > data. > > https://chromiumcodereview.**appspot.com/10537154/<https://chromiumcodereview... >
It took me a while to figure the failure; it turned out that it was as simple as fixing operator= in AutocompleteMatch; I initially changed copy constructor, but I didn't realize there was also another place to update... I addressed all comments, merged with head, resolved all conflicts and bots are happy. The last remaining issue is to perform additional protocol check (must be https) before setting the AQS. https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/30007/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_unittest.cc:532: // Note: We use dummy data to work around a compilation error On 2012/06/19 05:53:43, Peter Kasting wrote: > Nit: Slightly more informative: > > // MSVC doesn't support zero-length arrays, so supply some dummy data. Done.
LGTM on what you have so far. Ping me when you add the protocol check and I'll take a look at that.
On 2012/06/22 19:46:13, Peter Kasting wrote: > LGTM on what you have so far. Ping me when you add the protocol check and I'll > take a look at that. Done, PTAL. Currently, running "git try" to verify if all looks good.
https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_match.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_match.cc:74: if (match.associated_keyword.get()) Nit: For consistency, we could probably change these conditionals to look like the unconditional reset() calls in operator=() below. (The reverse change wouldn't be safe as they need to be unconditional in operator=().) https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:219: const std::string kHttps("https"); Nit: Use chrome::kHttpsScheme from chrome/common/url_constants.h. https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:221: // Either search or autocomplete base URL must be using HTTPS This code isn't actually sufficient. First, someone could manually create a search engine with the AQS substitution arg and an HTTP scheme, and we'd happily send the data over HTTP because we're assuming any URL that uses this is using the Google base URLs. Second, along similar lines, did you want to restrict AQS from being sent to non-Google URLs? If so, you'd also want some sort of IsGoogleHostname() or similar check besides what you have here. Perhaps the best way to solve this is by doing something like: * Recursively call ReplaceSearchTermsUsingTermsData(), but pass in a SearchTermsArgs that has no AQS string to avoid infinite recursion. This will give you a trustworthy "final URL". * Check the scheme and other properties of this URL directly to see whether it's safe to substitute in the AQS string. * Add some unittests for TemplateURL that create some URLs that violate the constraint(s) above to the test suite. Finally, the comment here is both too short and confusing. Perhaps a longer explanation of what AQS is, what the constraints on it are, and why those constraints are necessary, should be added to some canonical place that talks about AQS, and then this comment can reference there.
https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:221: // Either search or autocomplete base URL must be using HTTPS > If so, you'd also want some sort of IsGoogleHostname()... See candidates at chrome/browser/google/google_util.h
PTAL. Please note my question about AQS docs. https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_match.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/38002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_match.cc:74: if (match.associated_keyword.get()) On 2012/06/22 21:48:31, Peter Kasting wrote: > Nit: For consistency, we could probably change these conditionals to look like > the unconditional reset() calls in operator=() below. (The reverse change > wouldn't be safe as they need to be unconditional in operator=().) Actually, to satisfy both performance & consistency, I've moved this code to the constructor's initialization list. https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:219: const std::string kHttps("https"); On 2012/06/22 21:48:31, Peter Kasting wrote: > Nit: Use chrome::kHttpsScheme from chrome/common/url_constants.h. Done. https://chromiumcodereview.appspot.com/10537154/diff/62001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:221: // Either search or autocomplete base URL must be using HTTPS On 2012/06/22 21:48:31, Peter Kasting wrote: > This code isn't actually sufficient. > > First, someone could manually create a search engine with the AQS substitution > arg and an HTTP scheme, and we'd happily send the data over HTTP because we're > assuming any URL that uses this is using the Google base URLs. Yes, that's a good point. I fixed it according to your suggestion. > > Second, along similar lines, did you want to restrict AQS from being sent to > non-Google URLs? If so, you'd also want some sort of IsGoogleHostname() or > similar check besides what you have here. No, we don't want to do that; that's the whole point of privacy discussion and enforcing HTTPS checks, etc. In case of evil.com, privacy folks mentioned mechanism of blacklisting providers. > > Perhaps the best way to solve this is by doing something like: > > * Recursively call ReplaceSearchTermsUsingTermsData(), but pass in a > SearchTermsArgs that has no AQS string to avoid infinite recursion. This will > give you a trustworthy "final URL". Done. > * Check the scheme and other properties of this URL directly to see whether > it's safe to substitute in the AQS string. > * Add some unittests for TemplateURL that create some URLs that violate the > constraint(s) above to the test suite. > > Finally, the comment here is both too short and confusing. Perhaps a longer > explanation of what AQS is, what the constraints on it are, and why those > constraints are necessary, should be added to some canonical place that talks > about AQS, and then this comment can reference there. Where would you suggest adding the more detailed description of what AQS is? I want to avoid unnecessary code shifting from one place to another in case you have another preference than I do.
I don't have a strong preference for where I'd put the AQS docs. Probably in the SearchTermsArgs struct? It's really up to you. LGTM with comments https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:228: if (StartsWithASCII(base_url, chrome::kHttpsScheme, false)) { Nit: Safer: GURL base_url(ReplaceSearchTermsUsingTermsData( search_terms_args_without_aqs, search_terms_data); if (url.SchemeIs(chrome::kHttpsScheme)) { ... https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... File chrome/browser/search_engines/template_url_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... chrome/browser/search_engines/template_url_unittest.cc:405: // No {google:baseURL} and protocol is HTTP, we must not substitute AQS. We should also test that a non-Google base URL _with_ HTTPS allows AQS.
Peter, thanks for the long review process and putting up with me :) I hope that next reviews will be less painful as I have now more experience with Chrome code base. Assuming that this is looking good now, what does it take to have it submitted? I'm not sure if I can do it, or whether you want someone else to do it instead? (Running git try in parallel). https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... chrome/browser/search_engines/template_url.cc:228: if (StartsWithASCII(base_url, chrome::kHttpsScheme, false)) { On 2012/06/22 23:18:11, Peter Kasting wrote: > Nit: Safer: > > GURL base_url(ReplaceSearchTermsUsingTermsData( > search_terms_args_without_aqs, search_terms_data); > if (url.SchemeIs(chrome::kHttpsScheme)) { ... Done. https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... File chrome/browser/search_engines/template_url_unittest.cc (right): https://chromiumcodereview.appspot.com/10537154/diff/67001/chrome/browser/sea... chrome/browser/search_engines/template_url_unittest.cc:405: // No {google:baseURL} and protocol is HTTP, we must not substitute AQS. On 2012/06/22 23:18:11, Peter Kasting wrote: > We should also test that a non-Google base URL _with_ HTTPS allows AQS. Done.
On 2012/06/23 01:06:30, Bart N wrote: > Assuming that this is looking good now, what does it take to have it submitted? > I'm not sure if I can do it, or whether you want someone else to do it instead? Answering on behalf of Peter, you should be able to check the "Commit:" box on this code review page and have the system use the commit queue to run some tests and submit the code for you. You can use this box even though you are not a committer because the change has been approved by a committer. --mark
On Sat, Jun 23, 2012 at 8:40 AM, <mpearson@chromium.org> wrote: > On 2012/06/23 01:06:30, Bart N wrote: > >> Assuming that this is looking good now, what does it take to have it >> > submitted? > >> I'm not sure if I can do it, or whether you want someone else to do it >> > instead? > > Answering on behalf of Peter, you should be able to check the "Commit:" > box on > this code review page and have the system use the commit queue to run some > tests > and submit the code for you. You can use this box even though you are not > a > committer because the change has been approved by a committer. > Ah, thanks! I'll wait for a while in case Peter has more comments and then will submit. Thanks all. > > --mark > > > https://chromiumcodereview.**appspot.com/10537154/<https://chromiumcodereview... >
Once you have an LGTM, you're generally free to commit as soon as you've addressed any remaining comments to your own satisfaction, unless something major comes up. I've checked the commit box.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@google.com/10537154/70001
Change committed as 143827 |
