Use the DefaultSearchManager as the exclusive authority on DSE, ignoring Web Data.

chrome/browser/search/instant_service.h

Index: chrome/browser/search/instant_service.h
diff --git a/chrome/browser/search/instant_service.h b/chrome/browser/search/instant_service.h
index 393d0721fa06c06b3657c61f5e59482b230145f6..3c57d860956d5e97195441af241f793bd01f52eb 100644
--- a/chrome/browser/search/instant_service.h
+++ b/chrome/browser/search/instant_service.h
@@ -18,6 +18,7 @@
 #include "base/prefs/pref_change_registrar.h"
 #include "chrome/browser/google/google_url_tracker.h"
 #include "chrome/browser/history/history_types.h"
+#include "chrome/browser/search_engines/template_url_service_observer.h"
 #include "chrome/browser/ui/search/instant_search_prerenderer.h"
 #include "chrome/common/instant_types.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -100,6 +101,16 @@ class InstantService : public KeyedService,
   int omnibox_start_margin() const { return omnibox_start_margin_; }
 
  private:
+  class DefaultSearchEngineWatcher : public TemplateURLServiceObserver {

[Peter Kasting, 2014/05/03 00:28:18]

What do we gain by having this class instead of making InstantService a
TemplateURLServiceObserver directly?  It seems like this just calls the
identically-named method on its owning class.  I can't see the benefit.

[erikwright (departed), 2014/05/03 00:54:08]

It maintains encapsulation. OnTemplateURLServiceChanged is not intended to be
part of the public API of InstantService. Clients should not be able to invoke
it directly. Private inheritance would be an alternative approach but it
violates our style guide.

The style guide also notes that "Composition is often more appropriate than
inheritance."

In fact, some of the tests that had to be changed in this CL were tests that
called TemplateURLService::Observe directly. That's one example of a case where
weak encapsulation made this work harder.

I realize this is not the most common practice in Chromium, but I do think it
would be better if it were more widely used. I will go whichever way you prefer.

[Peter Kasting, 2014/05/03 01:18:17]

That's fine, you should be declaring that method private anyway, which prevents
people from calling it unless they're holding a valid pointer to a
TemplateURLServiceObserver (in which case they probably ought to be able to call
this -- but in general no one should do this).  That should allow you to
subclass directly.

(This is a reason I always advise people to declare method overrides "as private
as possible" instead of matching the visibility of the method declaration in the
base class.)

[erikwright (departed), 2014/05/03 01:24:59]

It's not as though converting from an InstantService to a
TemplateURLServiceObserver requires any hoops.

It's not production code that is likely to do this, it's tests, where nobody is
going to bat an eye if the line preceding a call to OnTemplateURLServiceChanged
is preceded by an assignment using implicit cast.

I will follow your preference, nonetheless.

[Peter Kasting, 2014/05/03 01:31:01]

It requires an explicit upcast or assignment.  That's enough of a hurdle that
any reviewer ought to look askance at such a line.
Well, as I just said, they ought to bat an eye.  But still, calling this
function directly _isn't_ actually problematic.  It doesn't do anything bad or
cause any state inconsistency.  This is an issue of theoretical API purity, but
it's _not_ an issue of practical problematic API, which means if non-production
code that does something unusual wants to call this, nothing is going to go
haywire -- and if in the future this does result in state issues, then the
affected code will be test code.  Which is precisely what you want to have break
if people are using things wrong.

I don't think you should feel like you're doing something suboptimal and lame
here.  This really does seem fine to me -- certainly safer than e.g.
FRIEND_TEST(), which we use all over the place.

[erikwright (departed), 2014/05/05 01:49:38]

Done.

+   public:
+    explicit DefaultSearchEngineWatcher(InstantService* outer);

[Peter Kasting, 2014/05/03 00:28:18]

Nit: I suggest explicitly declaring a virtual destructor.  (It's possible the
style linter would ask you for a destructor on this class anyway.)  Also,
|outer| seems like perhaps a worse name than |parent| or |owner|.

[erikwright (departed), 2014/05/05 01:49:38]

Done.

+    virtual void OnTemplateURLServiceChanged() OVERRIDE;
+
+   private:
+    InstantService* outer_;
+    DISALLOW_COPY_AND_ASSIGN(DefaultSearchEngineWatcher);

[Peter Kasting, 2014/05/03 00:28:18]

Tiny nit: I'd put a blank line above this.  Also probably between the
constructor and the virtual function above.

[erikwright (departed), 2014/05/05 01:49:38]

Done.

+  };
+
   friend class InstantExtendedTest;
   friend class InstantServiceTest;
   friend class InstantTestBase;
@@ -135,7 +146,7 @@ class InstantService : public KeyedService,
   void OnGoogleURLUpdated(Profile* profile,
                           GoogleURLTracker::UpdatedDetails* details);
 
-  void OnDefaultSearchProviderChanged(const std::string& pref_name);
+  void OnTemplateURLServiceChanged();
 
   void ResetInstantSearchPrerenderer();
 
@@ -168,6 +179,12 @@ class InstantService : public KeyedService,
   // Used for Top Sites async retrieval.
   base::WeakPtrFactory<InstantService> weak_ptr_factory_;
 
+  // Used to avoid resetting the instant_prerenderer_ if an unrelated change to
+  // the TemplateURLService occurs.
+  GURL current_search_result_prefetch_base_url_;
+
+  DefaultSearchEngineWatcher default_search_engine_watcher_;
+
   DISALLOW_COPY_AND_ASSIGN(InstantService);
 };