Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/autocomplete_result.cc

Index: chrome/browser/autocomplete/autocomplete_result.cc
diff --git a/chrome/browser/autocomplete/autocomplete_result.cc b/chrome/browser/autocomplete/autocomplete_result.cc
index 58a19c98b6bcbb4720141f66a1f0a0242b9b2706..111de79b4b7ae86e11da3d639910b2dc57d59e90 100644
--- a/chrome/browser/autocomplete/autocomplete_result.cc
+++ b/chrome/browser/autocomplete/autocomplete_result.cc
@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
+#include "chrome/browser/omnibox/omnibox_field_trial.h"
 
 // static
 const size_t AutocompleteResult::kMaxMatches = 6;
@@ -21,7 +22,9 @@ void AutocompleteResult::Selection::Clear() {
   is_history_what_you_typed_match = false;
 }
 
-AutocompleteResult::AutocompleteResult() {
+AutocompleteResult::AutocompleteResult()
+    : reorder_for_legal_default_match_(
+          OmniboxFieldTrial::InReorderForLegalDefaultMatchGroup()) {
   // Reserve space for the max number of matches we'll show.
   matches_.reserve(kMaxMatches);
 
@@ -99,12 +102,23 @@ void AutocompleteResult::SortAndCull(const AutocompleteInput& input,
                  matches_.end());
 
   // Sort and trim to the most relevant kMaxMatches matches.
-  const size_t num_matches = std::min(kMaxMatches, matches_.size());
-  std::partial_sort(matches_.begin(), matches_.begin() + num_matches,
-                    matches_.end(), &AutocompleteMatch::MoreRelevant);
-  matches_.resize(num_matches);
+  std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::MoreRelevant);
+  if (!empty() && !begin()->allowed_to_be_default_match &&
+      reorder_for_legal_default_match_) {
+    // Top match is not allowed to be the default match.  Find the most

[msw, 2013/07/18 06:23:57]

I find it odd that this simply checks the flag to determine if a match is truly
allowed to be the default, rather than enforcing something, like exact input or
inlinable fill_into_edit. I like the idea of making Autocomplete
Result|Controller enforce the sensibility of the default match. Unfortunately,
this solution falls short of what would hope for, by leaving the validity of a
legal default match up to individual providers. So the logic to determine inline
autocompletion behavior and the capacity of a match to be the default is still
scattered. Also, a simple error in setting the flag elsewhere could yield
undesirable behavior without any safeguard here to check that a default match is
truly allowed. What do you think?

[Peter Kasting, 2013/07/18 17:35:28]

It's basically impossible to enforce this correctly because this code would have
to understand every way we can do fuzzy matching.

For example, if the user inputs "http:///foo.co", we can inline-autocomplete
with a fill_into_edit of "http://foo.com".  It's pretty much impossible at this
point to realize that that removed slash in the middle is "acceptable" for
inlining purposes.

So I don't think we can or should try to do anything smart here.

[msw, 2013/07/18 17:46:53]

I disagree, I suspect the problem is hard, but should be addressed.
URL prefix matching should be able to handle an extra slash.
Can you think of other cases that really would make it impossible?

[Peter Kasting, 2013/07/18 17:51:54]

I don't know what this means.
In general we can do arbitrary transforms and/or spelling corrections.  Jut on
URLs we can certainly do a variety of fixup/escape/unescape changes.

I think it is completely appropriate to trust providers to do the right thing
here.  Even if it were possible to check things at this point, we'd need to do
it by duplicating all the logic in the providers that does this stuff.

It's not the controller's job to try and double-check the providers any more
than it's the edit model's job to double-check the controller (or the location
bar view's job, or whatever).

[msw, 2013/07/18 18:12:11]

We check for and trim schemes and www. prefixes elsewhere, and I suspect we
could do so in a robust enough manner to ignore an extra slash, etc. But your
point below makes this mostly moot.
That's a good point; provider complexities should not be duplicated here. More
straightforward inlining would be worth checking, but presuming that we have a
good deal of substitution that would seem arbitrary to the controller, I suppose
we must trust the providers.

+    // relevance legal match and shift it to the front.
+    for (AutocompleteResult::iterator it = begin() + 1; it != end(); ++it) {
+      if (it->allowed_to_be_default_match) {
+        std::rotate(matches_.begin(), it, it + 1);

[msw, 2013/07/18 06:23:57]

Why rotate instead of just moving the new default match to the top?
Shouldn't this try to be the least disruptive it can be?

[Peter Kasting, 2013/07/18 17:35:28]

This rotate command _does_ just move the default match to the top.

[msw, 2013/07/18 17:46:53]

afaict this will modify the matches preceding |it| as well:
http://www.cplusplus.com/reference/algorithm/rotate/
Looks like {a, b, c*, d} would become {c*, a, b, d}, right?

[Peter Kasting, 2013/07/18 17:51:54]

Yes, which is precisely the definition of "move c to the top".

[msw, 2013/07/18 18:12:11]

Hahaha, *face-palm*. I'll admit at first I was thinking it'd be rotating the
whole list, which would be wrong, but then I saw the |it + 1| end arg for the
rotate and still thought it'd be wrong. But how the hell did I write an example
that shows its correctness and still miss the point? I'll chalk that up to
watching the Kissinger Fireside Chat while reviewing.

+        break;
+      }
+    }
+  }
+  matches_.resize(std::min(kMaxMatches, matches_.size()));
 
   default_match_ = begin();
+  DCHECK((default_match_ == end()) ||
+      default_match_->allowed_to_be_default_match);

[msw, 2013/07/18 06:23:57]

Won't this fail if the user is not in the experiment and the providers were
disobedient?

[Peter Kasting, 2013/07/18 17:35:28]

Yes.

[Mark P, 2013/07/21 20:31:05]

Precisely: I want this to fail if the user is not the experiment and the
providers were disobedient because it will help me figure out if I missed a
condition in which something should be allowed to be the default match but I
didn't mark it as such.  Or it'll let me learn if we have a bug in some provider
where something is getting to be the default match but we don't think it should
be.

 
   // Set the alternate nav URL.
   alternate_nav_url_ = (default_match_ == end()) ?