Omnibox - Refactor |relevance_buckets| to Remove Memory Leak on Exit

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"
 
 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.
+  // This is used to store the score ranges of relevance buckets.
   // Please see GetFinalRelevancyScore() for details.
   typedef std::pair<double, int> ScoreMaxRelevance;
 
+  // A sorted vector of ScoreMaxRelevance entries, used by taking a score and
+  // interpolating between consecutive buckets.  See GetFinalRelevancyScore()
+  // for details.
+  typedef std::vector<ScoreMaxRelevance> ScoreMaxRelevances;

[Peter Kasting, 2016/12/10 02:31:15]

Nit: Prefer type alias ("using") to typedef (though if you do this, fix both
here to be consistent)

[Mark P, 2016/12/11 04:57:22]

Done.  (Any reason for this preference?  Or is it arbitrary: pick one for
consistency and use it everywhere.  I thought they were equivalent.)

[Peter Kasting, 2016/12/11 06:00:32]

Very minor differences:

* "using" syntax is more generalizable (with templates), so if you're going to
"pick one", that's the one that will work in more cases
* "using A=B" is more instantly obvious about which type is the "original" and
which is the "newly declared" type.  Perhaps I'm dumb, but after all these years
I still have to think carefully about that with typedefs when it's not obvious
("typedef UnfamiliarTypeX UnfamiliarTypeY").

+
   // 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
@@ skipping 80 matching lines @@
 
   // 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.
   float GetFrequency(const base::Time& now,
                      const bool bookmarked,
                      const VisitInfoVector& visits) 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);
+  // Combines the two component scores into a final score that's an appropriate
+  // value to use as a relevancy score.
+  static float GetFinalRelevancyScore(float topicality_score,
+                                      float frequency_score);
 
-  // Initializes the HQP experimental params: |hqp_relevance_buckets_|
-  // to default buckets. If hqp experimental scoring is enabled, it
-  // fetches the |hqp_experimental_scoring_enabled_|, |topicality_threshold_|
-  // and |hqp_relevance_buckets_| from omnibox field trials.
-  static void InitHQPExperimentalParams();
+  // Helper function that returns the string containing the scoring buckets
+  // (either the default ones or ones specified in an experiment).
+  static ScoreMaxRelevances GetHQPBuckets();
 
-  // Helper function to parse the string containing the scoring buckets.
-  // For example,
-  // String: "0.0:400,1.5:600,12.0:1300,20.0:1399"
-  // Buckets: vector[(0.0, 400),(1.5,600),(12.0,1300),(20.0,1399)]
-  // Returns false, in case if it fail to parse the string.
-  static bool GetHQPBucketsFromString(
-      const std::string& buckets_str,
-      std::vector<ScoreMaxRelevance>* hqp_buckets);
+  // Helper function to parse the string containing the scoring buckets and
+  // return the results.  For example, with |buckets_str| as
+  // "0.0:400,1.5:600,12.0:1300,20.0:1399", it returns [(0.0, 400), (1.5, 600),
+  // (12.0, 1300), (20.0, 1399)]. It returns an empty vector in the case of a
+  // malformed |buckets_str|.
+  static ScoreMaxRelevances GetHQPBucketsFromString(
+      const std::string& buckets_str);
 
   // If true, assign raw scores to be max(whatever it normally would be, a
   // score that's similar to the score HistoryURL provider would assign).
   static bool also_do_hup_like_scoring_;
 
   // Untyped visits to bookmarked pages score this, compared to 1 for
   // untyped visits to non-bookmarked pages and |typed_value_| for typed visits.
   static float bookmark_value_;
 
   // Typed visits to page score this, compared to 1 for untyped visits.
@@ skipping 13 matching lines @@
   // 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.
   // Words beyond this number are ignored.
   static size_t num_title_words_to_allow_;
 
-  // True, if hqp experimental scoring is enabled.
-  static bool hqp_experimental_scoring_enabled_;
-
   // |topicality_threshold_| is used to control the topicality scoring.
-  // If |topicality_threshold_| > 0, then URLs with topicality-score < threshold
-  // are given topicality score of 0. By default it is initalized to -1.
+  // If |topicality_threshold_| > 0, then URLs with topicality-score less than
+  // the threshold are given topicality score of 0.
   static float topicality_threshold_;
 
-  // |hqp_relevance_buckets_str_| is used to control the hqp score ranges.
-  // It is the string representation of |hqp_relevance_buckets_|.
-  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_;
+  // Used for testing.  A possibly null pointer to an vector.  If set,

[Peter Kasting, 2016/12/10 02:31:15]

Nit: an -> a

[Mark P, 2016/12/11 04:57:22]

Done.

+  // overrides the static local variable |relevance_buckets| declared in
+  // GetFinalRelevancyScore().
+  static ScoreMaxRelevances* relevance_buckets_override_;
 };
 typedef std::vector<ScoredHistoryMatch> ScoredHistoryMatches;
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_SCORED_HISTORY_MATCH_H_