Add confidence to AutocompleteMatch.

chrome/browser/autocomplete/history_contents_provider.cc

Index: chrome/browser/autocomplete/history_contents_provider.cc
diff --git a/chrome/browser/autocomplete/history_contents_provider.cc b/chrome/browser/autocomplete/history_contents_provider.cc
index 61749a2841155cb1e5615ab0b1762d8d4bdb6df4..ab779fd380927f9936b71afb360e7d8a76d7b4f9 100644
--- a/chrome/browser/autocomplete/history_contents_provider.cc
+++ b/chrome/browser/autocomplete/history_contents_provider.cc
@@ -24,19 +24,27 @@ namespace {
 // time it will take.
 const int kDaysToSearch = 30;
 
+}  // end namespace
+
 // When processing the results from the history query, this structure points to
 // a single result. It allows the results to be sorted and processed without
 // modifying the larger and slower results structure.
 struct MatchReference {
-  MatchReference(const history::URLResult* result, int relevance)
+  MatchReference(const history::URLResult* result,
+                 int relevance,
+                 float confidence)
       : result(result),
-        relevance(relevance) {
+        relevance(relevance),
+        confidence(confidence) {
   }
 
   const history::URLResult* result;
   int relevance;  // Score of relevance computed by CalculateRelevance.
+  float confidence;  // Confidence computed by CalculateConfidence.
 };
 
+namespace {

[Peter Kasting, 2011/08/09 20:53:00]

Nit: Don't split the anonymous namespace into two pieces.

I think CompareMatchRelevance can be a member of MatchReference (maybe declared
with a similar signature to operator>() and called via an adapter?  If not, then
at least as a static two-arg member).

[dominich, 2011/08/09 21:43:43]

Done.

+
 // This is a > operator for MatchReference.
 bool CompareMatchRelevance(const MatchReference& a, const MatchReference& b) {
   if (a.relevance != b.relevance)
@@ -46,7 +54,7 @@ bool CompareMatchRelevance(const MatchReference& a, const MatchReference& b) {
   return a.result->last_visit() > b.result->last_visit();
 }
 
-}  // namespace
+}  // end namespace
 
 using history::HistoryDatabase;
 
@@ -187,7 +195,8 @@ void HistoryContentsProvider::ConvertResults() {
   for (std::vector<history::URLResult*>::const_reverse_iterator i =
        results_.rbegin(); i != results_.rend(); ++i) {
     history::URLResult* result = *i;
-    MatchReference ref(result, CalculateRelevance(*result));
+    MatchReference ref(result, CalculateRelevance(*result),
+                       CalculateConfidence(*result, results_));
     result_refs.push_back(ref);
   }
 
@@ -197,8 +206,10 @@ void HistoryContentsProvider::ConvertResults() {
                     result_refs.end(), &CompareMatchRelevance);
   matches_.clear();
   for (size_t i = 0; i < max_for_provider; i++) {
-    matches_.push_back(ResultToMatch(*result_refs[i].result,
-                                     result_refs[i].relevance));
+    AutocompleteMatch match = ResultToMatch(result_refs[i]);

[Peter Kasting, 2011/08/09 20:53:00]

Nit: I prefer constructor form to assignment form when referring to heavyweight
classes (I know, the compiler optimizes one to the other)

[dominich, 2011/08/09 21:43:43]

Done.

+    UMA_HISTOGRAM_COUNTS_100("Autocomplete.Confidence_HistoryContents",
+                             match.confidence * 100);
+    matches_.push_back(match);
   }
 }
 
@@ -208,10 +219,13 @@ bool HistoryContentsProvider::MatchInTitle(const history::URLResult& result) {
 }
 
 AutocompleteMatch HistoryContentsProvider::ResultToMatch(
-    const history::URLResult& result,
-    int score) {
-  AutocompleteMatch match(this, score, true, MatchInTitle(result) ?
-      AutocompleteMatch::HISTORY_TITLE : AutocompleteMatch::HISTORY_BODY);
+    const MatchReference& match_reference) {
+  const history::URLResult& result = *match_reference.result;
+  AutocompleteMatch match(this, match_reference.relevance,

[Peter Kasting, 2011/08/09 20:53:00]

Nit: This wrapping would be OK:

  AutocompleteMatch match(this, match_reference.relevance,
      match_reference.confidence, true, MatchInTitle(result) ?
      AutocompleteMatch::HISTORY_TITLE : AutocompleteMatch::HISTORY_BODY);

[dominich, 2011/08/09 21:43:43]

Done.

+                          match_reference.confidence, true,
+                          MatchInTitle(result) ?
+                              AutocompleteMatch::HISTORY_TITLE :
+                              AutocompleteMatch::HISTORY_BODY);
   match.contents = StringForURLDisplay(result.url(), true, trim_http_);
   match.fill_into_edit =
       AutocompleteInput::FormattedStringWithEquivalentMeaning(result.url(),
@@ -263,6 +277,30 @@ int HistoryContentsProvider::CalculateRelevance(
       (1000 + star_title_count_++) : (550 + star_contents_count_++);
 }
 
+float HistoryContentsProvider::CalculateConfidence(
+    const history::URLResult& result,
+    const history::QueryResults& results) const {
+  // Calculate a score of [0, 0.5] based on visit count.
+  // TODO(dominic): Include typed count?

[Peter Kasting, 2011/08/09 20:53:00]

If you're only going to use one, prefer typed_count to visit_count, for two
reasons:

* It's a better indicator of what the user wants to open given that he is typing
in the address bar (users tend to open certain URLs by typing and others by e.g.
bookmarks, so visit_count is a good indicator of overall interest but a bad one
for specifically omnibox interest)

* Since the DB query is sorted by typed_count, the results you get back may be
effectively a random selection as far as visit_counts are concerned (meaning
many high-visit_count-URLs may be present in one query and absent in a similar
one), leading to wild swings in confidence for the same result across distinct
queries

[dominich, 2011/08/09 21:43:43]

Done.

+  float numerator = result.visit_count();
+  float denominator = 0.0f;
+  for (std::vector<history::URLResult*>::const_iterator it = results.begin();
+       it != results.end(); ++it) {
+    denominator += (*it)->visit_count();
+  }
+  // It should only be equal to 0 if the result is not in the results vector.
+  DCHECK(denominator > 0);
+  float score = 0.5f * (numerator / denominator);
+
+  // Add 0.5 if the URL is bookmarked to get a final range of [0, 1].

[Peter Kasting, 2011/08/09 20:53:00]

I think this gives far too much weight to bookmarking.

For example, users who don't use bookmarks at all will never get higher than a
50% confidence rating on a URL.  This seems obviously wrong.

[dominich, 2011/08/09 21:43:43]

Done.

+  if (profile_->GetBookmarkModel() &&
+      profile_->GetBookmarkModel()->IsBookmarked(result.url())) {
+    score += 0.5f;
+  }
+
+  return score;
+}
+
 void HistoryContentsProvider::QueryBookmarks(const AutocompleteInput& input) {
   BookmarkModel* bookmark_model = profile_->GetBookmarkModel();
   if (!bookmark_model)