Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
67693004:
Omnibox: Don't Let Users Escape Keyword Mode Accidentally (Closed)
|
Created:
7 years, 1 month ago by Mark P Modified:
7 years, 1 month ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox: Don't Let Users Escape Keyword Mode Accidentally
The current code already protects against users escaping keyword
mode accidentally in regular (non-reorder mode). This change
simply adds the proper annotations so reorder mode has the
protection as well.
There are three parts to this change:
* Mark navsuggestions as "not allowed to be default match" if the
user is in keyword mode. Revise tests to reflect this.
* Add a constraint that, in keyword mode, there must be at least
one keyword (query) suggestion that be the default match. Restore
the keyword verbatim if the constraint is violated. Revise tests to
reflect this.
* As a post-processing step, if there is at least one keyword mode
match that can be default [1], mark all matches from other sources
(e.g., the default search engine) as not allowed to be the default
match. Revise tests to reflect this.
[1] Given the second bullet (the constraint), you might wonder why
it could be the case that there might not be a keyword mode match
that can be default. The reason for this criteria is that the
user could be in keyword mode for a keyword extension or some
such thing, but we can't trust KeywordProvider to always make a
default match in those cases. For instance, the extension could be
disabled. Hence, we maintain the invariant that SearchProvider always
returns a match that can be default, even if needs to back off to
the default provider match to do so.
By the way, the regular mode protection works as follows:
(i) demote all navsuggestions past the score of the top query
when in keyword mode
(ii) abandon suggested relevance scores on the default provider,
meaning the top result from the default provider will score 250
when in keyword mode
(iii) enforce a constraint that a match (thus implicitly a keyword
mode match) must score at least 850
Together, these constraints (and the way it fixes the violations)
guarantee that a query from the keyword provider comes first in regular
(non-reorder mode).
BUG=317946, 318543
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236777
Patch Set 1 #
Total comments: 7
Patch Set 2 : tweaks #Patch Set 3 : new approach #
Total comments: 1
Patch Set 4 : fix incorrect comment #
Total comments: 12
Patch Set 5 : Peter's latest comments #
Total comments: 24
Patch Set 6 : Mike's comments #
Total comments: 4
Patch Set 7 : namespace #
Messages
Total messages: 22 (0 generated)
Hi Peter, This might be my last change to SearchProvider for reorder mode. Please take a look. Mike, you can look if you want, but it's probably not necessary. --mark https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider_unittest.cc (left): https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider_unittest.cc:2222: { "[\"a\",[\"b\"],[],[],{\"google:suggestrelevance\":[9999]," I moved this case below, to the "on the other hand section".
https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:1298: // When in keyword mode, make sure all matches are not associated with the Nit: are -> that are https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:1302: for (ACMatches::iterator it = matches_.begin(); it != matches_.end(); Is this truly the best place for this code? Perhaps we should be setting this individually on the matches in some other function that knows whether the match is from the keyword provider. It seems like, if a match isn't from the keyword provider, and we _have_ a keyword provider to begin with, then the match shouldn't be default? The only way I'd see that being wrong is if we could have a keyword provider when the default match was still supposed to be from the default provider. I dunno if this can happen... maybe if we type a keyword name, but no space yet, and the keyword in question requires a substitution parameter? https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:1305: it->allowed_to_be_default_match = false; Thought experiment. Let's say the user types an extension keyword. This won't trigger a verbatim match from the SearchProvider (see comment at line 1214 above). Now what if the keyword provider doesn't provide a verbatim match either? Then we're going to DCHECK eventually because we'll have no default. So what could make the keyword provider not provide a verbatim match? I think there is one possibility. If the keyword is from an extension disabled in incognito, and we're in incognito, then I _think_ SearchProvider::Start()'s call to KeywordProvider::GetSubstitutingTemplateURLForInput() gets the relevant TemplateURL even though it's disabled. But KeywordProvider::Start() will (correctly) prune these matches. If I'm right, then this is a bug in KeywordProvider; GetSubstitutingTemplateURLForInput() needs to check the incognito state like Start() and GetKeywordForText() already do (and/or this code should be rewritten to define one function in terms of another, allowing sharing the check by placing it in the innermost function).
Lots of things to think about here... --mark https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:1298: // When in keyword mode, make sure all matches are not associated with the On 2013/11/15 03:03:34, Peter Kasting wrote: > Nit: are -> that are Done. https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:1302: for (ACMatches::iterator it = matches_.begin(); it != matches_.end(); On 2013/11/15 03:03:34, Peter Kasting wrote: > Is this truly the best place for this code? > Perhaps we should be setting this > individually on the matches in some other function that knows whether the match > is from the keyword provider. It seems like, if a match isn't from the keyword > provider, and we _have_ a keyword provider to begin with, then the match > shouldn't be default? Yes, I think this is the right place. As you say, you can do it earlier (because we know the keyword provider status earlier). I think this code is best done here as a post-processing step, at least after we have the list of candidate matches. If we wanted to do this when matches are created, we would need the same logic in two places: the place that creates query AutocompleteMatches and the place that creates navigation AutocompleteMatches. I think it's better not to duplicate the logic. This function is the first place that those two matches are joined into one list. An additional, less significant reason: if we put the logic in at the time of creation, we would need to pass yet another argument in the already huge (13 parameter!) list of CreateSearchSuggestion. > The only way I'd see that being wrong is if we could have a keyword provider > when the default match was still supposed to be from the default provider. I > dunno if this can happen... > maybe if we type a keyword name, but no space yet, > and the keyword in question requires a substitution parameter? If you type "keyword", it will not be the default match; the search match comes first. If you type "keyword ", at least the default match prompt "Search keyword:" is created by keyword provider and marked as allowed to be default (even though it has no destination URL). So we're not in an entire disaster (and marking these as not default seems corrrect). Somehow pressing enter, though, does a search for the keyword on the default search engine. This is probably related to crbug/313643 more thoughts on these issues in the next comment https://codereview.chromium.org/67693004/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:1305: it->allowed_to_be_default_match = false; On 2013/11/15 03:03:34, Peter Kasting wrote: > Thought experiment. > > Let's say the user types an extension keyword. This won't trigger a verbatim > match from the SearchProvider (see comment at line 1214 above). Now what if the > keyword provider doesn't provide a verbatim match either? Then we're going to > DCHECK eventually because we'll have no default. > > So what could make the keyword provider not provide a verbatim match? I think > there is one possibility. If the keyword is from an extension disabled in > incognito, and we're in incognito, then I _think_ SearchProvider::Start()'s call > to KeywordProvider::GetSubstitutingTemplateURLForInput() gets the relevant > TemplateURL even though it's disabled. But KeywordProvider::Start() will > (correctly) prune these matches. > > If I'm right, then this is a bug in KeywordProvider; > GetSubstitutingTemplateURLForInput() needs to check the incognito state like > Start() and GetKeywordForText() already do (and/or this code should be rewritten > to define one function in terms of another, allowing sharing the check by > placing it in the innermost function). What a dang tricky thought experiment. I'm going to think through this verbally. You're right in the current situation that an extension keyword will leave SearchProvider stuck without a valid default match. That seems okay if we have faith that KeywordProvider will deliver a valid default match in all those cases. Even if true (which it's probably not due to incognito and the like) at the moment, it's a fragile assumption. What if we simply make this prohibit default match in keyword mode nuanced so as not to apply when its an extension keyword. Will that be enough? I'm inclined to believe no, not really. The things we rely on keyword provider to deliver are - suggestions from extension keyword - keyword suggestions for when there's no remaining text, or only partial text entered - keyword suggestions for non-substituting keywords Making sure the last two situations are handled correctly by the interaction between KeywordProvider and SearchProvider will be hard... This might still be a fragile interaction. We might need additional tests here for other times when we want to allow the default search engine matches to be allowed to be default. Hmmm... CONCLUSION: Here's my proposal after thinking through your complaints. (Thank you for raising them! I entirely forgot about extensions...) Take the current SearchProvider, as a final step look to see if there is a keyword provider match. If so, mark all other matches as not allowed to be the default match. If not, then rollback suggested relevance scores and try again. If there is a keyword provider match now, then mark all non-keyword matches as not allowed. Otherwise, leave everything marked as they original are. I'm not sure how your last paragraph and a half connects to this change. If you think the incognito behavior in SearchProvider is a bug (I am not convinced), then I think it's a bug before this change too... We can discuss that elsewhere.
On 2013/11/15 17:36:33, Mark P wrote: > CONCLUSION: Here's my proposal after thinking through your complaints. (Thank > you for raising them! I entirely forgot about extensions...) Take the current > SearchProvider, as a final step look to see if there is a keyword provider > match. If so, mark all other matches as not allowed to be the default match. > If not, then rollback suggested relevance scores and try again. If there is a > keyword provider match now, then mark all non-keyword matches as not allowed. > Otherwise, leave everything marked as they original are. I'm confused. How does the rolling back suggested scores affect whether there would be a keyword provider match to begin with? > I'm not sure how your last paragraph and a half connects to this change. If you > think the incognito behavior in SearchProvider (You mean KeywordProvider, I think; at least _I_ did) > is a bug (I am not convinced), > then I think it's a bug before this change too... We can discuss that > elsewhere. Yes, it's a bug before this change. I'm just concerned that when coupled with this change it would lead to a DCHECK.
On Fri, Nov 15, 2013 at 2:22 PM, <pkasting@chromium.org> wrote: > On 2013/11/15 17:36:33, Mark P wrote: > >> CONCLUSION: Here's my proposal after thinking through your complaints. >> (Thank >> you for raising them! I entirely forgot about extensions...) Take the >> > current > >> SearchProvider, as a final step look to see if there is a keyword provider >> match. If so, mark all other matches as not allowed to be the default >> match. >> If not, then rollback suggested relevance scores and try again. If there >> is a >> keyword provider match now, then mark all non-keyword matches as not >> allowed. >> Otherwise, leave everything marked as they original are. >> > > I'm confused. How does the rolling back suggested scores affect whether > there > would be a keyword provider match to begin with? The keyword engine could say "suppress verbatim" plus not offer any suggestions of its own. In that case, we'd want to abandon what it said and off the keyword verbatim. (Obviously, the server isn't supposed to send something like this, but it can't hurt to be safe.) > > > I'm not sure how your last paragraph and a half connects to this change. >> If >> > you > >> think the incognito behavior in SearchProvider >> > > (You mean KeywordProvider, I think; at least _I_ did) > > > is a bug (I am not convinced), >> then I think it's a bug before this change too... We can discuss that >> elsewhere. >> > > Yes, it's a bug before this change. I'm just concerned that when coupled > with > this change it would lead to a DCHECK. > > https://codereview.chromium.org/67693004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/11/15 22:35:12, Mark P wrote: > > I'm confused. How does the rolling back suggested scores affect whether > > there > > would be a keyword provider match to begin with? > > The keyword engine could say "suppress verbatim" plus not offer any > suggestions of its own. In that case, we'd want to abandon what it said > and off the keyword verbatim. (Obviously, the server isn't supposed to > send something like this, but it can't hurt to be safe.) Ah, didn't think of that. Offhand, I'd wonder if the same code which currently prevents the default search provider from doing this with non-keyword-mode inputs couldn't be made to perform the same checks on the keyword suggestions. Maybe not?
On Fri, Nov 15, 2013 at 2:38 PM, <pkasting@chromium.org> wrote: > On 2013/11/15 22:35:12, Mark P wrote: > >> > I'm confused. How does the rolling back suggested scores affect whether >> > there >> > would be a keyword provider match to begin with? >> > > The keyword engine could say "suppress verbatim" plus not offer any >> suggestions of its own. In that case, we'd want to abandon what it said >> and off the keyword verbatim. (Obviously, the server isn't supposed to >> send something like this, but it can't hurt to be safe.) >> > > Ah, didn't think of that. > > Offhand, I'd wonder if the same code which currently prevents the default > search > provider from doing this with non-keyword-mode inputs couldn't be made to > perform the same checks on the keyword suggestions. Maybe not? > It does this already. We just have to make sure that this happens in the correct order compared with deciding we need to let default search engine matches be allowed to be default matches. That why I proposed the two step thing in the CONCLUSION above. > https://codereview.chromium.org/67693004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, SGTM I guess. This would also avoid having the bug I mentioned above trigger a DCHECK, because rather than checking about being in "keyword mode" we'd specifically be looking at whether we have a keyword match. We should still fix that other bug. I'm not sure offhand exactly what the user-facing consequences of it are today.
Hi Peter, I implemented the new approach that we discussed. Please tell me what you think. --mark https://codereview.chromium.org/67693004/diff/170001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/170001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1472: // If there is a keyword match that is allowed to be the default match, I can move this block into a subroutine if you approve of the whole approach in this changelist.
FYI, rewrote changelist description.
I confess, for some reason my brain is refusing to wrap itself around this change tonight. I feel a nagging sense that I'm not doing a good job really reviewing. https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: return true; It seems kinda wrong to do this here, because it effectively makes the function lie. Why shouldn't the caller check this variable and not call the function to begin with instead? https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1408: // the default match. This comment doesn't seem to match the code? It looks like we make the correction in reorder mode too (and check for it below in both modes). https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1477: (keyword_match_it->keyword == keyword_url->keyword())) { Nit: You know, if we had a functor that could compare match keywords against a particular keyword, we could make this block be simpler by way of things like std::find_if(). https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1891: // keyword provider active lest it appear first and breaks the user Nit: breaks -> break Also probably add a comma after "active" https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:2249: // the keyword verbatim. Nit: "...score"? https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:2499: // we restore the keyword verbatim match. Nit: match -> score?
Because you're nervous, I'm going to ask Mike to review this as well. --mark https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1324: return true; On 2013/11/19 02:58:24, Peter Kasting wrote: > It seems kinda wrong to do this here, because it effectively makes the function > lie. Why shouldn't the caller check this variable and not call the function to > begin with instead? Good point. It does seem wrong. Fixed. https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1408: // the default match. On 2013/11/19 02:58:24, Peter Kasting wrote: > This comment doesn't seem to match the code? It looks like we make the > correction in reorder mode too (and check for it below in both modes). Clarified comment and added DCHECK. https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1477: (keyword_match_it->keyword == keyword_url->keyword())) { On 2013/11/19 02:58:24, Peter Kasting wrote: > Nit: You know, if we had a functor that could compare match keywords against a > particular keyword, we could make this block be simpler by way of things like > std::find_if(). I realized part of this block is redundant with HasKeywordMatchThatCanBeDefault() so I simplified it. Now the find_if doesn't seem necessary here. I thought about adding a functor for HasKeywordMatchThatCanBeDefault() but it seemed heavy-handed--it would only get used once, wouldn't save any net lines of code, and I'm not sure the net result will be more readable. https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1891: // keyword provider active lest it appear first and breaks the user On 2013/11/19 02:58:24, Peter Kasting wrote: > Nit: breaks -> break > > Also probably add a comma after "active" Agree with both. Done. https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:2249: // the keyword verbatim. On 2013/11/19 02:58:24, Peter Kasting wrote: > Nit: "...score"? Not necessary, I think, but added word "score" anyway. https://codereview.chromium.org/67693004/diff/230001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:2499: // we restore the keyword verbatim match. On 2013/11/19 02:58:24, Peter Kasting wrote: > Nit: match -> score? Done.
Hi Mike, You're the expert on SearchProvider constraints. Can you please review this? Peter's not entirely comfortable thinking about the constraint code. Another set of eyes would make him feel more comfortable. (Personally I'm comfortable with the code. Nonetheless, another set of eyes couldn't hurt.) thanks, mark
This seems mostly good; my comments are mostly minor. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1310: bool SearchProvider::IsTopMatchNavigation( Keep this as IsTopMatchNavigationInKeywordMode. I don't see the benefit of changing the function from something that checks a constraint violation to something that seems innocuous. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1318: bool SearchProvider::HasKeywordMatchThatCanBeDefault() const { Rename this to HasValidDefaultMatchInKeywordMode to parallel HasValidDefaultMatch. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1320: if (keyword_url == NULL) // not in keyword mode -> say everything's okay nit: Please make this comment more formal. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1382: // True if the omnibox will reorder matches as necessary to make the first nit: you don't check this outside the if block; leave it as it was. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1406: DCHECK(!IsTopMatchNavigation( nit: remove this DCHECK; it's redundant with the one at line 1452. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1409: if ((keyword_url != NULL) && !HasKeywordMatchThatCanBeDefault()) { Like IsTopMatchNavigation[InKeywordMode], I'd prefer this were a self-contained constraint check, rather than checking for keyword mode outside the function. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1464: // then prohibit default provider matches from being the defaul match lest nit: defaul->default https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:272: diagnostic_details; nit: this fits on the line above. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:1627: // Find the first match that's allowed to be the default match and check I know you only added this code in two places, but perhaps this deserves a helper function to parallel SearchProvider::FindTopMatch. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:1743: { "a.com", false, false }, This is the only behavior change that seems questionable. If the user types "k a" and the keyword provider 'k' says "go to a.com! that's my top result!", then we'll stomp the suggestion because it'll kick the user out of keyword mode... I think the right right solution is to somehow allow navigation suggestions from the keyword provider to be default matches without kicking the user out of keyword mode, though that seems impossible given the current architecture. So, while I think you're making the right change for now, we should keep the 'right' solution in mind, and perhaps file a bug in case we ever have the opportunity to allow keyword navigation matches to be default... https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:2248: // On the other hand, if there is no inlineable match, restore Nice, I think this is a good behavior change!
I don't know if I agree with Mike's desires to fold the "in keyword mode" stuff back into the oracles in question, but I don't really have a strong opinion. You two can decide. Given his review, LGTM otherwise.
Mike, please take another look. thanks, mark https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1310: bool SearchProvider::IsTopMatchNavigation( On 2013/11/20 21:05:03, msw wrote: > Keep this as IsTopMatchNavigationInKeywordMode. I don't see the benefit of > changing the function from something that checks a constraint violation to > something that seems innocuous. Okay. I don't feel strongly. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1318: bool SearchProvider::HasKeywordMatchThatCanBeDefault() const { On 2013/11/20 21:05:03, msw wrote: > Rename this to HasValidDefaultMatchInKeywordMode to parallel > HasValidDefaultMatch. Revised it to something similar to your suggested name. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1320: if (keyword_url == NULL) // not in keyword mode -> say everything's okay On 2013/11/20 21:05:03, msw wrote: > nit: Please make this comment more formal. Done. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1382: // True if the omnibox will reorder matches as necessary to make the first On 2013/11/20 21:05:03, msw wrote: > nit: you don't check this outside the if block; leave it as it was. Good point. That code movement comes from an earlier iteration of this change. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1406: DCHECK(!IsTopMatchNavigation( On 2013/11/20 21:05:03, msw wrote: > nit: remove this DCHECK; it's redundant with the one at line 1452. I'd prefer to keep it to verify that DemoteKeywordNavigationMatchesPastTopQuery() and that the constraint wasn't actually saved by one of the other constraint checks/repairs. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1409: if ((keyword_url != NULL) && !HasKeywordMatchThatCanBeDefault()) { On 2013/11/20 21:05:03, msw wrote: > Like IsTopMatchNavigation[InKeywordMode], I'd prefer this were a self-contained > constraint check, rather than checking for keyword mode outside the function. Okay. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1464: // then prohibit default provider matches from being the defaul match lest On 2013/11/20 21:05:03, msw wrote: > nit: defaul->default Done. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:272: diagnostic_details; On 2013/11/20 21:05:03, msw wrote: > nit: this fits on the line above. Done. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:1627: // Find the first match that's allowed to be the default match and check On 2013/11/20 21:05:03, msw wrote: > I know you only added this code in two places, but perhaps this deserves a > helper function to parallel SearchProvider::FindTopMatch. Okay, done. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:1743: { "a.com", false, false }, On 2013/11/20 21:05:03, msw wrote: > This is the only behavior change that seems questionable. If the user types "k > a" and the keyword provider 'k' says "go to a.com! that's my top result!", then > we'll stomp the suggestion because it'll kick the user out of keyword mode... I > think the right right solution is to somehow allow navigation suggestions from > the keyword provider to be default matches without kicking the user out of > keyword mode, though that seems impossible given the current architecture. > > So, while I think you're making the right change for now, we should keep the > 'right' solution in mind, and perhaps file a bug in case we ever have the > opportunity to allow keyword navigation matches to be default... Acknowledged. Filed https://code.google.com/p/chromium/issues/detail?id=322156 https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:2248: // On the other hand, if there is no inlineable match, restore On 2013/11/20 21:05:03, msw wrote: > Nice, I think this is a good behavior change! Acknowledged. (Thanks!)
LGTM with a couple new nits. https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1406: DCHECK(!IsTopMatchNavigation( On 2013/11/21 21:30:22, Mark P wrote: > On 2013/11/20 21:05:03, msw wrote: > > nit: remove this DCHECK; it's redundant with the one at line 1452. > > I'd prefer to keep it to verify that > DemoteKeywordNavigationMatchesPastTopQuery() and that the constraint wasn't > actually saved by one of the other constraint checks/repairs. Good call! https://codereview.chromium.org/67693004/diff/390001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/67693004/diff/390001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:123: // Returns the first matches in |matches| with |allowed_to_be_default_match| nit: s/matches/match/ https://codereview.chromium.org/67693004/diff/390001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:125: ACMatches::const_iterator FindDefaultMatch(const ACMatches& matches); nit: This doesn't use any class members, it should be a standalone function in an anonymous namespace in this file.
https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/67693004/diff/310001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1406: DCHECK(!IsTopMatchNavigation( On 2013/11/21 22:11:11, msw wrote: > On 2013/11/21 21:30:22, Mark P wrote: > > On 2013/11/20 21:05:03, msw wrote: > > > nit: remove this DCHECK; it's redundant with the one at line 1452. > > > > I'd prefer to keep it to verify that > > DemoteKeywordNavigationMatchesPastTopQuery() and that the constraint wasn't > > actually saved by one of the other constraint checks/repairs. > > Good call! Acknowledged. https://codereview.chromium.org/67693004/diff/390001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/67693004/diff/390001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:123: // Returns the first matches in |matches| with |allowed_to_be_default_match| On 2013/11/21 22:11:11, msw wrote: > nit: s/matches/match/ Done. https://codereview.chromium.org/67693004/diff/390001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:125: ACMatches::const_iterator FindDefaultMatch(const ACMatches& matches); On 2013/11/21 22:11:11, msw wrote: > nit: This doesn't use any class members, it should be a standalone function in > an anonymous namespace in this file. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/67693004/470001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/67693004/470001
Message was sent while issue was closed.
Change committed as 236777 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
