Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
158053002:
Part 4 of search provider refactoring. (Closed)
|
Created:
6 years, 10 months ago by Maria Modified:
6 years, 10 months ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPart 4 of search provider refactoring.
Moves AddMatchToMap method to the common base class. Adds method parameters
so that both SearchProvider and ZeroSuggestProvider can use this method.
For SearchProvider this is a pure refactoring. I added some helper classes
in SearchProvider to avoid duplicating code.
For ZeroSuggestProvider, this introduces some additional functionality on
top of what it used to have. Mainly the method may record additional metadata on
the AutocompleteMatch. This metadata is not currently used by zero suggest
provider and as far as I can tell will not in any way affect the operation
of zero suggest provider today. In the future, some of the metadata
recorded will actually be useful (e.g. I am planning to take advantage of
deletion_url).
BUG=338955
TBR=mpearson
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251236
Patch Set 1 #Patch Set 2 : Fix up comments and formatting #Patch Set 3 : Rebase #
Total comments: 18
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Remove static comment #
Total comments: 28
Patch Set 6 : Moved GetInput/GetTemplateURL into superclass #Patch Set 7 : Fixed some nits #
Total comments: 19
Patch Set 8 : Fixed up comments #
Total comments: 4
Patch Set 9 : Remove copying of suggestion #Patch Set 10 : Don't copy suggestion #
Messages
Total messages: 29 (0 generated)
I think you may want to see if Mark wants to review this once since it's not as straightforward as the other CLs, since you're changing function parameters this time.
Mark, This particular change actually involves modifying zero suggest provider functionality (in a way that I consider non-problematic, but I may be missing something) -- it would be good to have another pair of eyes familiar with the code.
On 2014/02/08 02:13:11, Maria wrote: > Mark, > > This particular change actually involves modifying zero suggest provider > functionality (in a way that I consider non-problematic, but I may be missing > something) -- it would be good to have another pair of eyes familiar with the > code. Thanks for inviting the review. I'm sad to say there's a lot that's problematic about this refactoring. I didn't even finish the review because I had so many comments about comments and parameters that I thought things will change so much once those are fixed, I shouldn't even finish looking right now. That said, there's no much you touch in zero_suggest_provider from what I saw, and I don't think I see anything that changes it "functionality" as you claim (though as I said, I didn't look at everything). What were you referring to? --mark
oops, forgot to publish the comments: https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:266: const char BaseSearchProvider::kRelevanceFromServerKey[] = nit: comment // static https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:437: AutocompleteMatch match = CreateSearchSuggestion(this, input, query_string, nit: formatting. (I think it's either always-align or always-indent-four, not a mix of the two.) https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:437: AutocompleteMatch match = CreateSearchSuggestion(this, input, query_string, You've switched this from |input_| to |input_| or |keyword_input_|, depending on which |result| is from. In SearchProvider, we unconditionally used |input_| here. I audited the code and think this either-or will be okay. This raises a side comment: you might as well remove the function you added GetInput() because there's no need to select input types based on result. Or you can pass the correct input (default or keyword as appropriate) and skip passing input_text, because this is always the correct input.text() https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:260: // The following keys are used to record additional information on matches. nit: I think static variables are supposed to go after functions and before normal variables in the class definition. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:346: void AddMatchToMap(const SuggestResult& result, This comment is somewhat backwards, as is the parameter list. One, the main effect of this is adding a match to |map|. That should be in the first or second sentence of the big comment. Two, especially given the explanation, |template_url| is almost as important a parameter as |result|. It should go earlier in the list, certainly earlier than those parameters only used for extra/side-channel information. Three, the comment does not mention |query_string| Also, calling this variable |query_string| is extremely misleading because it's not the query string. The query string is stored in |result|. This is the input text, sent separately from |input|. See comment in .cc for more about this. Four, I don't like the comment about |meta_data|. First, isn't this comment likely to become obsolete (prefetch only)? Maybe you can make a broader statement that will likely stand the test of time. (Can you simply add it to the list of things merely passed onto CreateSearchSuggestion()?) As it is, the comment invites more questions. If you don't want to make a broader statement, then you need to explain what |metadata| is meant to represent and why is it only used for prefetch queries? Five, having a parameter here called |default_search_provider_result| be called |append_extra_query_params| in the implementation is very confusing. Please make them correspond. Also note that |default_search_provider_result| naturally reads as "is this result from the default search provider", which is an incorrect interpretation. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:347: const AutocompleteInput input, nit: why not &? https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:276: const AutocompleteInput GetInput(const SuggestResult& result) const; possibly unnecessary. See comment elsewhere. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:281: // Returns whether |result| is coming from the default search provider. This comment is slightly wrong. It doesn't match the implementation. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:272: AddMatchToMap(suggestion, AutocompleteInput(), query_string, template_url, Hopefully this ugly (pre-existing) AutocompleteInput() will go away once you clean up the input/input text parameter passing.
https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:437: AutocompleteMatch match = CreateSearchSuggestion(this, input, query_string, On 2014/02/10 23:25:55, Mark P wrote: > You've switched this from |input_| to |input_| or |keyword_input_|, depending on > which |result| is from. In SearchProvider, we unconditionally used |input_| > here. I audited the code and think this either-or will be okay. > > This raises a side comment: you might as well remove the function you added > GetInput() because there's no need to select input types based on result. > > Or you can pass the correct input (default or keyword as appropriate) and skip > passing input_text, because this is always the correct input.text() Thanks for catching this. The switch to matching input object was unintentional. In terms of search_provider, I agree that we can do either: pass the correct input OR always pass input_ along with the query (which is today's functionality). In terms of zero_suggest_provider however, we seem to do something a bit weird. We construct an empty AutocompleteInput object and then pass the suggestion text as the query. So even though the real query in zero_suggest_provider is always "", we send the suggestion text as an input (as far as I can tell). One slightly cleaner way may be to pass the exact parts of the input that we need into CreateSearchSuggestion (input_text, input_type, prevent_inline_autocomplete) and have each provider pass correct values for those from AddMatchToMap. Another way would be to only pass AutocompleteInput object, but mangle zero_suggest_provider's AutocompleteInput to actually set text to the value we are interested in retrieving -- this will be a bit weird, but will require passing fewer parameters. Opinions on which one makes more sense?
On 2014/02/11 01:22:09, Maria wrote: > https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... > File chrome/browser/autocomplete/base_search_provider.cc (right): > > https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... > chrome/browser/autocomplete/base_search_provider.cc:437: AutocompleteMatch match > = CreateSearchSuggestion(this, input, query_string, > On 2014/02/10 23:25:55, Mark P wrote: > > You've switched this from |input_| to |input_| or |keyword_input_|, depending > on > > which |result| is from. In SearchProvider, we unconditionally used |input_| > > here. I audited the code and think this either-or will be okay. > > > > This raises a side comment: you might as well remove the function you added > > GetInput() because there's no need to select input types based on result. > > > > Or you can pass the correct input (default or keyword as appropriate) and skip > > passing input_text, because this is always the correct input.text() > > Thanks for catching this. The switch to matching input object was unintentional. > > In terms of search_provider, I agree that we can do either: pass the correct > input OR always pass input_ along with the query (which is today's > functionality). > > In terms of zero_suggest_provider however, we seem to do something a bit weird. > We construct an empty AutocompleteInput object and then pass the suggestion text > as the query. So even though the real query in zero_suggest_provider is always > "", we send the suggestion text as an input (as far as I can tell). > > One slightly cleaner way may be to pass the exact parts of the input that we > need into CreateSearchSuggestion (input_text, input_type, > prevent_inline_autocomplete) and have each provider pass correct values for > those from AddMatchToMap. > > Another way would be to only pass AutocompleteInput object, but mangle > zero_suggest_provider's AutocompleteInput to actually set text to the value we > are interested in retrieving -- this will be a bit weird, but will require > passing fewer parameters. > > Opinions on which one makes more sense? I'd prefer passing AutocompleteInput to AddMatchToMap(). It also seems redundant to me that both input and input_text to CreateSearchSuggestion(). Regarding ZeroSuggestProvider, the reason (as commented) for passing in the suggestion is to avoid bolding, and I agree that it's a hack.
On Mon, Feb 10, 2014 at 5:22 PM, <mariakhomenko@chromium.org> wrote: > > https://codereview.chromium.org/158053002/diff/70001/ > chrome/browser/autocomplete/base_search_provider.cc > File chrome/browser/autocomplete/base_search_provider.cc (right): > > https://codereview.chromium.org/158053002/diff/70001/ > chrome/browser/autocomplete/base_search_provider.cc#newcode437 > chrome/browser/autocomplete/base_search_provider.cc:437: > AutocompleteMatch match = CreateSearchSuggestion(this, input, > query_string, > On 2014/02/10 23:25:55, Mark P wrote: > >> You've switched this from |input_| to |input_| or |keyword_input_|, >> > depending on > >> which |result| is from. In SearchProvider, we unconditionally used >> > |input_| > >> here. I audited the code and think this either-or will be okay. >> > > This raises a side comment: you might as well remove the function you >> > added > >> GetInput() because there's no need to select input types based on >> > result. > > Or you can pass the correct input (default or keyword as appropriate) >> > and skip > >> passing input_text, because this is always the correct input.text() >> > > Thanks for catching this. The switch to matching input object was > unintentional. > > In terms of search_provider, I agree that we can do either: pass the > correct input OR always pass input_ along with the query (which is > today's functionality). > > In terms of zero_suggest_provider however, we seem to do something a bit > weird. We construct an empty AutocompleteInput object and then pass the > suggestion text as the query. So even though the real query in > zero_suggest_provider is always "", we send the suggestion text as an > input (as far as I can tell). > > One slightly cleaner way may be to pass the exact parts of the input > that we need into CreateSearchSuggestion (input_text, input_type, > prevent_inline_autocomplete) and have each provider pass correct values > for those from AddMatchToMap. > > Another way would be to only pass AutocompleteInput object, but mangle > zero_suggest_provider's AutocompleteInput to actually set text to the > value we are interested in retrieving -- this will be a bit weird, but > will require passing fewer parameters. > > Opinions on which one makes more sense? > I agree with Harry: let's pass the AutocompleteInput object. General philosophy: anything we can do to shorten the CreateSearchSuggestion() parameter list is good. AutocompleteInput is a natural struct grouping some information together. Passing the parts of it that we need all as separate parameters seems icky. --mark > > https://codereview.chromium.org/158053002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Made changes as discussed. Also the difference in behaviour, I was referring to in the original comment had to do with the fact that we might now record things like: should_prefetch and deletion_url on the autocomplete match object in zero suggest provider, which didn't happen before. However, zero suggest provider currently doesn't generally pass any of this data and doesn't use it for anything, which is why I am considering it harmless. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:266: const char BaseSearchProvider::kRelevanceFromServerKey[] = On 2014/02/10 23:25:55, Mark P wrote: > nit: comment // static Done. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:437: AutocompleteMatch match = CreateSearchSuggestion(this, input, query_string, On 2014/02/10 23:25:55, Mark P wrote: > nit: formatting. (I think it's either always-align or always-indent-four, not a > mix of the two.) Done. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:260: // The following keys are used to record additional information on matches. On 2014/02/10 23:25:55, Mark P wrote: > nit: I think static variables are supposed to go after functions and before > normal variables in the class definition. Actually, according to Google style guide, the order is Typedefs and Enums Constants (static const data members) Constructors Destructor Methods, including static methods Data Members (except static const data members) So I am moving this up higher. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:346: void AddMatchToMap(const SuggestResult& result, On 2014/02/10 23:25:55, Mark P wrote: > This comment is somewhat backwards, as is the parameter list. > > One, the main effect of this is adding a match to |map|. That should be in the > first or second sentence of the big comment. > > Two, especially given the explanation, |template_url| is almost as important a > parameter as |result|. It should go earlier in the list, certainly earlier than > those parameters only used for extra/side-channel information. > > Three, the comment does not mention |query_string| > Also, calling this variable |query_string| is extremely misleading because it's > not the query string. The query string is stored in |result|. This is the > input text, sent separately from |input|. See comment in .cc for more about > this. > > Four, I don't like the comment about |meta_data|. First, isn't this comment > likely to become obsolete (prefetch only)? Maybe you can make a broader > statement that will likely stand the test of time. (Can you simply add it to > the list of things merely passed onto CreateSearchSuggestion()?) As it is, the > comment invites more questions. If you don't want to make a broader statement, > then you need to explain what |metadata| is meant to represent and why is it > only used for prefetch queries? > > Five, having a parameter here called |default_search_provider_result| be called > |append_extra_query_params| in the implementation is very confusing. Please > make them correspond. > Also note that |default_search_provider_result| naturally reads as "is this > result from the default search provider", which is an incorrect interpretation. Done. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:347: const AutocompleteInput input, On 2014/02/10 23:25:55, Mark P wrote: > nit: why not &? Done. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:276: const AutocompleteInput GetInput(const SuggestResult& result) const; On 2014/02/10 23:25:55, Mark P wrote: > possibly unnecessary. See comment elsewhere. Done. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:281: // Returns whether |result| is coming from the default search provider. On 2014/02/10 23:25:55, Mark P wrote: > This comment is slightly wrong. It doesn't match the implementation. Done. https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/70001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:272: AddMatchToMap(suggestion, AutocompleteInput(), query_string, template_url, On 2014/02/10 23:25:55, Mark P wrote: > Hopefully this ugly (pre-existing) AutocompleteInput() will go away once you > clean up the input/input text parameter passing. Sadly, no :(
I need to look at this code some more, but a couple comments for now. https://codereview.chromium.org/158053002/diff/230001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/230001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:62: // static I'm not sure if you need comments here since they are obviously static? https://codereview.chromium.org/158053002/diff/230001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:442: omnibox_start_margin, ShouldAppendExtraParams(result)); I'm not sure if you need to define the extra ShouldAppendExtraParams() functions since it would be true anyway with zero suggest since suggestions from zero suggest are never in keyword mode (and append_extra_params would be true).
https://codereview.chromium.org/158053002/diff/230001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/230001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:62: // static On 2014/02/12 03:17:49, H Fung wrote: > I'm not sure if you need comments here since they are obviously static? I added all those based on Mark's comment on the patch before, but he probably just meant to add one, removing the extras. https://codereview.chromium.org/158053002/diff/230001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:442: omnibox_start_margin, ShouldAppendExtraParams(result)); On 2014/02/12 03:17:49, H Fung wrote: > I'm not sure if you need to define the extra ShouldAppendExtraParams() functions > since it would be true anyway with zero suggest since suggestions from zero > suggest are never in keyword mode (and append_extra_params would be true). The reason I need to is the definition in search_provider.cc uses providers_ field to compute the answer that I don't want to pull up into base class since zero suggest provider has only one fetcher.
I think this new version is much better. In fact, it looks good to me. You still may want to ask msw@ to see if he wants to review it. --mark https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:459: // Try to add |match| to |map|. If a match for |query_string| is already in query_string -> result.suggestion() https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:42: // Returns whether |match| is flagged as a query that should be prefetched. Are only queries allowed to be prefetched, or do prefetch hints apply to navsuggestions too? If you don't know, I suggest writing vaguer language. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:339: // |input| text. Adds the created match to |map|; if such a match already This doesn't sound right. Shouldn't it create a match to search for the query in |result|? https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:350: // Returns whether the destination URL for an AutocompleteMatch corresponding I find the phrase "for an AutocompleteMatch" here confusing. Is it necessary? https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:482: providers_.default_provider().empty(); does this not fit on one line? https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:279: // Returns the AutocompleteInput object corresponding to the given |result|. Can we try a comment that makes it clearer most properties of result don't matter? Perhaps something like: // Return the relevant AutocompleteInput object based on whether result is from the keyword provider. ditto analogous comment for GetTemplateURL https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:178: bool ZeroSuggestProvider::ShouldAppendExtraParams( It would be nice to comment why this is true. (Yes, I know the original code did not.) https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:273: const SuggestResult suggestion( Can you make a TODO in a later changelist to clean this up? It's icky to create a list of suggest results, then pull out some elements of each one, and use those elements to create new suggest results. We should simply create the right list in the first place.
I don't think I have much more comments. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:283: const TemplateURL* GetTemplateURL(const SuggestResult& result) const; Maybe consider moving this into BaseSearchProvider and override it in SearchProvider and ZeroSuggestProvider? That way, it doesn't need to be passed to AddMatchToMap(). https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:273: const SuggestResult suggestion( On 2014/02/12 23:35:49, Mark P wrote: > Can you make a TODO in a later changelist to clean this up? It's icky to create > a list of suggest results, then pull out some elements of each one, and use > those elements to create new suggest results. We should simply create the right > list in the first place. Thanks for noticing, Mark. What's the reason for needing to modify the original SuggestResult? Does it have to do with the query string?
https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:273: const SuggestResult suggestion( On 2014/02/13 01:43:33, H Fung wrote: > On 2014/02/12 23:35:49, Mark P wrote: > > Can you make a TODO in a later changelist to clean this up? It's icky to > create > > a list of suggest results, then pull out some elements of each one, and use > > those elements to create new suggest results. We should simply create the > right > > list in the first place. > > Thanks for noticing, Mark. What's the reason for needing to modify the original > SuggestResult? I don't recall. It may simply have to do with the way we copied code into ZeroSuggest in the first place (i.e., there might not be a reason at all). > Does it have to do with the query string?
https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:302: bool append_extra_query_params); With the new ShouldAppendExtraParams helper, this argument is no longer necessary (call ShouldAppendExtraParams inside the impl) https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1405: const AutocompleteInput SearchProvider::GetInput( Could BaseSearchProvider also declare this (and ZeroSuggestProvider also implement it, or have BaseSearchProvider supply a default impl with a default AutocompleteInput), so that the corresponding AutocompleteInput arguments could be removed from CreateSearchSuggestion and AddMatchToMap, like I hope you'll do with GetTemplateURL? https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:283: const TemplateURL* GetTemplateURL(const SuggestResult& result) const; On 2014/02/13 01:43:33, H Fung wrote: > Maybe consider moving this into BaseSearchProvider and override it in > SearchProvider and ZeroSuggestProvider? That way, it doesn't need to be passed > to AddMatchToMap(). +1 https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:277: // Set input's text to be query_string to avoid bolding nit: |input|'s and |query_string|; add a period at the end.
Addressed comments. PTAL. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:459: // Try to add |match| to |map|. If a match for |query_string| is already in On 2014/02/12 23:35:49, Mark P wrote: > query_string -> result.suggestion() Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:42: // Returns whether |match| is flagged as a query that should be prefetched. On 2014/02/12 23:35:49, Mark P wrote: > Are only queries allowed to be prefetched, or do prefetch hints apply to > navsuggestions too? > > If you don't know, I suggest writing vaguer language. This is only used to prefetch Google search page results -- so this would always be queries. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:302: bool append_extra_query_params); On 2014/02/13 02:43:50, msw wrote: > With the new ShouldAppendExtraParams helper, this argument is no longer > necessary (call ShouldAppendExtraParams inside the impl) I don't think it makes sense to do that since CreateSearchSuggestion is a static function. However, this static method could be changed to a private class method now that we call it only inside here. WDYT? https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:339: // |input| text. Adds the created match to |map|; if such a match already On 2014/02/12 23:35:49, Mark P wrote: > This doesn't sound right. Shouldn't it create a match to search for the query > in |result|? Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:350: // Returns whether the destination URL for an AutocompleteMatch corresponding On 2014/02/12 23:35:49, Mark P wrote: > I find the phrase "for an AutocompleteMatch" here confusing. Is it necessary? Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:482: providers_.default_provider().empty(); On 2014/02/12 23:35:49, Mark P wrote: > does this not fit on one line? 83 chars :( https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1405: const AutocompleteInput SearchProvider::GetInput( On 2014/02/13 02:43:50, msw wrote: > Could BaseSearchProvider also declare this (and ZeroSuggestProvider also > implement it, or have BaseSearchProvider supply a default impl with a default > AutocompleteInput), so that the corresponding AutocompleteInput arguments could > be removed from CreateSearchSuggestion and AddMatchToMap, like I hope you'll do > with GetTemplateURL? Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:279: // Returns the AutocompleteInput object corresponding to the given |result|. On 2014/02/12 23:35:49, Mark P wrote: > Can we try a comment that makes it clearer most properties of result don't > matter? Perhaps something like: > // Return the relevant AutocompleteInput object based on whether result is from > the keyword provider. > > ditto analogous comment for GetTemplateURL I decided to change this to take is_keyword instead because I realized that will be more useful in one of my next refactorings. Comment adjusted. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:283: const TemplateURL* GetTemplateURL(const SuggestResult& result) const; On 2014/02/13 01:43:33, H Fung wrote: > Maybe consider moving this into BaseSearchProvider and override it in > SearchProvider and ZeroSuggestProvider? That way, it doesn't need to be passed > to AddMatchToMap(). Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:178: bool ZeroSuggestProvider::ShouldAppendExtraParams( On 2014/02/12 23:35:49, Mark P wrote: > It would be nice to comment why this is true. (Yes, I know the original code > did not.) Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:273: const SuggestResult suggestion( On 2014/02/12 23:35:49, Mark P wrote: > Can you make a TODO in a later changelist to clean this up? It's icky to create > a list of suggest results, then pull out some elements of each one, and use > those elements to create new suggest results. We should simply create the right > list in the first place. Done. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:277: // Set input's text to be query_string to avoid bolding On 2014/02/13 02:43:50, msw wrote: > nit: |input|'s and |query_string|; add a period at the end. Done.
LGTM with nits. https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:302: bool append_extra_query_params); On 2014/02/13 21:07:36, Maria wrote: > On 2014/02/13 02:43:50, msw wrote: > > With the new ShouldAppendExtraParams helper, this argument is no longer > > necessary (call ShouldAppendExtraParams inside the impl) > > I don't think it makes sense to do that since CreateSearchSuggestion is a static > function. > > However, this static method could be changed to a private class method now that > we call it only inside here. WDYT? Hmm, no. Leave it as-is for now, I think. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:429: // Android and iOS have no InstantService nit: add a period at the end. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:348: // Returns the TemplateURL based on whether the |result| is from the default nit: consider "Returns the TemplateURL for the given |result|." (discussing the implementation here doesn't seem helpful) https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:353: // Returns the AutocompleteInput based on whether the |result| is from the ditto nit: consider "Returns the AutocompleteInput for the given |result|." https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1406: for (size_t i = 0; i < results.size(); ++i) { nit: remove curly braces. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:188: const base::string16& query_string(result.suggestion()); nit: inline this below. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:288: // TODO(mariakhomenko): Do not reconstruct SuggestResult object with nit: "objects" or "each object" or similar. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.h:97: // Creates AutocompleteMatches to search for "<suggestion>" for nit: consider "Adds AutocompleteMatches for each of the suggestions in |results| to |map|." or similar.
lgtm Thanks for cleaning up the code. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:338: // Creates an AutocompleteMatch from |result| to search |template_url| for template_url and input are not parameters anymore.
https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:348: // Returns the TemplateURL based on whether the |result| is from the default On 2014/02/13 22:47:18, msw wrote: > nit: consider "Returns the TemplateURL for the given |result|." > (discussing the implementation here doesn't seem helpful) I am okay with this suggestion. (I mention this because I think Maria put in this comment because of me.) https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:353: // Returns the AutocompleteInput based on whether the |result| is from the On 2014/02/13 22:47:18, msw wrote: > ditto nit: consider "Returns the AutocompleteInput for the given |result|." I disagree. There's no natural notion for the AutocompleteInput for a given result. I prefer this comment as-is.
https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:353: // Returns the AutocompleteInput based on whether the |result| is from the On 2014/02/13 23:01:53, Mark P wrote: > On 2014/02/13 22:47:18, msw wrote: > > ditto nit: consider "Returns the AutocompleteInput for the given |result|." > > I disagree. There's no natural notion for the AutocompleteInput for a given > result. I prefer this comment as-is. That's a good point; this doesn't return the input from the time the Result was constructed, but instead returns the current input corresponding to the result's keyword/default source. Ignore my comment.
https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:429: // Android and iOS have no InstantService On 2014/02/13 22:47:18, msw wrote: > nit: add a period at the end. Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:338: // Creates an AutocompleteMatch from |result| to search |template_url| for On 2014/02/13 22:52:39, H Fung wrote: > template_url and input are not parameters anymore. Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:348: // Returns the TemplateURL based on whether the |result| is from the default On 2014/02/13 23:01:53, Mark P wrote: > On 2014/02/13 22:47:18, msw wrote: > > nit: consider "Returns the TemplateURL for the given |result|." > > (discussing the implementation here doesn't seem helpful) > > I am okay with this suggestion. (I mention this because I think Maria put in > this comment because of me.) Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.h:353: // Returns the AutocompleteInput based on whether the |result| is from the On 2014/02/13 22:47:18, msw wrote: > ditto nit: consider "Returns the AutocompleteInput for the given |result|." Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1406: for (size_t i = 0; i < results.size(); ++i) { On 2014/02/13 22:47:18, msw wrote: > nit: remove curly braces. Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:188: const base::string16& query_string(result.suggestion()); On 2014/02/13 22:47:18, msw wrote: > nit: inline this below. Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:288: // TODO(mariakhomenko): Do not reconstruct SuggestResult object with On 2014/02/13 22:47:18, msw wrote: > nit: "objects" or "each object" or similar. Done. https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://codereview.chromium.org/158053002/diff/430001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.h:97: // Creates AutocompleteMatches to search for "<suggestion>" for On 2014/02/13 22:47:18, msw wrote: > nit: consider "Adds AutocompleteMatches for each of the suggestions in |results| > to |map|." or similar. Done.
The CQ bit was checked by mariakhomenko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/158053002/5...
Still LGTM, but I have two more minor nits. https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:189: input.UpdateText(base::string16(result.suggestion()), nit: I don't think you need to construct a string16 copy here. https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.h:98: // |map|. nit: this fits on line above.
The CQ bit was unchecked by mariakhomenko@chromium.org
https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.cc:189: input.UpdateText(base::string16(result.suggestion()), On 2014/02/13 23:27:54, msw wrote: > nit: I don't think you need to construct a string16 copy here. Oops. You are right! Done. https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://codereview.chromium.org/158053002/diff/560001/chrome/browser/autocomp... chrome/browser/autocomplete/zero_suggest_provider.h:98: // |map|. On 2014/02/13 23:27:54, msw wrote: > nit: this fits on line above. Nope, 81 chars.
The CQ bit was checked by mariakhomenko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/158053002/8...
Message was sent while issue was closed.
Change committed as 251236 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
