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

chrome/browser/search/instant_service.h

 // Copyright 2013 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_SEARCH_INSTANT_SERVICE_H_
 #define CHROME_BROWSER_SEARCH_INSTANT_SERVICE_H_
 
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #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"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
 class GURL;
 class InstantIOContext;
 class InstantServiceObserver;
 class InstantTestBase;
@@ skipping 62 matching lines @@
   // changed.
   void OnOmniboxStartMarginChanged(int start_margin);
 
   InstantSearchPrerenderer* instant_search_prerenderer() {
     return instant_prerenderer_.get();
   }
 
   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;
   friend class InstantUnitTestBase;
 
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedManualTest,
                            MANUAL_SearchesFromFakebox);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, ProcessIsolation);
   FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, SendsSearchURLsToRenderer);
 
@@ skipping 15 matching lines @@
 
   // Notifies the observer about the last known most visited items.
   void NotifyAboutMostVisitedItems();
 
   // Theme changed notification handler.
   void OnThemeChanged(ThemeService* theme_service);
 
   void OnGoogleURLUpdated(Profile* profile,
                           GoogleURLTracker::UpdatedDetails* details);
 
-  void OnDefaultSearchProviderChanged(const std::string& pref_name);
+  void OnTemplateURLServiceChanged();
 
   void ResetInstantSearchPrerenderer();
 
   Profile* const profile_;
 
   // The process ids associated with Instant processes.
   std::set<int> process_ids_;
 
   // InstantMostVisitedItems sent to the Instant Pages.
   std::vector<InstantMostVisitedItem> most_visited_items_;
@@ skipping 12 matching lines @@
   PrefChangeRegistrar profile_pref_registrar_;
 
   scoped_refptr<InstantIOContext> instant_io_context_;
 
   // Set to NULL if the default search provider does not support Instant.
   scoped_ptr<InstantSearchPrerenderer> instant_prerenderer_;
 
   // 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);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_INSTANT_SERVICE_H_