Break up dependency cycle between search_engines and omnibox

In 2015 a dependency from search_engines to omnibox was created
and it's created a cycle that causes linking problems in some
ios+jumbo configurations. It works without jumbo because the cycle
will be part of a dead .o file which will be ignored by the linker.
With jumbo the .o file is not 100% dead anymore since dead and live
code will be mixed in it.

Bug: 488901
Change-Id: I4b36a1b4e9f46a2814d9562f288e55dadc63d9dd
Reviewed-on: https://chromium-review.googlesource.com/c/1254082
Commit-Queue: Daniel Bratell <bratell@opera.com>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597093}

components/omnibox/browser/BUILD.gn

@@ -214,9 +214,6 @@ jumbo_static_library("browser") {
       "//components/vector_icons",
     ]
   }
-
-  # TODO(brettw) Fix the include cycle and remove this line.
-  allow_circular_includes_from = [ "//components/search_engines" ]
 }
 
 if (is_android) {

components/omnibox/browser/omnibox_field_trial.cc

@@ -668,13 +668,6 @@ bool OmniboxFieldTrial::HUPSearchDatabase() {
   return value.empty() || (value == "true");
 }
 
-bool OmniboxFieldTrial::KeywordRequiresRegistry() {
-  const std::string& value = variations::GetVariationParamValue(
-      kBundledExperimentFieldTrialName,
-      kKeywordRequiresRegistryRule);
-  return value.empty() || (value == "true");
-}
-
 bool OmniboxFieldTrial::KeywordRequiresPrefixMatch() {
   const std::string& value = variations::GetVariationParamValue(
       kBundledExperimentFieldTrialName,

components/omnibox/browser/omnibox_field_trial.h

@@ -389,11 +389,15 @@ class OmniboxFieldTrial {
   // For the aggressive keyword matching experiment that's part of the bundled
   // omnibox field trial.
 
-  // Returns whether KeywordProvider should consider the registry portion
+  // One function is missing from here to avoid a cyclic dependency
+  // between search_engine and omnibox. In the search_engine component
+  // there is a OmniboxFieldTrialKeywordRequiresRegistry function
+  // that logically should be here.
+  //
+  // It returns whether KeywordProvider should consider the registry portion
   // (e.g., co.uk) of keywords that look like hostnames as an important part of
   // the keyword name for matching purposes.  Returns true if the experiment
   // isn't active.
-  static bool KeywordRequiresRegistry();
 
   // For keywords that look like hostnames, returns whether KeywordProvider
   // should require users to type a prefix of the hostname to match against

components/search_engines/BUILD.gn

@@ -65,23 +65,18 @@ static_library("search_engines") {
     ":prepopulated_engines",
     "//base:i18n",
     "//components/history/core/browser",
-    "//third_party/metrics_proto",
-
-    # The search_engines target is in an include cycle with
-    # components/omnibox/browser. The cycle is whitelisted in the
-    # omnibox/browser target, but should ideally be fixed, then this
-    # dependency added:
-    #"//components/omnibox/browser",
     "//components/infobars/core",
     "//components/pref_registry",
     "//components/rappor",
     "//components/strings",
     "//components/url_formatter",
+    "//components/variations",
     "//google_apis",
     "//net",
     "//services/network/public/cpp",
     "//sql",
     "//third_party/libxml",
+    "//third_party/metrics_proto",
     "//ui/base",
     "//ui/gfx",
     "//ui/gfx/geometry",

components/search_engines/DEPS

@@ -3,8 +3,6 @@ include_rules = [
   "+components/history/core",
   "+components/infobars/core",
   "+components/keyed_service/core",
-  # TODO(mpearson): for experiment; remove after crbug.com/488901 is launched.
-  "+components/omnibox/browser/omnibox_field_trial.h",
   "+components/policy",
   "+components/pref_registry",
   "+components/prefs",
@@ -13,6 +11,7 @@ include_rules = [
   "+components/sync",
   "+components/sync_preferences/testing_pref_service_syncable.h",
   "+components/url_formatter",
+  "+components/variations",
   "+components/webdata",
   "+google_apis",
   "+libxml",

components/search_engines/template_url_service.cc

@@ -15,7 +15,6 @@
 #include "base/stl_util.h"
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
-#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/rappor_service_impl.h"
@@ -30,6 +29,7 @@
 #include "components/sync/protocol/search_engine_specifics.pb.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/url_formatter/url_fixer.h"
+#include "components/variations/variations_associated_data.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "url/gurl.h"
 
@@ -150,12 +150,31 @@ base::string16 GetDomainAndRegistry(const base::string16& host) {
           net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES));
 }
 
+// For keywords that look like hostnames, returns whether KeywordProvider
+// should require users to type a prefix of the hostname to match against
+// them, rather than just the domain name portion. In other words, returns
+// whether the prefix before the domain name should be considered important
+// for matching purposes. Returns true if the experiment isn't active.
+bool OmniboxFieldTrialKeywordRequiresRegistry() {
+  // This would normally be
+  // bool OmniboxFieldTrial::KeywordRequiresRegistry()
+  // but that would create a dependency cycle since omnibox depends on
+  // search_engines (and search -> search_engines)
+  constexpr char kBundledExperimentFieldTrialName[] =
+      "OmniboxBundledExperimentV1";
+  constexpr char kKeywordRequiresRegistryRule[] = "KeywordRequiresRegistry";
+  const std::string value = variations::GetVariationParamValue(
+      kBundledExperimentFieldTrialName, kKeywordRequiresRegistryRule);
+  return value.empty() || (value == "true");
+}
+
 // Returns the length of the important part of the |keyword|, assumed to be
 // associated with the TemplateURL.  For instance, for the keyword
 // google.co.uk, this can return 6 (the length of "google").
 size_t GetMeaningfulKeywordLength(const base::string16& keyword,
                                   const TemplateURL* turl) {
-  if (OmniboxFieldTrial::KeywordRequiresRegistry())
+  // Using Omnibox from here is a layer violation and should be fixed.
+  if (OmniboxFieldTrialKeywordRequiresRegistry())
     return keyword.length();
   const size_t registry_length = GetRegistryLength(keyword);
   if (registry_length == std::string::npos)