Omnibox - Boost Frequency Scores Based on Number of Matching Pages

components/omnibox/browser/scored_history_match.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_OMNIBOX_BROWSER_SCORED_HISTORY_MATCH_H_
 #define COMPONENTS_OMNIBOX_BROWSER_SCORED_HISTORY_MATCH_H_
 
 #include <stddef.h>
 
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #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;
 
 // An HistoryMatch that has a score as well as metrics defining where in the
 // history item's URL and/or page title matches have occurred.
 struct ScoredHistoryMatch : public history::HistoryMatch {
   // ScoreMaxRelevance maps from an intermediate-score to the maximum
   // final-relevance score given to a URL for this intermediate score.
   // This is used to store the score ranges of HQP relevance buckets.
   // Please see GetFinalRelevancyScore() for details.
   typedef std::pair<double, int> ScoreMaxRelevance;
 
   // Required for STL, we don't use this directly.
   ScoredHistoryMatch();
   ScoredHistoryMatch(const ScoredHistoryMatch& other);
 
   // Initializes the ScoredHistoryMatch with a raw score calculated for the
   // history item given in |row| with recent visits as indicated in |visits|. It
   // first determines if the row qualifies by seeing if all of the terms in
   // |terms_vector| occur in |row|.  If so, calculates a raw score.  This raw
   // score is in part determined by whether the matches occur at word
   // boundaries, the locations of which are stored in |word_starts|.  For some
   // terms, it's appropriate to look for the word boundary within the term. For
   // instance, the term ".net" should look for a word boundary at the "n".
   // 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,
                      const String16Vector& terms_vector,
                      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();
 
   // Compares two matches by score.  Functor supporting URLIndexPrivateData's
   // HistoryItemsForTerms function.  Looks at particular fields within
   // with url_info to make tie-breaking a bit smarter.
   static bool MatchScoreGreater(const ScoredHistoryMatch& m1,
                                 const ScoredHistoryMatch& m2);
 
@@ skipping 50 matching lines @@
                            const RowWordStarts& word_starts);
 
   // Returns a recency score based on |last_visit_days_ago|, which is
   // how many days ago the page was last visited.
   float GetRecencyScore(int last_visit_days_ago) const;
 
   // Examines the first |max_visits_to_score_| and returns a score (higher is
   // 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
   // specified through |hqp_relevance_buckets|. Please see the function
   // implementation for more details.
   static float GetFinalRelevancyScore(
       float topicality_score,
       float frequency_score,
       const std::vector<ScoreMaxRelevance>& hqp_relevance_buckets);
 
@@ skipping 24 matching lines @@
   static float typed_value_;
 
   // True if we should fix a bug in frequency scoring relating to how we
   // extrapolate frecency when the URL has been visited few times.
   static bool fix_few_visits_bug_;
 
   // Determines whether GetFrequency() returns a score based on on the weighted
   // 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_;
 
   // If true, we allow input terms to match in the TLD (e.g., ".com").
   static bool allow_tld_matches_;
 
   // If true, we allow input terms to match in the scheme (e.g., "http://").
   static bool allow_scheme_matches_;
 
   // The number of title words examined when computing topicality scores.
@@ skipping 13 matching lines @@
   static char hqp_relevance_buckets_str_[];
 
   // |hqp_relevance_buckets_| gives mapping from (topicality*frequency)
   // to the final relevance scoring. Please see GetFinalRelevancyScore()
   // for more details and scoring method.
   static std::vector<ScoreMaxRelevance>* hqp_relevance_buckets_;
 };
 typedef std::vector<ScoredHistoryMatch> ScoredHistoryMatches;
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_SCORED_HISTORY_MATCH_H_