Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
470313004:
[AiS] Use top local result for prefetch. (Closed)
|
Created:
6 years, 4 months ago by groby-ooo-7-16 Modified:
6 years, 2 months ago Reviewers:
Mark P CC:
chromium-reviews, James Su, Peter Kasting, Justin Donnelly Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[AiS] Use top local result for prefetch.
For answers prefetching, Chrome needs to know what the top search history
result for the current input is - _before_ issuing a search engine query
for that same input
BUG=406588
Committed: https://crrev.com/e5fcee487262976448cde70e40ac6bbe11660f6b
Cr-Commit-Position: refs/heads/master@{#296873}
Patch Set 1 #Patch Set 2 : Score HistoryResults earlier if part of Answers FieldTrial #
Total comments: 2
Patch Set 3 : Fix empty results. #Patch Set 4 : Use top suggestion to retrieve answer. #
Total comments: 60
Patch Set 5 : Rebase to HEAD #Patch Set 6 : Rename result/scored_result -> raw_result/transformed_result. #Patch Set 7 : Review fixes. #
Total comments: 22
Patch Set 8 : REBASE to HEAD #Patch Set 9 : Moar review fixes. #
Total comments: 4
Patch Set 10 : Review fixes. #
Total comments: 4
Patch Set 11 : Fix nits. #Patch Set 12 : Ooops. Fix transposed words. #Patch Set 13 : Fixed unit test to use proper function invocation. #
Total comments: 6
Patch Set 14 : Moar review fixes. #
Total comments: 2
Patch Set 15 : Experiment: Remove reference #Patch Set 16 : Experiment: remove reference. #Patch Set 17 : Fix review nit. #
Messages
Total messages: 39 (7 generated)
Heads-up - some refactoring going on here for SearchProvider and history results. Not ready for review, just in case you want to keep an eye.
Stage 2 - compute scores earlier, but only if part of Answers FieldTrial https://codereview.chromium.org/470313004/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:250: if (OmniboxFieldTrial::EnableAnswersInSuggest()) { Do we want a UMA histogram for the time spent here? https://codereview.chromium.org/470313004/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:892: if (!OmniboxFieldTrial::EnableAnswersInSuggest()) { Since scoring can now take two different paths - should we exercise both paths in the unit tests? For my own reference, here's the list of tests that touch this code path: SearchProviderTest.QueryDefaultProvider SearchProviderTest.HonorPreventInlineAutocomplete SearchProviderTest.QueryKeywordProvider SearchProviderTest.DontAutocompleteURLLikeTerms SearchProviderTest.DontAutocompleteUntilMultipleWordsTyped SearchProviderTest.AutocompleteMultipleVisitsImmediately SearchProviderTest.AutocompleteAfterSpace SearchProviderTest.ScoreNewerSearchesHigher SearchProviderTest.DontReplacePreviousAutocompletion SearchProviderTest.DontCrowdOutSingleWords SearchProviderTest.InlineMixedCaseMatches SearchProviderTest.KeywordOrderingAndDescriptions SearchProviderTest.LocalAndRemoteRelevances SearchProviderTest.ReflectsBookmarkBarState SearchProviderTest.TestDeleteHistoryQueryMatch SearchProviderTest.CheckDuplicateMatchesSaved
FYI, I glanced at it. This looks like a fine start. --mark On Fri, Aug 22, 2014 at 2:35 PM, <groby@chromium.org> wrote: > Reviewers: , > > Message: > Heads-up - some refactoring going on here for SearchProvider and history > results. Not ready for review, just in case you want to keep an eye. > > Description: > [AiS] Use top local result for prefetch. > > For answers prefetching, Chrome needs to know what the top search history > result for the current input is - _before_ issuing a search engine query > for that same input > > BUG=406588 > > Please review this at https://codereview.chromium.org/470313004/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+39, -27 lines): > M chrome/browser/autocomplete/search_provider.h > M chrome/browser/autocomplete/search_provider.cc > > > Index: chrome/browser/autocomplete/search_provider.cc > diff --git a/chrome/browser/autocomplete/search_provider.cc > b/chrome/browser/autocomplete/search_provider.cc > index c059c6ec2030c57e826980c33907b53beb5e7646.. > 957233d3c5e9c9ae3113f34f949292dd688c2d0d 100644 > --- a/chrome/browser/autocomplete/search_provider.cc > +++ b/chrome/browser/autocomplete/search_provider.cc > @@ -873,33 +873,9 @@ void SearchProvider::AddHistoryResultsToMap(const > HistoryResults& results, > return; > > base::TimeTicks start_time(base::TimeTicks::Now()); > - bool prevent_inline_autocomplete = input_.prevent_inline_autocomplete() > || > - (input_.type() == metrics::OmniboxInputType::URL); > - const base::string16& input_text = > - is_keyword ? keyword_input_.text() : input_.text(); > - bool input_multiple_words = HasMultipleWords(input_text); > - > SearchSuggestionParser::SuggestResults scored_results; > - if (!prevent_inline_autocomplete && input_multiple_words) { > - // ScoreHistoryResults() allows autocompletion of multi-word, 1-visit > - // queries if the input also has multiple words. But if we were > already > - // scoring a multi-word, multi-visit query aggressively, and the > current > - // input is still a prefix of it, then changing the suggestion > suddenly > - // feels wrong. To detect this case, first score as if only one word > has > - // been typed, then check if the best result came from aggressive > search > - // history scoring. If it did, then just keep that score set. This > - // 1200 the lowest possible score in CalculateRelevanceForHistory()'s > - // aggressive-scoring curve. > - scored_results = ScoreHistoryResults(results, > prevent_inline_autocomplete, > - false, input_text, is_keyword); > - if ((scored_results.front().relevance() < 1200) || > - !HasMultipleWords(scored_results.front().suggestion())) > - scored_results.clear(); // Didn't detect the case above, score > normally. > - } > - if (scored_results.empty()) > - scored_results = ScoreHistoryResults(results, > prevent_inline_autocomplete, > - input_multiple_words, input_text, > - is_keyword); > + ScoreHistoryResultsMultiWord(results, is_keyword, &scored_results); > + > for (SearchSuggestionParser::SuggestResults::const_iterator i( > scored_results.begin()); i != scored_results.end(); ++i) { > AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true, > @@ -1021,6 +997,39 @@ SearchSuggestionParser::SuggestResults > SearchProvider::ScoreHistoryResults( > return scored_results; > } > > +void SearchProvider::ScoreHistoryResultsMultiWord( > + const HistoryResults& results, > + bool is_keyword, > + SearchSuggestionParser::SuggestResults* scored_results) { > + bool prevent_inline_autocomplete = > + input_.prevent_inline_autocomplete() || > + (input_.type() == metrics::OmniboxInputType::URL); > + const base::string16& input_text = > + is_keyword ? keyword_input_.text() : input_.text(); > + bool input_multiple_words = HasMultipleWords(input_text); > + > + if (!prevent_inline_autocomplete && input_multiple_words) { > + // ScoreHistoryResults() allows autocompletion of multi-word, 1-visit > + // queries if the input also has multiple words. But if we were > already > + // scoring a multi-word, multi-visit query aggressively, and the > current > + // input is still a prefix of it, then changing the suggestion > suddenly > + // feels wrong. To detect this case, first score as if only one word > has > + // been typed, then check if the best result came from aggressive > search > + // history scoring. If it did, then just keep that score set. This > + // 1200 the lowest possible score in CalculateRelevanceForHistory()'s > + // aggressive-scoring curve. > + *scored_results = ScoreHistoryResults( > + results, prevent_inline_autocomplete, false, input_text, > is_keyword); > + if ((scored_results->front().relevance() < 1200) || > + !HasMultipleWords(scored_results->front().suggestion())) > + scored_results->clear(); // Didn't detect the case above, score > normally. > + } > + if (scored_results->empty()) > + *scored_results = ScoreHistoryResults(results, > prevent_inline_autocomplete, > + input_multiple_words, input_text, > + is_keyword); > +} > + > void SearchProvider::AddSuggestResultsToMap( > const SearchSuggestionParser::SuggestResults& results, > const std::string& metadata, > Index: chrome/browser/autocomplete/search_provider.h > diff --git a/chrome/browser/autocomplete/search_provider.h > b/chrome/browser/autocomplete/search_provider.h > index 14a4836e2c6fc63aa14fa1bd3fa3d41de79c8f23.. > 268f0318be8ad6be749eb91367ffbee443935f9c 100644 > --- a/chrome/browser/autocomplete/search_provider.h > +++ b/chrome/browser/autocomplete/search_provider.h > @@ -260,7 +260,10 @@ class SearchProvider : public BaseSearchProvider, > bool input_multiple_words, > const base::string16& input_text, > bool is_keyword); > - > + void ScoreHistoryResultsMultiWord( > + const HistoryResults& results, > + bool is_keyword, > + SearchSuggestionParser::SuggestResults* scored_results); > // Adds matches for |results| to |map|. > void AddSuggestResultsToMap( > const SearchSuggestionParser::SuggestResults& results, > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
groby@chromium.org changed reviewers: + mpearson@chromium.org
OK, this looks like a working version. Mark, please take a look. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:251: if (OmniboxFieldTrial::EnableAnswersInSuggest()) { We could _always_ do the scoring after DoHistoryQuery - but that might impact UMA timing. Related, let me repost a previous comment: Do we want a histogram just for the scoring times to see how much impact on AddHistoryResultsTime this has? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:900: if (!OmniboxFieldTrial::EnableAnswersInSuggest()) { Another repost: Do we want to run unit tests both with and without Answers enabled? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:914: // TODO(groby): Can this be merged with AddSuggestResultsToMap? I don't think it can because AddSuggestResults because it sets |accepted_suggestion| to the index of the result, while AddHistoryResults sets the index to true. (Which, BTW, is odd. Is that intended?) https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1321: // results, duplicates be damned. Re-use of the partial map extends the state the SearchProvider carries around, but it seems possible - except that it slightly messes with "prefer items added first". This only affects verbatim matches, AFAICT, which should already be scored sufficiently high. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: // This seems to only matter for AQS.... See question above. If I share state, I'll obviously need to pass in did_not_accept_suggestion. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: // This seems to only matter for AQS.... See question above. This might also be relevant when I share state as discussed above. The question is, is did_not_accept_suggestion available before all results are int? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3265: QueryForInput(ASCIIToUTF16("weather l"), false, false); QueryForInput is only called to set up providers_ - does that warrant a comment?
https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:249: // Answers needs the scored history results before any suggest query has been nit: omit "the" https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:250: // started. Please explain why, either here or elsewhere if there's a better place. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:251: if (OmniboxFieldTrial::EnableAnswersInSuggest()) { On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > We could _always_ do the scoring after DoHistoryQuery - but that might impact > UMA timing. I like your approach in this changelist. > Related, let me repost a previous comment: Do we want a histogram just for the > scoring times to see how much impact on AddHistoryResultsTime this has? I'm not concerned. If you want to add it, it's fine with me. I'm not asking you to add it. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:258: } Consider: else { clear all relevant data } https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:896: // here for non-Answers Chrome, to prevent any impact on timing measurements. The reason for this is not "to prevent any impact on timing measurements", rather it's to prevent performance regressions from moving the scoring code before the suggest request is sent. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:897: // Answers FieldTrials will instead use scoring results obtained previously. nit: "Answers FieldTrials" is awkward. Consider replacing the sentence. Something like "For users with Answers enabled, the history results will be scored earlier." https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:900: if (!OmniboxFieldTrial::EnableAnswersInSuggest()) { On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > Another repost: Do we want to run unit tests both with and without Answers > enabled? It's up to you. My typical strategy when I add a field trial: * add test(s) if necessary that specifically exercise the new code. * run all tests in a local build that has the field trial on for everyone I don't bother duplicating tests for field-trial-on and field-trial-off unless they should/do behave differently under those cases. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:914: // TODO(groby): Can this be merged with AddSuggestResultsToMap? On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > I don't think it can because AddSuggestResults because it sets > |accepted_suggestion| to the index of the result, while AddHistoryResults sets > the index to true. (Which, BTW, is odd. Is that intended?) You're reading this wrong. It sets |mark_as_deletable| to true, which is accurate. The |accepted_suggestion| variable is set to |did_not_accept_suggestion|, which seems appropriate for a suggestion that didn't come from the server. (This |accepted_suggestion| variable is used to send feedback to the server.) You can try combining these two functions into one. I'm not sure it's worth the time. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:919: if (results.empty()) I'm not sure the results.empty() short-circuit is worth anything. Please remove it. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1045: if (results.empty()) Shouldn't we clear scored_results in this case? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1049: input_.prevent_inline_autocomplete() || nit: can't this go on the previous line? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1052: is_keyword ? keyword_input_.text() : input_.text(); Consider GetInput(is_keyword).text() https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1071: if (scored_results->empty()) nit: { } https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1321: // results, duplicates be damned. On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > Re-use of the partial map extends the state the SearchProvider carries around, > but it seems possible - except that it slightly messes with "prefer items added > first". This only affects verbatim matches, AFAICT, which should already be > scored sufficiently high. I'm not sure I follow your proposal here. Note that verbatim matches may vary in score nowadays. They can score as low as 851. Lots of things will beat them. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: // This seems to only matter for AQS.... On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > See question above. > > If I share state, I'll obviously need to pass in did_not_accept_suggestion. ditto: I'm not sure I follow your proposal here. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: // This seems to only matter for AQS.... On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > See question above. > > This might also be relevant when I share state as discussed above. The question > is, is did_not_accept_suggestion available before all results are in? No. In fact, the value of did_not_accept_suggestion depends on whether results are in. int did_not_accept_default_suggestion = default_results_.suggest_results.empty() ? TemplateURLRef::NO_SUGGESTIONS_AVAILABLE : TemplateURLRef::NO_SUGGESTION_CHOSEN; https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1325: AddScoredHistoryResultsToMap(scored_keyword_history_results_, false, &map); Please do not pass false for an int without an explicit cast. Also, more importantly, false is likely the wrong value, regardless. Please use the right value. Note that given my other comment, you might not be able to have the right value at this point. I should figure out if it's important to have the right value in this variable. If so, you need to correct its value later. If not, you should comment here about how it doesn't matter. (And perhaps comment in the .h by this function and possibly in the .h by the internal variable that gets assigned this as a result that this particular field is set incorrectly.) https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:251: // Adds a match for each scored result in |results| to |map|. nit: missing blank line https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:251: // Adds a match for each scored result in |results| to |map|. I don't really like the notion of calling these "scored" results. While it's true that they have relevance scores, I think the bigger difference is that these results are already converted into SuggestResults. Can you think of better terminology? Perhaps the first function could be AddRawHistoryResultsToMap() and the second could be AddTransformedHistoryResultsToMap(). Or perhaps you have a better idea? If you take this suggestion, please search through this changelist for "scored" and revise the variable names and comments appropriately. And maybe change the original un-scored variable names to "raw" or whatever you decide. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:264: void ScoreHistoryResultsMultiWord( I originally read this and added the comment "optional nit: MultiWord -> MultiWordInput" but reading the code I find that I'm interpreting this wrong. What is it supposed to mean? Please give it a better name. Here's one idea: isn't this now the primary ScoreHistoryResults function and the original is merely a helper function, e.g., ScoreHistoryResultsHelper? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:268: // Adds matches for |results| to |map|. nit: missing blank line https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:321: // current |input| and sets |prefetch_data_|. This comment doesn't seem appropriate anymore. Please revise. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:347: // Scored searches in the user's history - based on xxx_history_results_. nit: xxx_history_results_. -> keyword_history_results_ or default_history_results_ as appropriate. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3260: query, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, WHAT_YOU_TYPED seems inappropriate https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3263: true, false, query); This true (relevance from server) should be false if you want to create something that looks like a history result. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3265: QueryForInput(ASCIIToUTF16("weather l"), false, false); On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > QueryForInput is only called to set up providers_ - does that warrant a comment? Nah.
PTAL https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:249: // Answers needs the scored history results before any suggest query has been On 2014/09/05 22:18:16, Mark P wrote: > nit: omit "the" Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:250: // started. On 2014/09/05 22:18:15, Mark P wrote: > Please explain why, either here or elsewhere if there's a better place. Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:251: if (OmniboxFieldTrial::EnableAnswersInSuggest()) { On 2014/09/05 22:18:16, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > We could _always_ do the scoring after DoHistoryQuery - but that might impact > > UMA timing. > > I like your approach in this changelist. > > > Related, let me repost a previous comment: Do we want a histogram just for the > > scoring times to see how much impact on AddHistoryResultsTime this has? > > I'm not concerned. If you want to add it, it's fine with me. I'm not asking > you to add it. > Acknowledged. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:258: } On 2014/09/05 22:18:15, Mark P wrote: > Consider: > else { clear all relevant data } > Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:896: // here for non-Answers Chrome, to prevent any impact on timing measurements. On 2014/09/05 22:18:16, Mark P wrote: > The reason for this is not "to prevent any impact on timing measurements", > rather it's to prevent performance regressions from moving the scoring code > before the suggest request is sent. Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:897: // Answers FieldTrials will instead use scoring results obtained previously. On 2014/09/05 22:18:16, Mark P wrote: > nit: "Answers FieldTrials" is awkward. Consider replacing the sentence. > Something like "For users with Answers enabled, the history results will be > scored earlier." Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:900: if (!OmniboxFieldTrial::EnableAnswersInSuggest()) { On 2014/09/05 22:18:16, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > Another repost: Do we want to run unit tests both with and without Answers > > enabled? > > It's up to you. > > My typical strategy when I add a field trial: > * add test(s) if necessary that specifically exercise the new code. > * run all tests in a local build that has the field trial on for everyone > I don't bother duplicating tests for field-trial-on and field-trial-off unless > they should/do behave differently under those cases. They _shouldn't_ behave differently except for Answers, which is covered. I'll give the local run a spin. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:914: // TODO(groby): Can this be merged with AddSuggestResultsToMap? On 2014/09/05 22:18:15, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > I don't think it can because AddSuggestResults because it sets > > |accepted_suggestion| to the index of the result, while AddHistoryResults sets > > the index to true. (Which, BTW, is odd. Is that intended?) > > You're reading this wrong. It sets |mark_as_deletable| to true, which is > accurate. > > The |accepted_suggestion| variable is set to |did_not_accept_suggestion|, which > seems appropriate for a suggestion that didn't come from the server. (This > |accepted_suggestion| variable is used to send feedback to the server.) > > You can try combining these two functions into one. I'm not sure it's worth the > time. Skipping the merging, then. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:919: if (results.empty()) On 2014/09/05 22:18:16, Mark P wrote: > I'm not sure the results.empty() short-circuit is worth anything. Please remove > it. Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1045: if (results.empty()) On 2014/09/05 22:18:16, Mark P wrote: > Shouldn't we clear scored_results in this case? Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1049: input_.prevent_inline_autocomplete() || On 2014/09/05 22:18:16, Mark P wrote: > nit: can't this go on the previous line? It can. I blame git cl format. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1052: is_keyword ? keyword_input_.text() : input_.text(); On 2014/09/05 22:18:15, Mark P wrote: > Consider GetInput(is_keyword).text() Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1071: if (scored_results->empty()) On 2014/09/05 22:18:15, Mark P wrote: > nit: { } Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1321: // results, duplicates be damned. On 2014/09/05 22:18:16, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > Re-use of the partial map extends the state the SearchProvider carries around, > > but it seems possible - except that it slightly messes with "prefer items > added > > first". This only affects verbatim matches, AFAICT, which should already be > > scored sufficiently high. > > I'm not sure I follow your proposal here. We're building the MatchMap twice - once here, once in ConvertResultsToAutocompleteMatches. The latter adds more than the history results, though. After looking at this a bit more, sharing the map carries more complexity than I'd like to add, so discarding that. The alternate proposal - "scan through results" would just sort the scored results, without building a MatchMap. That relies on AddMatchToMap never modifying the score, and would require re-implementing the logic in AutocompleteMatch::MoreRelevant - so I'm discarding that too, for now. > > Note that verbatim matches may vary in score nowadays. They can score as low as > 851. Lots of things will beat them. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: // This seems to only matter for AQS.... On 2014/09/05 22:18:16, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > See question above. > > > > If I share state, I'll obviously need to pass in did_not_accept_suggestion. > > ditto: I'm not sure I follow your proposal here. Shorter: Does it matter that AddTransformed...() is passed TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|? As far as I can tell, it does not, and it correctly reflects that we don't have suggestions at this point, so I'll go with that. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: // This seems to only matter for AQS.... On 2014/09/05 22:18:15, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > See question above. > > > > This might also be relevant when I share state as discussed above. The > question > > is, is did_not_accept_suggestion available before all results are in? > > No. In fact, the value of did_not_accept_suggestion depends on whether results > are in. > int did_not_accept_default_suggestion = > default_results_.suggest_results.empty() ? > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE : > TemplateURLRef::NO_SUGGESTION_CHOSEN; > https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... Acknowledged. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1325: AddScoredHistoryResultsToMap(scored_keyword_history_results_, false, &map); On 2014/09/05 22:18:16, Mark P wrote: > Please do not pass false for an int without an explicit cast. > > Also, more importantly, false is likely the wrong value, regardless. Please use > the right value. It is indeed - thanks for catching this. I read the prototype wrong. > Note that given my other comment, you might not be able to > have the right value at this point. I should figure out if it's important to > have the right value in this variable. If so, you need to correct its value > later. I can't correct it later - the MatchMap is scoped to this function. > If not, you should comment here about how it doesn't matter. (And > perhaps comment in the .h by this function and possibly in the .h by the > internal variable that gets assigned this as a result that this particular field > is set incorrectly.) This function discards everything in MatchMap, so nothing is assigned an incorrect result. I've modified AddTransformedHistoryResultsToMap so that it does not affect any members, but takes a pointer to modified objects. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:251: // Adds a match for each scored result in |results| to |map|. On 2014/09/05 22:18:16, Mark P wrote: > nit: missing blank line Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:251: // Adds a match for each scored result in |results| to |map|. On 2014/09/05 22:18:16, Mark P wrote: > I don't really like the notion of calling these "scored" results. While it's > true that they have relevance scores, I think the bigger difference is that > these results are already converted into SuggestResults. Can you think of > better terminology? Perhaps the first function could be > AddRawHistoryResultsToMap() and the second could be > AddTransformedHistoryResultsToMap(). Or perhaps you have a better idea? Not really, so I'll stick with the suggestion :) > > If you take this suggestion, please search through this changelist for "scored" > and revise the variable names and comments appropriately. And maybe change the > original un-scored variable names to "raw" or whatever you decide. Done. > https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:264: void ScoreHistoryResultsMultiWord( On 2014/09/05 22:18:16, Mark P wrote: > I originally read this and added the comment > "optional nit: MultiWord -> MultiWordInput" > but reading the code I find that I'm interpreting this wrong. > > What is it supposed to mean? Please give it a better name. The name is based on the comment in DoHistoryQuery[1] and AddHistoryResultsToMap[2] As far as I can tell, it's essentially adjusting scoring for multi-words if the current input is a prefix for a multi-word suggestion. The original is still doing scoring, this is just doing adjustments to that scoring for specific multi-word cases. So "Helper" would not really capture it. And I can't come up with a good one, except maybe ScoreHistoryResultsAdjusted. Which is not too hot either. Ideas? [1] https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... [2] https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... > > Here's one idea: isn't this now the primary ScoreHistoryResults function and > the original is merely a helper function, e.g., ScoreHistoryResultsHelper? https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:268: // Adds matches for |results| to |map|. On 2014/09/05 22:18:16, Mark P wrote: > nit: missing blank line Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:321: // current |input| and sets |prefetch_data_|. On 2014/09/05 22:18:16, Mark P wrote: > This comment doesn't seem appropriate anymore. Please revise. Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:347: // Scored searches in the user's history - based on xxx_history_results_. On 2014/09/05 22:18:16, Mark P wrote: > nit: xxx_history_results_. -> keyword_history_results_ or > default_history_results_ as appropriate. Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3260: query, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, On 2014/09/05 22:18:17, Mark P wrote: > WHAT_YOU_TYPED seems inappropriate Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3263: true, false, query); On 2014/09/05 22:18:17, Mark P wrote: > This true (relevance from server) should be false if you want to create > something that looks like a history result. Done. https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3265: QueryForInput(ASCIIToUTF16("weather l"), false, false); On 2014/09/05 22:18:17, Mark P wrote: > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > QueryForInput is only called to set up providers_ - does that warrant a > comment? > Nah. > Acknowledged.
Friendly ping :) Also, test results with field trial on are at https://codereview.chromium.org/558293004 I consider it "sufficiently green" to be confident the flag doesn't have unexpected side effect. (some swarming/android/ios bots exploded as usual)
Sorry, I've been sick. I'll get to it this week. On Mon, Sep 15, 2014 at 12:57 PM, <groby@chromium.org> wrote: > Friendly ping :) > > Also, test results with field trial on are at > https://codereview.chromium.org/558293004 > > I consider it "sufficiently green" to be confident the flag doesn't have > unexpected side effect. (some swarming/android/ios bots exploded as usual) > > > https://codereview.chromium.org/470313004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you, and hope you're better! On Mon, Sep 15, 2014 at 2:19 PM, Mark Pearson <mpearson@chromium.org> wrote: > > Sorry, I've been sick. I'll get to it this week. > > On Mon, Sep 15, 2014 at 12:57 PM, <groby@chromium.org> wrote: > >> Friendly ping :) >> >> Also, test results with field trial on are at >> https://codereview.chromium.org/558293004 >> >> I consider it "sufficiently green" to be confident the flag doesn't have >> unexpected side effect. (some swarming/android/ios bots exploded as usual) >> >> >> https://codereview.chromium.org/470313004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Getting pretty close now... --mark https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1321: // results, duplicates be damned. On 2014/09/10 23:21:35, groby wrote: > On 2014/09/05 22:18:16, Mark P wrote: > > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > > Re-use of the partial map extends the state the SearchProvider carries > around, > > > but it seems possible - except that it slightly messes with "prefer items > > added > > > first". This only affects verbatim matches, AFAICT, which should already be > > > scored sufficiently high. > > > > I'm not sure I follow your proposal here. > > We're building the MatchMap twice - once here, once in > ConvertResultsToAutocompleteMatches. The latter adds more than the history > results, though. After looking at this a bit more, sharing the map carries more > complexity than I'd like to add, so discarding that. > > The alternate proposal - "scan through results" would just sort the scored > results, without building a MatchMap. That relies on AddMatchToMap never > modifying the score, and would require re-implementing the logic in > AutocompleteMatch::MoreRelevant - so I'm discarding that too, for now. > > > > > Note that verbatim matches may vary in score nowadays. They can score as low > as > > 851. Lots of things will beat them. > Acknowledged. https://codereview.chromium.org/470313004/diff/120001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/470313004/diff/120001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3423: std::string(), false, 1200, true, false, query); On 2014/09/10 23:21:38, groby wrote: > On 2014/09/05 22:18:17, Mark P wrote: > > This true (relevance from server) should be false if you want to create > > something that looks like a history result. > > Done. Not done. It looks like the third to last parameter is still true. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:975: // here for non-Answers Chrome, to prevent scoring regressions from moving the nit: ^scoring^performance (I think) If you are worried about scoring regressions, please tell me why. (I don't think there could be any.) https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:987: transformed_results = is_keyword ? &transformed_keyword_history_results_ On 2014/09/10 23:21:33, groby wrote: > On 2014/09/05 22:18:16, Mark P wrote: > > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > > See question above. > > > > > > If I share state, I'll obviously need to pass in did_not_accept_suggestion. > > > > ditto: I'm not sure I follow your proposal here. > > Shorter: Does it matter that AddTransformed...() is passed > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|? > > As far as I can tell, it does not, and it correctly reflects that we don't have > suggestions at this point, so I'll go with that. I'd prefer if you could add somewhere (possibly here) that, before using transformed_history_results, munges them to revise whether suggestions are available at this instant. Otherwise when answers is enabled all our history results will be annotated with "no suggestions available", even if suggestions are available. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1408: // results, duplicates be damned. Given that you've discarded this idea (fine by me), perhaps you want to remove the TODO? https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1411: // This seems to only matter for AQS.... On 2014/09/10 23:21:33, groby wrote: > On 2014/09/05 22:18:16, Mark P wrote: > > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > > See question above. > > > > > > If I share state, I'll obviously need to pass in did_not_accept_suggestion. > > > > ditto: I'm not sure I follow your proposal here. > > Shorter: Does it matter that AddTransformed...() is passed > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|? > > As far as I can tell, it does not, and it correctly reflects that we don't have > suggestions at this point, so I'll go with that. I'm comfortable with this here. As I understand it, we only use the results of this AddTransformedHistoryResultsToMap() call to calculate prefetch_data, and we never look at the suggestions_available field in prefetch_data. Please correct me if I'm wrong. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1424: LOG(ERROR) << "SIZE:" << matches.size(); nit: remove unneeded LOG lines https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1428: StartsWith(matches[0].contents, input, false /* case-insensitive */)) { Minor question: why this is StartsWith necessary? What's the harm if it's missing? https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.h:270: void ScoreHistoryResultsMultiWord( On 2014/09/10 23:21:38, groby wrote: > On 2014/09/05 22:18:16, Mark P wrote: > > I originally read this and added the comment > > "optional nit: MultiWord -> MultiWordInput" > > but reading the code I find that I'm interpreting this wrong. > > > > What is it supposed to mean? Please give it a better name. > > The name is based on the comment in DoHistoryQuery[1] and > AddHistoryResultsToMap[2] > > As far as I can tell, it's essentially adjusting scoring for multi-words if the > current input is a prefix for a multi-word suggestion. > > The original is still doing scoring, this is just doing adjustments to that > scoring for specific multi-word cases. So "Helper" would not really capture it. > And I can't come up with a good one, except maybe ScoreHistoryResultsAdjusted. > Which is not too hot either. Ideas? I know what's bothering me. ScoreHistoryResultsMultiWord() reads like it's going to handle a subset of cases (merely multi-word cases) that ScoreHistoryResults() will handle. Yet this isn't the case. It's actually the opposite. We need a name to align our intuition with the code. That's why I proposed this function being named ScoreHistoryResults() (it can handle any situation) and the original ScoreHistoryResults() being named some sort of helper function (it can do some things, but those things are sometimes adjusted or ignored later). > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... > > > > Here's one idea: isn't this now the primary ScoreHistoryResults function and > > the original is merely a helper function, e.g., ScoreHistoryResultsHelper? > https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.h:329: // This only happens if |input| is a prefix for the top-scoring result. On 2014/09/10 23:21:38, groby wrote: > On 2014/09/05 22:18:16, Mark P wrote: > > This comment doesn't seem appropriate anymore. Please revise. > > Done. This comment is still not appropriate. It, for instance, should explain the return value. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.h:356: // or default_history_results_ as appropriate. nit: be consistent about either using || or not.
PTAL - with one major question around the suggestion annotation issue. https://codereview.chromium.org/470313004/diff/120001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/470313004/diff/120001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3423: std::string(), false, 1200, true, false, query); On 2014/09/16 22:24:55, Mark P wrote: > On 2014/09/10 23:21:38, groby wrote: > > On 2014/09/05 22:18:17, Mark P wrote: > > > This true (relevance from server) should be false if you want to create > > > something that looks like a history result. > > > > Done. > > Not done. It looks like the third to last parameter is still true. Done. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:975: // here for non-Answers Chrome, to prevent scoring regressions from moving the On 2014/09/16 22:24:55, Mark P wrote: > nit: ^scoring^performance > (I think) > > If you are worried about scoring regressions, please tell me why. (I don't > think there could be any.) performance regressions, indeed. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:987: transformed_results = is_keyword ? &transformed_keyword_history_results_ On 2014/09/16 22:24:55, Mark P wrote: > On 2014/09/10 23:21:33, groby wrote: > > On 2014/09/05 22:18:16, Mark P wrote: > > > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > > > See question above. > > > > > > > > If I share state, I'll obviously need to pass in > did_not_accept_suggestion. > > > > > > ditto: I'm not sure I follow your proposal here. > > > > Shorter: Does it matter that AddTransformed...() is passed > > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|? > > > > As far as I can tell, it does not, and it correctly reflects that we don't > have > > suggestions at this point, so I'll go with that. > > I'd prefer if you could add somewhere (possibly here) that, before using > transformed_history_results, munges them to revise whether suggestions are > available at this instant. Otherwise when answers is enabled all our history > results will be annotated with "no suggestions available", even if suggestions > are available. I'm not sure I understand why results would be annotated improperly. Unless I misunderstand, the annotation should ultimately be stored stored in the AutoCompleteMatch's |search_term_args.accepted_suggestion|, correct? Here is what happens when Answers is enabled: 1) Right after DoHistoryQuery, transformed_..._results is populated. As far as I can tell, "suggestion available" doesn't matter when populating SuggestResults 2) This function - AddRawHistoryResultsToMap() - is called from ConvertResultsToAutocompleteMatches. |did_not_accept_suggestions| is passed in from there, and is computed there: int did_not_accept_default_suggestion = default_results_.suggest_results.empty() ? TemplateURLRef::NO_SUGGESTIONS_AVAILABLE : TemplateURLRef::NO_SUGGESTION_CHOSEN; 3) AddTransformedHistoryResultsToMap() calls AddMatchToMap(), passing through |did_not_accept_suggestion|. 4) AddMatchToMap converts the SuggestResult into an AutocompleteMatch, once more passing through |did_not_accept_suggestion|, using it to set |match.search_terms_args->accepted_suggestion| https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1408: // results, duplicates be damned. On 2014/09/16 22:24:55, Mark P wrote: > Given that you've discarded this idea (fine by me), perhaps you want to remove > the TODO? Done. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1411: // This seems to only matter for AQS.... On 2014/09/16 22:24:55, Mark P wrote: > On 2014/09/10 23:21:33, groby wrote: > > On 2014/09/05 22:18:16, Mark P wrote: > > > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > > > See question above. > > > > > > > > If I share state, I'll obviously need to pass in > did_not_accept_suggestion. > > > > > > ditto: I'm not sure I follow your proposal here. > > > > Shorter: Does it matter that AddTransformed...() is passed > > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|? > > > > As far as I can tell, it does not, and it correctly reflects that we don't > have > > suggestions at this point, so I'll go with that. > > I'm comfortable with this here. As I understand it, we only use the results of > this AddTransformedHistoryResultsToMap() call to calculate prefetch_data, and we > never look at the suggestions_available field in prefetch_data. Please correct > me if I'm wrong. Correct, so removed comment. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1424: LOG(ERROR) << "SIZE:" << matches.size(); On 2014/09/16 22:24:55, Mark P wrote: > nit: remove unneeded LOG lines Done. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1428: StartsWith(matches[0].contents, input, false /* case-insensitive */)) { On 2014/09/16 22:24:55, Mark P wrote: > Minor question: why this is StartsWith necessary? What's the harm if it's > missing? By missing you mean leaving the test out completely? As long as history results are always prefix-matching on input, nothing. Probably just paranoia on my part, so taking it out. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.h:270: void ScoreHistoryResultsMultiWord( On 2014/09/16 22:24:55, Mark P wrote: > On 2014/09/10 23:21:38, groby wrote: > > On 2014/09/05 22:18:16, Mark P wrote: > > > I originally read this and added the comment > > > "optional nit: MultiWord -> MultiWordInput" > > > but reading the code I find that I'm interpreting this wrong. > > > > > > What is it supposed to mean? Please give it a better name. > > > > The name is based on the comment in DoHistoryQuery[1] and > > AddHistoryResultsToMap[2] > > > > As far as I can tell, it's essentially adjusting scoring for multi-words if > the > > current input is a prefix for a multi-word suggestion. > > > > The original is still doing scoring, this is just doing adjustments to that > > scoring for specific multi-word cases. So "Helper" would not really capture > it. > > And I can't come up with a good one, except maybe ScoreHistoryResultsAdjusted. > > Which is not too hot either. Ideas? > > I know what's bothering me. ScoreHistoryResultsMultiWord() reads like it's > going to handle a subset of cases (merely multi-word cases) that > ScoreHistoryResults() will handle. Yet this isn't the case. It's actually the > opposite. We need a name to align our intuition with the code. > > That's why I proposed this function being named ScoreHistoryResults() (it can > handle any situation) and the original ScoreHistoryResults() being named some > sort of helper function (it can do some things, but those things are sometimes > adjusted or ignored later). I'll stick with helper. I've spent 30 minutes with a thesaurus without results :) > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox... > > > > > > Here's one idea: isn't this now the primary ScoreHistoryResults function > and > > > the original is merely a helper function, e.g., ScoreHistoryResultsHelper? > > https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.h:329: // This only happens if |input| is a prefix for the top-scoring result. On 2014/09/16 22:24:55, Mark P wrote: > On 2014/09/10 23:21:38, groby wrote: > > On 2014/09/05 22:18:16, Mark P wrote: > > > This comment doesn't seem appropriate anymore. Please revise. > > > > Done. > > This comment is still not appropriate. It, for instance, should explain the > return value. Hopefully, the new comment is a bit clearer. https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.h:356: // or default_history_results_ as appropriate. On 2014/09/16 22:24:55, Mark P wrote: > nit: be consistent about either using || or not. Done.
https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:987: transformed_results = is_keyword ? &transformed_keyword_history_results_ On 2014/09/18 23:27:24, groby wrote: > On 2014/09/16 22:24:55, Mark P wrote: > > On 2014/09/10 23:21:33, groby wrote: > > > On 2014/09/05 22:18:16, Mark P wrote: > > > > On 2014/08/28 21:36:08, groby (OOO until Sep. 8) wrote: > > > > > See question above. > > > > > > > > > > If I share state, I'll obviously need to pass in > > did_not_accept_suggestion. > > > > > > > > ditto: I'm not sure I follow your proposal here. > > > > > > Shorter: Does it matter that AddTransformed...() is passed > > > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|? > > > > > > As far as I can tell, it does not, and it correctly reflects that we don't > > have > > > suggestions at this point, so I'll go with that. > > > > I'd prefer if you could add somewhere (possibly here) that, before using > > transformed_history_results, munges them to revise whether suggestions are > > available at this instant. Otherwise when answers is enabled all our history > > results will be annotated with "no suggestions available", even if suggestions > > are available. > > I'm not sure I understand why results would be annotated improperly. Unless I > misunderstand, the annotation should ultimately be stored stored in the > AutoCompleteMatch's |search_term_args.accepted_suggestion|, correct? > > Here is what happens when Answers is enabled: > > 1) Right after DoHistoryQuery, transformed_..._results is populated. As far as I > can tell, "suggestion available" doesn't matter when populating SuggestResults > > 2) This function - AddRawHistoryResultsToMap() - is called from > ConvertResultsToAutocompleteMatches. |did_not_accept_suggestions| is passed in > from there, and is computed there: > int did_not_accept_default_suggestion = > default_results_.suggest_results.empty() ? > TemplateURLRef::NO_SUGGESTIONS_AVAILABLE : > TemplateURLRef::NO_SUGGESTION_CHOSEN; > > 3) AddTransformedHistoryResultsToMap() calls AddMatchToMap(), passing through > |did_not_accept_suggestion|. Thank you for the explanation. I see my worry was incorrect. I didn't notice that we pass through a newly-computed |did_not_accept_suggestion| when we add the transformed results to the map. (I was thinking that we used the same value we used when we computed the transformed results in the first place.) > > 4) AddMatchToMap converts the SuggestResult into an AutocompleteMatch, once more > passing through |did_not_accept_suggestion|, using it to set > |match.search_terms_args->accepted_suggestion| > https://codereview.chromium.org/470313004/diff/120001/components/omnibox/sear... components/omnibox/search_provider.cc:1428: StartsWith(matches[0].contents, input, false /* case-insensitive */)) { On 2014/09/18 23:27:24, groby wrote: > On 2014/09/16 22:24:55, Mark P wrote: > > Minor question: why this is StartsWith necessary? What's the harm if it's > > missing? > > By missing you mean leaving the test out completely? As long as history results > are always prefix-matching on input, nothing. Probably just paranoia on my part, > so taking it out. > Yes, search history matches should always be prefix matches. https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... components/omnibox/search_provider.cc:1136: // ScoreHistoryResults() allows autocompletion of multi-word, 1-visit ...Helper() ? (I think) https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... components/omnibox/search_provider.h:333: // AnswersQueryData. On 2014/09/18 23:27:24, groby wrote: > On 2014/09/16 22:24:55, Mark P wrote: > > On 2014/09/10 23:21:38, groby wrote: > > > On 2014/09/05 22:18:16, Mark P wrote: > > > > This comment doesn't seem appropriate anymore. Please revise. > > > > > > Done. > > > > This comment is still not appropriate. It, for instance, should explain the > > return value. > > Hopefully, the new comment is a bit clearer. A bit better is some ways, worse in others. The description for a function should mention the parameters. It now no longer mentions |input|.
PTAL https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... components/omnibox/search_provider.cc:1136: // ScoreHistoryResults() allows autocompletion of multi-word, 1-visit On 2014/09/19 18:45:36, Mark P wrote: > ...Helper() > ? > > (I think) I think you're right. https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/160001/components/omnibox/sear... components/omnibox/search_provider.h:333: // AnswersQueryData. On 2014/09/19 18:45:36, Mark P wrote: > On 2014/09/18 23:27:24, groby wrote: > > On 2014/09/16 22:24:55, Mark P wrote: > > > On 2014/09/10 23:21:38, groby wrote: > > > > On 2014/09/05 22:18:16, Mark P wrote: > > > > > This comment doesn't seem appropriate anymore. Please revise. > > > > > > > > Done. > > > > > > This comment is still not appropriate. It, for instance, should explain the > > > return value. > > > > Hopefully, the new comment is a bit clearer. > > A bit better is some ways, worse in others. The description for a function > should mention the parameters. It now no longer mentions |input|. That's because it isn't needed any more - removed. (|input| was only necessary for prefix matching, which as per prev. discussion can be assumed given for SEARCH_HISTORY matches)
lgtm https://codereview.chromium.org/470313004/diff/180001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/180001/components/omnibox/sear... components/omnibox/search_provider.h:330: // Answers prefetch handling - finds previously displayed answer matching the nit: finds -> find the https://codereview.chromium.org/470313004/diff/180001/components/omnibox/sear... components/omnibox/search_provider.h:332: // the query data associated with it. Otherwise, returns empty nit: returns empty -> returns an empty AnswersQueryData.
https://codereview.chromium.org/470313004/diff/180001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/180001/components/omnibox/sear... components/omnibox/search_provider.h:330: // Answers prefetch handling - finds previously displayed answer matching the On 2014/09/22 20:23:49, Mark P wrote: > nit: finds -> find the Done. https://codereview.chromium.org/470313004/diff/180001/components/omnibox/sear... components/omnibox/search_provider.h:332: // the query data associated with it. Otherwise, returns empty On 2014/09/22 20:23:49, Mark P wrote: > nit: returns empty -> returns an empty AnswersQueryData. Done.
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/470313004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/470313004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/470313004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Found one bug, likely is not your issue. https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... components/omnibox/search_provider.cc:969: if (raw_results.empty()) This test for early exit is only appropriate if AnswersInSuggest is off, right? If it's on, it doesn't matter if we have raw_results because we've already converted them to transformed results.
https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... components/omnibox/search_provider.cc:278: should we clear the raw_ results here because we don't need them anymore? https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... components/omnibox/search_provider.cc:966: bool is_keyword, It's a little awkward that we're passing the results and the keyword state, then using the keyword state to look up the transformed results. I suggest either passing in the keyword state and then looking up the raw & transformed results as need, or passing in the keyword state, raw results, and transformed results, and not looking anything up.
https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... components/omnibox/search_provider.cc:278: On 2014/09/25 17:07:26, Mark P wrote: > should we clear the raw_ results here because we don't need them anymore? We could, but a) that requires the fix you describe below, and b) I'm not sure we gain much by doing this. Again, I'll add this as a preventative fix. (If it would solve the swarming issue, I'd be very confused, though :) https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... components/omnibox/search_provider.cc:966: bool is_keyword, On 2014/09/25 17:07:27, Mark P wrote: > It's a little awkward that we're passing the results and the keyword state, then > using the keyword state to look up the transformed results. > I suggest either passing in the keyword state and then looking up the raw & > transformed results as need, or passing in the keyword state, raw results, and > transformed results, and not looking anything up. We need is_keyword for ScoreHistoryResults in the non-answers case. So I'll go and look up the raw results as well. https://codereview.chromium.org/470313004/diff/240001/components/omnibox/sear... components/omnibox/search_provider.cc:969: if (raw_results.empty()) On 2014/09/25 17:01:44, Mark P wrote: > This test for early exit is only appropriate if AnswersInSuggest is off, right? > If it's on, it doesn't matter if we have raw_results because we've already > converted them to transformed results. That is correct. (It doesn't matter, though, because empty raw results will have resulted in empty transformed results, which means nothing is added to the map) I'll add the fix anyways, just in case :)
Fixes uploaded. Trying a swarming run just because...
https://codereview.chromium.org/470313004/diff/260001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/260001/components/omnibox/sear... components/omnibox/search_provider.h:249: // Adds a match for each result in |results| to |map|. |is_keyword| indicates nit: please revise this comment so as not to reference |results|
Problem found - a reference that shouldn't have been a reference. Gah. https://codereview.chromium.org/470313004/diff/260001/components/omnibox/sear... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/470313004/diff/260001/components/omnibox/sear... components/omnibox/search_provider.h:249: // Adds a match for each result in |results| to |map|. |is_keyword| indicates On 2014/09/25 22:36:45, Mark P wrote: > nit: please revise this comment so as not to reference |results| Done.
On 2014/09/26 00:20:42, groby wrote: > Problem found - a reference that shouldn't have been a reference. Gah. Nice work. (yay for unit tests finding real issues!) --mark
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/470313004/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as 02b9cd558abf6ccc6f442c52ddd4d2ffafff689f
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/e5fcee487262976448cde70e40ac6bbe11660f6b Cr-Commit-Position: refs/heads/master@{#296873} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
