Omnibox: Don't Let Users Escape Keyword Mode Accidentally

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 2e79b08f3841cd8c1562bf669ea5ce13ea10a12b..c484c763afa111c0f2c70e81bd1aafc01b6f292e 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -1208,8 +1208,9 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                   std::string(),
                   &map);
   }
+  const TemplateURL* keyword_url = NULL;
   if (!keyword_input_.text().empty()) {
-    const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
+    keyword_url = providers_.GetKeywordProviderURL();
     // We only create the verbatim search query match for a keyword
     // if it's not an extension keyword.  Extension keywords are handled
     // in KeywordProvider::Start().  (Extensions are complicated...)
@@ -1293,6 +1294,17 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
 
     matches_.push_back(*i);
   }
+  if (keyword_url != NULL) {
+    // When in keyword mode, make sure all matches are not associated with the

[Peter Kasting, 2013/11/15 03:03:34]

Nit: are -> that are

[Mark P, 2013/11/15 17:36:33]

Done.

+    // keyword are not allowed to be the default match lest they cause the user
+    // to break out of keyword mode.  For reference, non-keyword matches are
+    // queries to the default search engine and navsuggestions from any source.
+    for (ACMatches::iterator it = matches_.begin(); it != matches_.end();

[Peter Kasting, 2013/11/15 03:03:34]

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?

[Mark P, 2013/11/15 17:36:33]

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.
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

+         ++it) {
+      if (it->keyword != keyword_url->keyword())
+        it->allowed_to_be_default_match = false;

[Peter Kasting, 2013/11/15 03:03:34]

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).

[Mark P, 2013/11/15 17:36:33]

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.

+    }
+  }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.ConvertResultsTime",
                       base::TimeTicks::Now() - start_time);
 }