Omnibox: Create Bundled Field Trial; Convert SearchHistory trial to it

chrome/browser/omnibox/omnibox_field_trial.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 CHROME_BROWSER_OMNIBOX_OMNIBOX_FIELD_TRIAL_H_
 #define CHROME_BROWSER_OMNIBOX_OMNIBOX_FIELD_TRIAL_H_
 
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "chrome/browser/autocomplete/autocomplete_input.h"
 
 // This class manages the Omnibox field trials.
 class OmniboxFieldTrial {
  public:
   // Creates the static field trial groups.
   // *** MUST NOT BE CALLED MORE THAN ONCE. ***
   static void ActivateStaticTrials();
 
   // Activates all dynamic field trials.  The main difference between
   // the autocomplete dynamic and static field trials is that the former
@@ skipping 72 matching lines @@
   // if the field trial is not active, etc., returns false.
   // CalculateScore()'s return value is a product of this maximum
   // relevance score and some attenuating factors that are all between
   // 0 and 1.  (Note that Shortcuts results may have their scores
   // reduced later if the assigned score is higher than allowed for
   // non-inlineable results.  Shortcuts results are not allowed to be
   // inlined.)
   static bool ShortcutsScoringMaxRelevance(int* max_relevance);
 
   // ---------------------------------------------------------
-  // For the SearchHistory field trial.
+  // For the SearchHistory experiment that's part of the bundled omnibox
+  // field trial.
 
-  // Returns true if the user is in the experiment group that scores
-  // search history query suggestions less aggressively so that they don't
-  // inline.
-  static bool SearchHistoryPreventInlining();
+  // Returns true if the user is in the experiment group that, given the
+  // provided |current_page_classification| context, scores search history
+  // query suggestions less aggressively so that they don't inline.
+  static bool SearchHistoryPreventInlining(
+      AutocompleteInput::PageClassification current_page_classification);
 
-  // Returns true if the user is in the experiment group that disables
-  // all query suggestions from search history.
-  static bool SearchHistoryDisable();
+  // Returns true if the user is in the experiment group that, given the
+  // provided |current_page_classification| context, disables all query
+  // suggestions from search history.
+  static bool SearchHistoryDisable(
+      AutocompleteInput::PageClassification current_page_classification);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(OmniboxFieldTrialTest,
+                           GetValueOfRuleInPageClassificationContext);
+
+  // Returns the value associated with the |rule| that applies in the context
+  // of |current_page_classification|.  If no such rule exists, looks for that
+  // rule in the global context and return its value if found.  If the rule
+  // remains unfound in the global context, returns the empty string.  For
+  // more details, see the implementation.  How to interpret the value is
+  // left to the caller; this is rule-dependent.
+  static std::string GetValueOfRuleInPageClassificationContext(

[Peter Kasting, 2013/08/01 22:44:48]

Nit: Slightly shorter might be

GetValueForRuleWithPageClassification()

or

GetValueOfRuleForPageClassification()

[Mark P, 2013/08/02 00:44:36]

They are shorter.  I think the word context definitely adds something here and
I'd rather not drop it.  Left as is.

Addendum after reading your other context comments: I think context is an
important notion.  For instance, I can easily see adding an input_type context
as well.  I can easily imagine contexts overlapping and having priorities
between them.  I think it really helps to think of the context as a bigger
notion.  Indeed, I'd rather drop PageClassification from this function name
rather than Context.  (the former can be inferred from the parameter types).

[Peter Kasting, 2013/08/02 00:49:33]

I partly react to this with "then update the name and comments to reflect that
larger context at the time you actually add such a larger context; but for now
you don't have it".  But if you're really committed to this, then I think you
should go ahead and drop "PageClassification" right now, and explicitly note in
your comments that while you only have one piece of context at the moment,
you're planning to add more.

But if you're not planning to add more and that comment is purely hypothetical,
then as I said, I would drop the notion of context for now, because as a reader
it's really confusing.

[Mark P, 2013/08/02 19:50:44]

Next week I'm going to add another piece of context (whether Instant Extended is
enabled), so I revised the comment and function name to put less emphasis on
page classification.

+      const std::string& rule,
+      AutocompleteInput::PageClassification current_page_classification);

[Peter Kasting, 2013/08/01 22:44:48]

Nit: "current" probably not necessary (this function doesn't really care if the
provided classification is in any sense "current")

[Mark P, 2013/08/02 00:44:36]

Good point.  Revised.

+
   DISALLOW_IMPLICIT_CONSTRUCTORS(OmniboxFieldTrial);
 };
 
 #endif  // CHROME_BROWSER_OMNIBOX_OMNIBOX_FIELD_TRIAL_H_