Omnibox - Boost Frequency Scores Based on Number of Matching Pages

components/omnibox/browser/scored_history_match.h

Index: components/omnibox/browser/scored_history_match.h
diff --git a/components/omnibox/browser/scored_history_match.h b/components/omnibox/browser/scored_history_match.h
index dacd31cec537b30c501c35a6410bca55cbe20d96..821311a33afc358ecc90443e6d1db30ddbb9e66d 100644
--- a/components/omnibox/browser/scored_history_match.h
+++ b/components/omnibox/browser/scored_history_match.h
@@ -16,6 +16,7 @@
 #include "components/history/core/browser/history_match.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/omnibox/browser/in_memory_url_index_types.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 
 class ScoredHistoryMatchTest;
 
@@ -43,9 +44,11 @@ struct ScoredHistoryMatch : public history::HistoryMatch {
   // These offsets (".net" should have an offset of 1) come from
   // |terms_to_word_starts_offsets|. |is_url_bookmarked| indicates whether the
   // match's URL is referenced by any bookmarks, which can also affect the raw
-  // score.  The raw score allows the matches to be ordered and can be used to
-  // influence the final score calculated by the client of this index.  If the
-  // row does not qualify the raw score will be 0.
+  // score.  |num_matching_pages| indicates how many URLs in the eligible URL
+  // database match the user's input; it can also affect the raw score.  The raw
+  // score allows the matches to be ordered and can be used to influence the
+  // final score calculated by the client of this index.  If the row does not
+  // qualify the raw score will be 0.
   ScoredHistoryMatch(const history::URLRow& row,
                      const VisitInfoVector& visits,
                      const base::string16& lower_string,
@@ -53,6 +56,7 @@ struct ScoredHistoryMatch : public history::HistoryMatch {
                      const WordStarts& terms_to_word_starts_offsets,
                      const RowWordStarts& word_starts,
                      bool is_url_bookmarked,
+                     size_t num_matching_pages,

[Peter Kasting, 2016/12/01 07:07:53]

Nit: There are a lot of args here and the unittests pass "1" a lot.  Do default
args, or alternate constructors that take fewer args, or just a helper in the
unittest to construct these, make sense?

[Mark P, 2016/12/04 01:06:42]

Almost all of these args are requires and need reasonable values consistent with
other values.  The only exceptions are |is_url_bookmarked| and
|num_matching_pages|.  Aside from the unittest, we only construct these classes
in one place.  I don't think having an alternate constructor or default args
will buy us anything.

I can imagine doing something in the unit tests, though I appreciate unittests
being easily readable and clear about how parameters are set.  In other words,
I'd like to use the full constructor, not a helper function, in the unit tests,
especially because it only lets us get rid of two args.  The constructors will
remain long; the extra level of indirection isn't worth it in my opinion.

                      base::Time now);
 
   ~ScoredHistoryMatch();
@@ -123,10 +127,12 @@ struct ScoredHistoryMatch : public history::HistoryMatch {
   // better) based the rate of visits, whether the page is bookmarked, and
   // how often those visits are typed navigations (i.e., explicitly
   // invoked by the user).  |now| is passed in to avoid unnecessarily
-  // recomputing it frequently.
+  // recomputing it frequently.  |num_matching_pages| is used to alter the
+  // frequency score depending on how many pages matched the user's input.
   float GetFrequency(const base::Time& now,
                      const bool bookmarked,
-                     const VisitInfoVector& visits) const;
+                     const VisitInfoVector& visits,
+                     const size_t num_matching_pages) const;
 
   // Combines the two component scores into a final score that's
   // an appropriate value to use as a relevancy score. Scoring buckets are
@@ -171,6 +177,12 @@ struct ScoredHistoryMatch : public history::HistoryMatch {
   // sum of visit scores instead of the weighted average.
   static bool frequency_uses_sum_;
 
+  // A mapping from the number of matching pages to the multiplier by which
+  // all those matching pages' frequency scores should be multiplied.
+  // Omitted values are assumed to have multipliers of 1.0.

[Peter Kasting, 2016/12/01 07:07:53]

Nit: I would omit this last sentence and possibly refer the user to the
documentation on the type itself (if need be)

[Mark P, 2016/12/04 01:06:42]

Did both.  Now with the restructuring of the data type, I think the reference
"need be".

+  static OmniboxFieldTrial::NumMatchesMultipliers*
+      num_matches_to_frequency_multiplier_;
+
   // The maximum number of recent visits to examine in GetFrequency().
   static size_t max_visits_to_score_;