Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12090006:
Omnibox: Create Keyword Verbatim Result in Search Provider (Closed)
|
Created:
7 years, 11 months ago by Mark P Modified:
7 years, 10 months ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox: Create Keyword Verbatim Result in Search Provider
It's a little weird that SearchProvider creates the default provider's
search-what-you-typed (a.k.a. verbatim) match, the default provider's
suggestion matches, and the keyword provider's suggestions, but not
the keyword provider's search-what-you-typed match.
This change adds code to create the keyword-search-what-you-typed
suggestion (for keywords that aren't extensions) to SearchProvider. It
also modifies the code in KeywordProvider to prevent creating the
keyword-search-what-you-typed suggestions in these cases.
This is the natural step to fix the linked bug. It also preps the path to get suggested relevance scores for keyword providers.
It should have nearly no user-visible effect. The reason for the nearly no
and not a "no" is because now that the SearchProvider is generating
the keyword verbatim result, we allow the SearchProvider to return
an additional result to the AutocompleteController. (The keyword provider
would always return this result, so we leave an extra slot in SearchProvider's
result set for this result.) However, it's vaguely conceivable if we
have suggest relevance scoring and have more than four very-highly-scoring
suggestions that those suggestions could starve out the keyword verbatim
result. Frankly, I'm not worried about this.
Note that there is a logging change: the verbatim result for the
keyword provider's search was previously marked as Keyword
provider, SEARCH_OTHER_ENGINE result type. Now it will be
marked as Search provider, SEARCH_OTHER_ENGINE result type.
As for how I created CalculateRelevanceForKeywordVerbatim(), it's a direct
copy of KeywordProvider::CalculateRelevance() with certain tests removed
for things that are guaranteed to be true/false in SearchProvider if we're
using the keyword fetcher. I decided not to make this code in common
because there's no reason the Keyword scoring algorithm should
be the same as the SearchProvider's scoring algorithm. i.e., if one
later alters one of these functions, no harm will come as a result of
the fact that the code isn't shared.
BUG=171104
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181181
Patch Set 1 #Patch Set 2 : corrected tests. #Patch Set 3 : fix last failing test #Patch Set 4 : cleanup extension keyword handling #Patch Set 5 : more extension cleanup #
Total comments: 37
Patch Set 6 : Bart's comments plus more tests. #
Total comments: 6
Patch Set 7 : Bart's final comments. #
Total comments: 33
Patch Set 8 : Mike's comments. #Patch Set 9 : Mike's comments, plus more tests he requested. #Patch Set 10 : Better extension tests for SearchProvider. #
Total comments: 18
Patch Set 11 : Mike's comments. #
Total comments: 10
Patch Set 12 : Mike's comments. #
Total comments: 4
Patch Set 13 : Mike's final comments. #Patch Set 14 : Update comment. #
Total comments: 10
Patch Set 15 : parens #Patch Set 16 : revise comments #Patch Set 17 : rebased #Patch Set 18 : roperly resolved (variable rename) #Messages
Total messages: 34 (0 generated)
Hi Bart, Can you give this a painstakingly detailed review before I ask anyone else to take a look? I want your utmost confidence that this is doing what it promises. thanks, mark
On 2013/01/28 18:07:25, Mark P wrote: > Hi Bart, > > Can you give this a painstakingly detailed review before I ask anyone else to > take a look? I want your utmost confidence that this is doing what it promises. Hey Mark - I will send my feedback sometime later today/tonight, as I'm currently busy with non-Chrome specific stuff... I hope it's not blocking you.
Mark, I've gone with this change carefully and I believe it should work as you intended. If I wasn't sure about something, I posted a question in my comments. One thing I'm not entirely sure is the KP logic wrt extension apps, but we can discuss it offline. Sorry for the delay. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:288: const TemplateURL* template_url(model->GetTemplateURLForKeyword(keyword)); That's a weird way of initializing. Why no simply " = " as above and elsewhere? https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:291: if (remaining_input.empty() || is_extension_keyword) { This is getting ugly (many nested ifs and not to mention size of this method which definitely exceeds recommended 25 limit). Can you simplify it by negating the condition and returning, i.e.: if (!remaining_input.empty() && !is_extension_keyword) return; https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:165: // Exact keyword matches with remaining text should return nothing. Can you add a unit test for non empty remaining text and extension app should return something? https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:793: if (!keyword_input_text_.empty() && keyword_url && Most of the time keyword_input_text_ is empty, so it'd be worth to optimize this code by moving const TemplateURL* keyword_url = providers_.GetKeywordProviderURL() after the if condition. I know Peter doesn't like assignments in if clauses, but you can nest ifs if you want. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:794: !keyword_url->IsExtensionKeyword()) { It's not clear to me why we ignore extension keyword here. Mind adding a comment or pointing to relevant documentation? https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:837: // Allow additional match(es) for "what you typed" results if present. If I read the code correctly, we may end up with 5 matches, because what_you_typed_size can be 2. Is this expected? https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:838: const size_t max_total_matches = kMaxMatches + what_you_typed_size; Technically, this is not "what you typed" matches, because in case of SEARCH_OTHER_ENGINE case you've added above, we are adding partial query (keyword is stripped from the original query). I'd rename this variable to something like verbatim or "trivial_matches_size" etc. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:875: bool SearchProvider::IsTopMatchNotInlinable() const { Don't you want to modify this condition now to include SEARCH_OTHER_ENGINE as well? (see http://code.google.com/p/chromium/issues/detail?id=171104#c1). https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1103: return (type == AutocompleteInput::QUERY) ? 1450 : 1100; Nit: We don't need () around the first expression. It looks like the only case we can beat keyword verbatim suggestion is when we don't prefer keyword and input is non-query, e.g. URL. BTW, under which circumstances can we get keyword verbatim and prefer_keyword is false? https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:296: // Calculate the relevance score for the keyword verbatim result (used if Nit: We usually refer in 3rd-person when describing top level comment (i.e. "Calculates" vs "Calculate"). I know some of the comments are inconsistent, but most of them use the right form. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:298: static int CalculateRelevanceForKeywordVerbatim(AutocompleteInput::Type type, Note: As you've seen in my pending CL (12039053), this will most likely gets simplified by replacing type/prefer_keyword with AutocompleteInput& keyword_input. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:790: // Verifies AutocompleteControllers returns results (including keyword Nit: returns -> return (unless you meant one AutocompleteController). https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:791: // results) in the right order and sets descriptions for them correctly. sets -> set. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:791: // results) in the right order and sets descriptions for them correctly. sets -> set https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:815: EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance); Technically speaking, why GT and not GE? https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:818: EXPECT_FALSE(result.match_at(0).keyword.empty()); Honestly, I'd prefer exact assertion here, i.e. say exactly what keyword we are expecting. The current set of expectations is very vague. If you do that, you will not need the extra two EXPECTs you've added. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:827: } I think it would be worth to add a unit test that actually verifies that keyword verbatim type is SEARCH_OTHER_ENGINE.
https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:165: // Exact keyword matches with remaining text should return nothing. On 2013/01/29 18:50:42, Bart N. wrote: > Can you add a unit test for non empty remaining text and extension app should > return something? This may help: http://build.chromium.org/f/chromium/coverage/linux-debug/179094/total_covera...
Thanks for the detailed review. I think I've answered all your questions. Please take another look when you get a chance. thanks, mark https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:288: const TemplateURL* template_url(model->GetTemplateURLForKeyword(keyword)); On 2013/01/29 18:50:42, Bart N. wrote: > That's a weird way of initializing. Why no simply " = " as above and elsewhere? Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:291: if (remaining_input.empty() || is_extension_keyword) { On 2013/01/29 18:50:42, Bart N. wrote: > This is getting ugly (many nested ifs and not to mention size of this method > which definitely exceeds recommended 25 limit). > > Can you simplify it by negating the condition and returning, i.e.: > if (!remaining_input.empty() && !is_extension_keyword) > return; Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:165: // Exact keyword matches with remaining text should return nothing. On 2013/01/29 21:48:35, Bart N. wrote: > On 2013/01/29 18:50:42, Bart N. wrote: > > Can you add a unit test for non empty remaining text and extension app should > > return something? > This may help: > http://build.chromium.org/f/chromium/coverage/linux-debug/179094/total_covera... Acknowledged. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:165: // Exact keyword matches with remaining text should return nothing. On 2013/01/29 18:50:42, Bart N. wrote: > Can you add a unit test for non empty remaining text and extension app should > return something? Good question. I re-enabled a test in chrome/browser/extensions/api/omnibox/omnibox_apitest.cc that does precisely this. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:793: if (!keyword_input_text_.empty() && keyword_url && On 2013/01/29 18:50:42, Bart N. wrote: > Most of the time keyword_input_text_ is empty, so it'd be worth to optimize this > code by moving > const TemplateURL* keyword_url = providers_.GetKeywordProviderURL() > after the if condition. > > I know Peter doesn't like assignments in if clauses, but you can nest ifs if you > want. Done. (Perhaps I have too much respect for optimizing compilers.) https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:794: !keyword_url->IsExtensionKeyword()) { On 2013/01/29 18:50:42, Bart N. wrote: > It's not clear to me why we ignore extension keyword here. Mind adding a comment > or pointing to relevant documentation? Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:837: // Allow additional match(es) for "what you typed" results if present. On 2013/01/29 18:50:42, Bart N. wrote: > If I read the code correctly, we may end up with 5 matches, because > what_you_typed_size can be 2. Is this expected? Yes. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:838: const size_t max_total_matches = kMaxMatches + what_you_typed_size; On 2013/01/29 18:50:42, Bart N. wrote: > Technically, this is not "what you typed" matches, because in case of > SEARCH_OTHER_ENGINE case you've added above, we are adding partial query > (keyword is stripped from the original query). > > I'd rename this variable to something like verbatim or "trivial_matches_size" > etc. Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:875: bool SearchProvider::IsTopMatchNotInlinable() const { On 2013/01/29 18:50:42, Bart N. wrote: > Don't you want to modify this condition now to include SEARCH_OTHER_ENGINE as > well? (see http://code.google.com/p/chromium/issues/detail?id=171104#c1). I was thinking about doing that in another changelist, but I might as well do it now. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1103: return (type == AutocompleteInput::QUERY) ? 1450 : 1100; On 2013/01/29 18:50:42, Bart N. wrote: > Nit: We don't need () around the first expression. Done. > It looks like the only case we can beat keyword verbatim suggestion is when we > don't prefer keyword and input is non-query, e.g. URL. Correct. Though this will change in a follow-up changelist once I turn on suggested relevance scores for keyword providers. > BTW, under which circumstances can we get keyword verbatim and prefer_keyword is > false? If you paste text into the omnibox that starts with a keyword, the omnibox will not go into keyword mode. This means prefer_keyword is false and yet you'll get keyword suggestions. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:296: // Calculate the relevance score for the keyword verbatim result (used if On 2013/01/29 18:50:42, Bart N. wrote: > Nit: We usually refer in 3rd-person when describing top level comment (i.e. > "Calculates" vs "Calculate"). I know some of the comments are inconsistent, but > most of them use the right form. Cleaned up some nearby comments. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:298: static int CalculateRelevanceForKeywordVerbatim(AutocompleteInput::Type type, On 2013/01/29 18:50:42, Bart N. wrote: > Note: As you've seen in my pending CL (12039053), this will most likely gets > simplified by replacing type/prefer_keyword with AutocompleteInput& > keyword_input. Ack. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:790: // Verifies AutocompleteControllers returns results (including keyword On 2013/01/29 18:50:42, Bart N. wrote: > Nit: returns -> return (unless you meant one AutocompleteController). Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:790: // Verifies AutocompleteControllers returns results (including keyword On 2013/01/29 18:50:42, Bart N. wrote: > Nit: returns -> return (unless you meant one AutocompleteController). Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:791: // results) in the right order and sets descriptions for them correctly. On 2013/01/29 18:50:42, Bart N. wrote: > sets -> set. Done. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:791: // results) in the right order and sets descriptions for them correctly. On 2013/01/29 18:50:42, Bart N. wrote: > sets -> set Duplicate of another comment. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:815: EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance); On 2013/01/29 18:50:42, Bart N. wrote: > Technically speaking, why GT and not GE? There shouldn't be ties in this test. If there are, that's a problem. https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:818: EXPECT_FALSE(result.match_at(0).keyword.empty()); On 2013/01/29 18:50:42, Bart N. wrote: > Honestly, I'd prefer exact assertion here, i.e. say exactly what keyword we are > expecting. The current set of expectations is very vague. If you do that, you > will not need the extra two EXPECTs you've added. Mostly done. Did the exact equal asserts for the keyword provider keyword. Still did a EXPECT_NE comparison for the default provider; not sure what keyword that coming with. (When running the test it appears to arrive with "dummy"; I'm not sure where that's coming from so I don't want to test that exact name explicitly.) https://codereview.chromium.org/12090006/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:827: } On 2013/01/29 18:50:42, Bart N. wrote: > I think it would be worth to add a unit test that actually verifies that keyword > verbatim type is SEARCH_OTHER_ENGINE. Added to this test (and analogous ones).
LGTM Mark, this is looking very clean to me now. Please see a few minor comments, though. https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:400: // match should thus be a search-other-engine match iff Extra space before "match". https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:346: } I think "else" here was probably better than now (no need for extra return above." https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:139: // {ASCIIToUTF16("z a b c++"), 1, {GURL("a+++b+++c%2B%2B=z")}}, Why is this commented out?
Thanks for the review. Now to add more reviewers. https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_provider_unittest.cc:400: // match should thus be a search-other-engine match iff On 2013/01/30 21:36:21, Bart N. wrote: > Extra space before "match". Done. https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:346: } On 2013/01/30 21:36:21, Bart N. wrote: > I think "else" here was probably better than now (no need for extra return > above." Done. https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/11003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:139: // {ASCIIToUTF16("z a b c++"), 1, {GURL("a+++b+++c%2B%2B=z")}}, On 2013/01/30 21:36:21, Bart N. wrote: > Why is this commented out? Oops, I should've removed it. It was failing: it's an exact keyword match with remaining text. Done. (I think the test below covers this case nicely already.)
Hi Mike, Can you please look at this? I'd like the most eyes on it before I interrupt Peter's recuperation with this change. It gets rid of the ickiness in that other change I showed up that makes SearchProvider aware of the keyword_verbatim _score. It also paves the way to use suggested relevances in keyword mode for people who type google blah. thanks, mark
https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:290: profile_ && template_url->IsExtensionKeyword(); Is |profile_| actually required for extension keywords? (or was that a separate condition in the old check below?) https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:292: // We only create an exact match if remaining_input is empty or optional nits: |remaining_input| here and below and I prefer passive voice. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:329: } else if (input.matches_requested() == optional nit: don't split this line https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:436: int KeywordProvider::CalculateRelevance(AutocompleteInput::Type type, optional nit: add a decl comment that these scores should stay in sync with SearchProvider::CalculateRelevanceForKeywordVerbatim. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:109: // Exact matches should prevent returning inexact matches. nit: add a comment about the verbatim match suppression. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:181: {ASCIIToUTF16("a 1 2+ 3"), 3, {ASCIIToUTF16("Search aa for 1 2+ 3"), optional nit: wrap this and the case above like the "aaa" case on line 174. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && I realize the only SEARCH_OTHER_ENGINE match at the moment is verbatim, and I like how simple it is to check right now, but this may change in the future. You should either ensure the SEARCH_OTHER_ENGINE match is a keyword verbatim, or add a comment regarding this weakness in the check. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:297: // we have a keyword provider). optional nit: "if we have" doesn't really explain the condition. clarify with "if the input matches one of the profile's keywords." or whatever. Also, mention that the scores used should match those from KeywordProvider. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:790: // Verifies AutocompleteControllers return results (including keyword I would really like to see some additional tests here regarding the conditions to trigger the keyword verbatim result, as well as the content of the keyword verbatim match itself (check the handling of spaces the same way KeywordProviderTest does). https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:811: EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance); nit: isn't this scoring guaranteed by the match ordering? (redundant checks?) https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:829: nit: remove extra line.
Mike, Aside from the requested additional tests, I replied to your comments. Some require clarification. Please take another look. thanks, mark https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:290: profile_ && template_url->IsExtensionKeyword(); On 2013/01/31 00:38:16, msw wrote: > Is |profile_| actually required for extension keywords? > (or was that a separate condition in the old check below?) That was a condition in the original if test so I thought it was worth keeping. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:292: // We only create an exact match if remaining_input is empty or On 2013/01/31 00:38:16, msw wrote: > optional nits: |remaining_input| here and below and I prefer passive voice. Done. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:329: } else if (input.matches_requested() == On 2013/01/31 00:38:16, msw wrote: > optional nit: don't split this line Done. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:436: int KeywordProvider::CalculateRelevance(AutocompleteInput::Type type, On 2013/01/31 00:38:16, msw wrote: > optional nit: add a decl comment that these scores should stay in sync with > SearchProvider::CalculateRelevanceForKeywordVerbatim. It doesn't have to stay in sync. See the last paragraph in the changelist description. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:109: // Exact matches should prevent returning inexact matches. On 2013/01/31 00:38:16, msw wrote: > nit: add a comment about the verbatim match suppression. Done. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider_unittest.cc:181: {ASCIIToUTF16("a 1 2+ 3"), 3, {ASCIIToUTF16("Search aa for 1 2+ 3"), On 2013/01/31 00:38:16, msw wrote: > optional nit: wrap this and the case above like the "aaa" case on line 174. Done. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/01/31 00:38:16, msw wrote: > I realize the only SEARCH_OTHER_ENGINE match at the moment is verbatim, and I > like how simple it is to check right now, but this may change in the future. You > should either ensure the SEARCH_OTHER_ENGINE match is a keyword verbatim, or add > a comment regarding this weakness in the check. I like this idea: > ensure that SEARCH_OTHER_ENGINE match is a keyword verbatim How do you suggest I do that? It's not obvious to me that you do the analogous thing and verify SEARCH_WHAT_YOU_TYPED is the default engine verbatim. Some ideas: In the code to create the keyword verbatim match, DCHECK that input.text() = keyword + (some amount of spaces) + remaining_text? Simply rename SEARCH_OTHER_ENGINE globally to what it actually means: KEYWORD_SEARCH_WHAT_YOU_TYPED? Assert in tests that there is only one one SEARCH_OTHER_ENGINE result? (This already happens implicitly because we explicitly check the type of each result, and we EXPECT only one SEARCH_OTHER_ENGINE result.) https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:297: // we have a keyword provider). On 2013/01/31 00:38:16, msw wrote: > optional nit: "if we have" doesn't really explain the condition. clarify with > "if the input matches one of the profile's keywords." or whatever. Done. > Also, mention > that the scores used should match those from KeywordProvider. As mentioned in the changelist description this isn't necessary. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:790: // Verifies AutocompleteControllers return results (including keyword On 2013/01/31 00:38:16, msw wrote: > I would really like to see some additional tests here regarding the conditions > to trigger the keyword verbatim result, as well as the content of the keyword > verbatim match itself (check the handling of spaces the same way > KeywordProviderTest does). I'll do this soon. Right now I'll hit reply to see if you have more comments on the rest of this changelist. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:811: EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance); On 2013/01/31 00:38:16, msw wrote: > nit: isn't this scoring guaranteed by the match ordering? (redundant checks?) Not as far as I know. The controller resorts matches it receives; I don't think there's any policy that controllers must return matches in sorted order. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:829: On 2013/01/31 00:38:16, msw wrote: > nit: remove extra line. Done.
I hope I caught everything, looking forward to tests! https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:290: profile_ && template_url->IsExtensionKeyword(); On 2013/01/31 23:09:27, Mark P wrote: > On 2013/01/31 00:38:16, msw wrote: > > Is |profile_| actually required for extension keywords? > > (or was that a separate condition in the old check below?) > > That was a condition in the original if test so I thought it was worth keeping. I see that, but I asked if that was a separate condition. The question is, how does the presence of |profile_| factor into your new check on line 297? If it's unrelated, just keep it as a separate check in the conditional at line 306. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:436: int KeywordProvider::CalculateRelevance(AutocompleteInput::Type type, On 2013/01/31 23:09:27, Mark P wrote: > On 2013/01/31 00:38:16, msw wrote: > > optional nit: add a decl comment that these scores should stay in sync with > > SearchProvider::CalculateRelevanceForKeywordVerbatim. > > It doesn't have to stay in sync. See the last paragraph in the changelist > description. It still seems odd if they diverged, but I don't really understand why they should or shouldn't. Perhaps just mention the relationship? https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/01/31 23:09:27, Mark P wrote: > In the code to create the keyword verbatim match, DCHECK that input.text() = > keyword + (some amount of spaces) + remaining_text? Hmm, that might be a decent DCHECK to add there, regardless of this, but it may be overkill to check that here right now. > Simply rename SEARCH_OTHER_ENGINE globally to what it actually means: > KEYWORD_SEARCH_WHAT_YOU_TYPED? It seems like you'd need to split SEARCH_OTHER_ENGINE into perhaps SEARCH_KEYWORD and KEYWORD_WHAT_YOU_TYPED. If you do a mass-renaming, perhaps replace *_WHAT_YOU_TYPED with *_VERBATIM :) > Assert in tests that there is only one one SEARCH_OTHER_ENGINE result? (This > already happens implicitly because we explicitly check the type of each result, > and we EXPECT only one SEARCH_OTHER_ENGINE result.) That also sounds like a good idea for SearchProviderTest. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:811: EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance); On 2013/01/31 23:09:27, Mark P wrote: > On 2013/01/31 00:38:16, msw wrote: > > nit: isn't this scoring guaranteed by the match ordering? (redundant checks?) > > Not as far as I know. The controller resorts matches it receives; I don't think > there's any policy that controllers must return matches in sorted order. Okay, then this is good, thanks.
I added the tests you wanted (or at least a framework for them). If you like what you see, I'll fill in the missing TEST_F blocks that I commented. Please take another look. thanks, mark https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:290: profile_ && template_url->IsExtensionKeyword(); On 2013/02/01 20:52:37, msw wrote: > On 2013/01/31 23:09:27, Mark P wrote: > > On 2013/01/31 00:38:16, msw wrote: > > > Is |profile_| actually required for extension keywords? > > > (or was that a separate condition in the old check below?) > > > > That was a condition in the original if test so I thought it was worth > keeping. > > I see that, but I asked if that was a separate condition. The question is, how > does the presence of |profile_| factor into your new check on line 297? If it's > unrelated, just keep it as a separate check in the conditional at line 306. It's unrelated. Moved as you suggested. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:436: int KeywordProvider::CalculateRelevance(AutocompleteInput::Type type, On 2013/02/01 20:52:37, msw wrote: > On 2013/01/31 23:09:27, Mark P wrote: > > On 2013/01/31 00:38:16, msw wrote: > > > optional nit: add a decl comment that these scores should stay in sync with > > > SearchProvider::CalculateRelevanceForKeywordVerbatim. > > > > It doesn't have to stay in sync. See the last paragraph in the changelist > > description. > > It still seems odd if they diverged, but I don't really understand why they > should or shouldn't. Perhaps just mention the relationship? Done. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/02/01 20:52:37, msw wrote: > On 2013/01/31 23:09:27, Mark P wrote: > > In the code to create the keyword verbatim match, DCHECK that input.text() = > > keyword + (some amount of spaces) + remaining_text? > Hmm, that might be a decent DCHECK to add there, regardless of this, but it may > be overkill to check that here right now. Agreed that writing the DCHECK is overkill (lots of whitespace possibilities to handle). Instead, I'll make sure the tests handle whitespace. > > Simply rename SEARCH_OTHER_ENGINE globally to what it actually means: > > KEYWORD_SEARCH_WHAT_YOU_TYPED? > > It seems like you'd need to split SEARCH_OTHER_ENGINE into perhaps > SEARCH_KEYWORD and KEYWORD_WHAT_YOU_TYPED. Oooh, good point. I don't want to do anything that'll require changing the UMA logs data. Pass. > If you do a mass-renaming, perhaps > replace *_WHAT_YOU_TYPED with *_VERBATIM :) > > > Assert in tests that there is only one one SEARCH_OTHER_ENGINE result? (This > > already happens implicitly because we explicitly check the type of each > result, > > and we EXPECT only one SEARCH_OTHER_ENGINE result.) > > That also sounds like a good idea for SearchProviderTest. Added more tests.
Tests are looking good, thanks! https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/02/02 00:24:35, Mark P wrote: > On 2013/02/01 20:52:37, msw wrote: > > On 2013/01/31 23:09:27, Mark P wrote: > > > In the code to create the keyword verbatim match, DCHECK that input.text() = > > > keyword + (some amount of spaces) + remaining_text? > > Hmm, that might be a decent DCHECK to add there, regardless of this, but it > may > > be overkill to check that here right now. > > Agreed that writing the DCHECK is overkill (lots of whitespace possibilities to > handle). Instead, I'll make sure the tests handle whitespace. It might not be too tough if you use .EndsWith, etc. Unless the trailing whitespace issues would be problematic here too. > > > > Simply rename SEARCH_OTHER_ENGINE globally to what it actually means: > > > KEYWORD_SEARCH_WHAT_YOU_TYPED? > > > > It seems like you'd need to split SEARCH_OTHER_ENGINE into perhaps > > SEARCH_KEYWORD and KEYWORD_WHAT_YOU_TYPED. > > Oooh, good point. I don't want to do anything that'll require changing the UMA > logs data. Pass. Too bad, that sounded nice. > > > If you do a mass-renaming, perhaps > > replace *_WHAT_YOU_TYPED with *_VERBATIM :) > > > > > Assert in tests that there is only one one SEARCH_OTHER_ENGINE result? > (This > > > already happens implicitly because we explicitly check the type of each > > result, > > > and we EXPECT only one SEARCH_OTHER_ENGINE result.) > > > > That also sounds like a good idea for SearchProviderTest. > > Added more tests. I'd still very much like to see a stronger check or a mention somewhere (even a comment/TODO here) that this assumes the SEARCH_OTHER_ENGINE result is a keyword verbatim match, even though it's not checked. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:434: // Perhaps this should be kept similar to nit: comment inside the function or at the declaration. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1109: // Perhaps this should be kept similar to nit: comment inside the function or at the declaration. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:188: data.SetURL("http://defaulturl/{searchTerms}"); The extra 't' stands for 'template', but this is ok if the test still passes. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:291: ResultType AutocompleteMatch::* member) { nit: I'm unfamiliar with this syntax, but others seem to omit the space after * https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:301: if (matches.size() == cases[i].num_results) { optional nit: it's okay to ignore this since you check it above. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:871: {ASCIIToUTF16("k foo"), 2, {GURL("http://keyword/foo"), nit: spaces after '{' here and elsewhere https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:874: // Make sure extra whitespace after the keyword doesn't change the What about whitespace before the keyword? https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:890: {ASCIIToUTF16("www.k foo"), 2, {GURL("http://keyword/foo"), Definitely also check .com if that's applicable (or maybe that's just tab-to-search and not keywords). Optionally also check http://, https:// http://www., etc. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:911: // This is similar to the above test except it checks the fill_into_edit You could add these other types to the same TestData struct and check all three at the same time; that would also alleviate templating, afaict.
Hi Mike, Here's all the cleanup you wanted. --mark https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/02/02 02:27:52, msw wrote: > On 2013/02/02 00:24:35, Mark P wrote: > > On 2013/02/01 20:52:37, msw wrote: > > > On 2013/01/31 23:09:27, Mark P wrote: > > > > In the code to create the keyword verbatim match, DCHECK that input.text() > = > > > > keyword + (some amount of spaces) + remaining_text? > > > Hmm, that might be a decent DCHECK to add there, regardless of this, but it > > may > > > be overkill to check that here right now. > > > > Agreed that writing the DCHECK is overkill (lots of whitespace possibilities > to > > handle). Instead, I'll make sure the tests handle whitespace. > > It might not be too tough if you use .EndsWith, etc. Unless the trailing > whitespace issues would be problematic here too. I tried to write a test that would work (verify the SEARCH_OTHER_ENGINE result is effectively verbatim) but given the different ways the same keyword can be expressed (bing.com vs www.bing.com versus http://bing.com etc.) I found it too difficult without a ton of code. :( > > > > > > Simply rename SEARCH_OTHER_ENGINE globally to what it actually means: > > > > KEYWORD_SEARCH_WHAT_YOU_TYPED? > > > > > > It seems like you'd need to split SEARCH_OTHER_ENGINE into perhaps > > > SEARCH_KEYWORD and KEYWORD_WHAT_YOU_TYPED. > > > > Oooh, good point. I don't want to do anything that'll require changing the > UMA > > logs data. Pass. > > Too bad, that sounded nice. > > > > > > If you do a mass-renaming, perhaps > > > replace *_WHAT_YOU_TYPED with *_VERBATIM :) > > > > > > > Assert in tests that there is only one one SEARCH_OTHER_ENGINE result? > > (This > > > > already happens implicitly because we explicitly check the type of each > > > result, > > > > and we EXPECT only one SEARCH_OTHER_ENGINE result.) > > > > > > That also sounds like a good idea for SearchProviderTest. > > > > Added more tests. > > I'd still very much like to see a stronger check or a mention somewhere (even a > comment/TODO here) that this assumes the SEARCH_OTHER_ENGINE result is a keyword > verbatim match, even though it's not checked. Added comments here and also where the verbatim match is created. Hopefully these comments will be enough (given the lack of an explicit test). https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:434: // Perhaps this should be kept similar to On 2013/02/02 02:27:52, msw wrote: > nit: comment inside the function or at the declaration. Done. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1109: // Perhaps this should be kept similar to On 2013/02/02 02:27:52, msw wrote: > nit: comment inside the function or at the declaration. Done. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:188: data.SetURL("http://defaulturl/{searchTerms}"); On 2013/02/02 02:27:52, msw wrote: > The extra 't' stands for 'template', but this is ok if the test still passes. Okay. I thought it was a typo. Put back, and tweaked the test that I had that depends on it. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:291: ResultType AutocompleteMatch::* member) { On 2013/02/02 02:27:52, msw wrote: > nit: I'm unfamiliar with this syntax, but others seem to omit the space after * Left as is. I copied this format from keyword provider's test, so I'm sticking with the local autocomplete style for this. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:301: if (matches.size() == cases[i].num_results) { On 2013/02/02 02:27:52, msw wrote: > optional nit: it's okay to ignore this since you check it above. Left as is. It's not okay because cases could be longer and then the test would segfault. I guess you'd still be notified but then the other parts of the test wouldn't run. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:871: {ASCIIToUTF16("k foo"), 2, {GURL("http://keyword/foo"), On 2013/02/02 02:27:52, msw wrote: > nit: spaces after '{' here and elsewhere Done. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:874: // Make sure extra whitespace after the keyword doesn't change the On 2013/02/02 02:27:52, msw wrote: > What about whitespace before the keyword? Commented. https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:890: {ASCIIToUTF16("www.k foo"), 2, {GURL("http://keyword/foo"), On 2013/02/02 02:27:52, msw wrote: > Definitely also check .com if that's applicable (or maybe that's just > tab-to-search and not keywords). Optionally also check http://, https:// > http://www., etc. Added some tests. (BTW, you cannot add a .com; normally that's part of the keyword, not an ignorable extra piece.) https://codereview.chromium.org/12090006/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:911: // This is similar to the above test except it checks the fill_into_edit On 2013/02/02 02:27:52, msw wrote: > You could add these other types to the same TestData struct and check all three > at the same time; that would also alleviate templating, afaict. Done.
Looking mostly good; please try URLPrefix::BestURLPrefix. https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/02/04 19:46:09, Mark P wrote: > I tried to write a test that would work (verify the SEARCH_OTHER_ENGINE result > is effectively verbatim) but given the different ways the same keyword can be > expressed (http://bing.com vs http://www.bing.com versus http://bing.com etc.) I found it too > difficult without a ton of code. :( Please try using URLPrefix::BestURLPrefix. https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:71: struct result_info { nit: ResultInfo (struct types should have CamelCase names). https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:73: gurl(GURL("")), nit: omit; the default ctor will do this. https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:75: fill_into_edit(ASCIIToUTF16("")) { nit: omit; the default string16 ctor will do this. https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:88: struct test_data { nit: TestData (struct types should have CamelCase names). https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:309: ASCIIToUTF16("Input was: ") + cases[i].input << nit: cache a local string to use in these 4 checks.
Mike, Here ya go. --mark https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/13003/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:887: matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE && On 2013/02/04 22:51:39, msw wrote: > On 2013/02/04 19:46:09, Mark P wrote: > > I tried to write a test that would work (verify the SEARCH_OTHER_ENGINE result > > is effectively verbatim) but given the different ways the same keyword can be > > expressed (http://bing.com vs http://www.bing.com versus http://bing.com etc.) > I found it too > > difficult without a ton of code. :( > > Please try using URLPrefix::BestURLPrefix. I was hoping you'd let me get away without this check. The ugly mass is now there in UpdateMatches(). https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:71: struct result_info { On 2013/02/04 22:51:39, msw wrote: > nit: ResultInfo (struct types should have CamelCase names). Done. (Trouble from copying the structure in KeywordProviderTest.) https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:73: gurl(GURL("")), On 2013/02/04 22:51:39, msw wrote: > nit: omit; the default ctor will do this. Done. https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:75: fill_into_edit(ASCIIToUTF16("")) { On 2013/02/04 22:51:39, msw wrote: > nit: omit; the default string16 ctor will do this. Done. https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:88: struct test_data { On 2013/02/04 22:51:39, msw wrote: > nit: TestData (struct types should have CamelCase names). Done. https://codereview.chromium.org/12090006/diff/28002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:309: ASCIIToUTF16("Input was: ") + cases[i].input << On 2013/02/04 22:51:39, msw wrote: > nit: cache a local string to use in these 4 checks. Done.
https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:902: #ifdef DEBUG Shoot, I thought you were going to put this into a unit test; it's probably overkill here. But now that I think about it, you already test for the the content of the keyword matches, so maybe adding a test like that isn't necessary at all... sorry. https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:72: ResultInfo(): nit: either combine this line and the line below with a space before ':', or move the ':' onto the next line, indented 4 spaces from 'ResultInfo()' with a space between ':' and result_type.
Now I think it's time to get Peter's eyes on this. https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:902: #ifdef DEBUG On 2013/02/05 01:35:19, msw wrote: > Shoot, I thought you were going to put this into a unit test; it's probably > overkill here. But now that I think about it, you already test for the the > content of the keyword matches, so maybe adding a test like that isn't necessary > at all... sorry. Removed. At least it's now in the code review history so if we want it back we can easily bring it back. https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/12090006/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider_unittest.cc:72: ResultInfo(): On 2013/02/05 01:35:19, msw wrote: > nit: either combine this line and the line below with a space before ':', or > move the ':' onto the next line, indented 4 spaces from 'ResultInfo()' with a > space between ':' and result_type. Done.
Peter, Bart and Mike have given this change an extensive review (as you can see from the code review history). It's also well tested. It should be easy for you to review in whatever depth you want. Regardless of the depth of review you want to give it, you should at least be aware of what it's doing--read the first few paragraphs of the changelist description. I'd like to hear a comment (if only "this sounds plausible" or "hell no" (though I hope it's not the latter :>)) before I submit it. thanks, mark
LGTM https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to I'm not a huge fan of the "perhaps" comments here and in the keyword provider. It's not clear to a reader whether they should or shouldn't be identical, how to tell, if some action will or should be taken, etc. I feel like, make a judgement call, then if necessary explain why you did it the way you did and/or how to tell (or what to do) if problems arise. https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1127: return type == AutocompleteInput::QUERY ? 1450 : 1100; Nit: I'd put parens around binary subexpression for clarity
Peter, Thanks for the fast review! --mark https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to On 2013/02/05 23:17:48, Peter Kasting wrote: > I'm not a huge fan of the "perhaps" comments here and in the keyword provider. > It's not clear to a reader whether they should or shouldn't be identical, how to > tell, if some action will or should be taken, etc. > > I feel like, make a judgement call, then if necessary explain why you did it the > way you did and/or how to tell (or what to do) if problems arise. I don't like these comments either. I think they add more confusion than they remove. Originally these functions were both uncommented; I added the comments about how they're pretty similar but don't really have to be upon Mike's request. I can remove the comments if you prefer. https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1127: return type == AutocompleteInput::QUERY ? 1450 : 1100; On 2013/02/05 23:17:48, Peter Kasting wrote: > Nit: I'd put parens around binary subexpression for clarity Done.
mpcomplete@, As the OWNER of chrome/browser/extensions/api/omnibox/omnibox_apitest.cc can you please review and approve the changes to that file? It should be easy. thanks, mark
lgtm
https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to On 2013/02/06 01:31:59, Mark P wrote: > On 2013/02/05 23:17:48, Peter Kasting wrote: > > I'm not a huge fan of the "perhaps" comments here and in the keyword provider. > > > It's not clear to a reader whether they should or shouldn't be identical, how > to > > tell, if some action will or should be taken, etc. > > > > I feel like, make a judgement call, then if necessary explain why you did it > the > > way you did and/or how to tell (or what to do) if problems arise. > > I don't like these comments either. I think they add more confusion than they > remove. Originally these functions were both uncommented; I added the comments > about how they're pretty similar but don't really have to be upon Mike's > request. I can remove the comments if you prefer. My hope was either: * Make the functions match and write a comment as to why they need to be kept in sync * Leave the functions not matching and write a comment as to why you think not matching is correct, or how to tell if you're wrong It's OK to make the wrong judgment call. I'd just rather, if there is potential confusion here, someone _made_ a judgment call and documented it. The comments as-is and the functions uncommented are about the same in terms of future reader confusion.
https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to On 2013/02/06 02:01:41, Peter Kasting wrote: > On 2013/02/06 01:31:59, Mark P wrote: > > On 2013/02/05 23:17:48, Peter Kasting wrote: > > > I'm not a huge fan of the "perhaps" comments here and in the keyword > provider. > > > > > It's not clear to a reader whether they should or shouldn't be identical, > how > > to > > > tell, if some action will or should be taken, etc. > > > > > > I feel like, make a judgement call, then if necessary explain why you did it > > the > > > way you did and/or how to tell (or what to do) if problems arise. > > > > I don't like these comments either. I think they add more confusion than they > > remove. Originally these functions were both uncommented; I added the > comments > > about how they're pretty similar but don't really have to be upon Mike's > > request. I can remove the comments if you prefer. > > My hope was either: > > * Make the functions match and write a comment as to why they need to be kept in > sync > * Leave the functions not matching and write a comment as to why you think not > matching is correct, or how to tell if you're wrong > > It's OK to make the wrong judgment call. I'd just rather, if there is potential > confusion here, someone _made_ a judgment call and documented it. The comments > as-is and the functions uncommented are about the same in terms of future reader > confusion. My judgment call is that they don't need to match. I can add the following comment (if you prefer it to no comment at all and prefer it to the wishy-washy perhaps comment): >>> This function bears some resemblance to KeywordProvider::CalculateRelevance() but there is no need to keep them looking like each other. They're independent. Reminder: if you change this function, you should evaluate the change as with any other omnibox ranking change. >>> BTW, it's hard to explain why they don't need to match. That's like asking me to comment in the SearchProvider code why this relevance function doesn't look anything like the relevance function in the HistoryURL code. They're independent. What do you think of the proposed comment?
https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to Honestly, looking at these, it sure looks to me like they're supposed to match. The code here looks like what you get if you take the keyword provider code and substitute in the conditions that must be true here -- although I'm not sure about allow_exact_keyword_match; maybe we should be checking that here? Do we risk triggering keyword mode when we shouldn't, with this patch?
https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to On 2013/02/06 18:47:34, Peter Kasting wrote: > Honestly, looking at these, it sure looks to me like they're supposed to match. > The code here looks like what you get if you take the keyword provider code and > substitute in the conditions that must be true here You are correct; that's how I made the function. (This change is supposed to be user-invisible, so I had to keep the scoring functions the same.) But there is no reason they cannot diverge in the future. There's no reason we need to score verbatim queries for regular keyword search engines the same as verbatim queries for extension-based keyword search engines. I can easily imagine wanting to make one more important than the other. > -- although I'm not sure > about allow_exact_keyword_match; maybe we should be checking that here? We don't need to check it here. We wouldn't have a keyword provider is we don't allow_exact_keyword match. This is enforced here: http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/autocomplete... > Do we > risk triggering keyword mode when we shouldn't, with this patch? I don't believe so.
https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to On 2013/02/06 18:54:11, Mark P wrote: > On 2013/02/06 18:47:34, Peter Kasting wrote: > > Honestly, looking at these, it sure looks to me like they're supposed to > match. > > The code here looks like what you get if you take the keyword provider code > and > > substitute in the conditions that must be true here > > You are correct; that's how I made the function. (This change is supposed to be > user-invisible, so I had to keep the scoring functions the same.) But there is > no reason they cannot diverge in the future. There's no reason we need to score > verbatim queries for regular keyword search engines the same as verbatim queries > for extension-based keyword search engines. I can easily imagine wanting to > make one more important than the other. That seems like the right comment, then: // This function is responsible for scoring verbatim matches for non-extension keywords. KeywordProvider::CalculateRelevance() scores verbatim matches for extension keywords, as well as non-verbatim keyword matches. These are currently in sync, but there's no reason we couldn't decide in the future to score verbatim matches differently for extension and non-extension keywords. If you make such a change, however, you should update this comment to describe it, so it's clear why the functions diverge.
I'm glad we got a comment we're all happy with. I'll try submitting this later this afternoon. --mark https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/12090006/diff/51002/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1118: // Perhaps this should be kept similar to On 2013/02/06 19:00:09, Peter Kasting wrote: > On 2013/02/06 18:54:11, Mark P wrote: > > On 2013/02/06 18:47:34, Peter Kasting wrote: > > > Honestly, looking at these, it sure looks to me like they're supposed to > > match. > > > The code here looks like what you get if you take the keyword provider code > > and > > > substitute in the conditions that must be true here > > > > You are correct; that's how I made the function. (This change is supposed to > be > > user-invisible, so I had to keep the scoring functions the same.) But there > is > > no reason they cannot diverge in the future. There's no reason we need to > score > > verbatim queries for regular keyword search engines the same as verbatim > queries > > for extension-based keyword search engines. I can easily imagine wanting to > > make one more important than the other. > > That seems like the right comment, then: > > // This function is responsible for scoring verbatim matches for non-extension > keywords. KeywordProvider::CalculateRelevance() scores verbatim matches for > extension keywords, as well as non-verbatim keyword matches. These are > currently in sync, but there's no reason we couldn't decide in the future to > score verbatim matches differently for extension and non-extension keywords. If > you make such a change, however, you should update this comment to describe it, > so it's clear why the functions diverge. That comment sounds good to me. I copied and pasted it with a minor edit for clarity (in my opinion). Also put an analogous comment in keyword_provider.cc.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/12090006/46002
Failed to trigger a try job on android_clang_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/12090006/51012
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/12090006/57003
Message was sent while issue was closed.
Change committed as 181181 |
