No chrome dependencies in TemplateURLService

chrome/browser/search_engines/template_url_service.h

Index: chrome/browser/search_engines/template_url_service.h
diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h
index 34e18f65c7f8e4370af62c6cd5c7997ebd7cfb7f..4786038a3a52d442d0082de3a67cd0e88a86e42a 100644
--- a/chrome/browser/search_engines/template_url_service.h
+++ b/chrome/browser/search_engines/template_url_service.h
@@ -23,18 +23,16 @@
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_id.h"
 #include "components/webdata/common/web_data_service_consumer.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/syncable_service.h"
 
 class GURL;
 class PrefService;
-class Profile;
 class SearchHostToURLsMap;
 class SearchTermsData;
 class TemplateURL;
 struct TemplateURLData;
+class TemplateURLService;
 class TemplateURLServiceObserver;
 
 namespace rappor {
@@ -46,9 +44,27 @@ class SyncData;
 class SyncErrorFactory;
 }
 
-namespace history {
-struct URLVisitedDetails;
-}
+// This interface provides history related functionalities required by

[Peter Kasting, 2014/07/02 20:45:57]

Nit: functionalities -> functionality

[hashimoto, 2014/07/03 00:05:07]

Done.

+// TemplateURLService.
+// TODO(hashimoto): Get rid of this once HistoryService gets componentized.
+class KeywordHistoryServiceInterface {

[Peter Kasting, 2014/07/02 20:45:57]

Nit: I think this interface should be declared in its own header file.

Also, isn't there some existing naming convention for classes declared in
components/ which are then implemented concretely in chrome/?  I didn't think
"XXXInterface"/"XXX" was the convention there, but maybe I'm wrong.

One of the componentization leads would hopefully know this.

[hashimoto, 2014/07/03 00:05:07]

Some components have "XXXClient" classes which are implmented by chrome as
"ChromeXXXClient". (e.g. HistoryClient/ChromeHistoryClient)

Following this example, renamed this interface to KeywordHistoryService and the
implementation, the former KeywordHistoryService, is renamed to
ChromeKeywordHistoryService.
The interface definition is in its own file.

I'll ask blundell@ for his opinion as well.

[hashimoto, 2014/07/03 07:13:31]

Colin, I'm going to introduce a new interface KeywordHistoryService to insulate
HistoryService and its chrome implementation is named as
ChromeKeywordHistoryService.
Does this naming sound good to you?

+ public:
+  virtual ~KeywordHistoryServiceInterface() {}
+
+  // Sets the pointer to the owner of this object.
+  virtual void SetOwner(TemplateURLService* owner) = 0;
+
+  // Deletes all search terms for the specified keyword.
+  virtual void DeleteAllSearchTermsForKeyword(TemplateURLID id) = 0;
+
+  // Sets the search terms for the specified url and keyword.
+  virtual void SetKeywordSearchTermsForURL(const GURL& url,
+                                           TemplateURLID id,
+                                           const base::string16& term) = 0;
+
+  // Adds the given URL to history as a keyword generated visit.
+  virtual void AddKeywordGeneratedVisit(const GURL& url) = 0;
+};
 
 // TemplateURLService is the backend for keywords. It's used by
 // KeywordAutocomplete.
@@ -59,8 +75,6 @@ struct URLVisitedDetails;
 // This allows TemplateURLService to notify listeners of changes as well as keep
 // the database in sync.
 //
-// There is a TemplateURLService per Profile.
-//
 // TemplateURLService does not load the vector of TemplateURLs in its
 // constructor (except for testing). Use the Load method to trigger a load.
 // When TemplateURLService has completed loading, observers are notified via
@@ -73,7 +87,6 @@ struct URLVisitedDetails;
 
 class TemplateURLService : public WebDataServiceConsumer,
                            public KeyedService,
-                           public content::NotificationObserver,
                            public syncer::SyncableService {
  public:
   typedef std::map<std::string, std::string> QueryTerms;
@@ -91,7 +104,16 @@ class TemplateURLService : public WebDataServiceConsumer,
     const char* const content;
   };
 
-  TemplateURLService(Profile* profile,
+  struct URLVisitedDetails {
+    GURL url;
+    bool is_keyword_transition;
+  };
+
+  TemplateURLService(PrefService* prefs,
+                     scoped_ptr<SearchTermsData> search_terms_data,
+                     KeywordWebDataService* web_data_service,
+                     scoped_ptr<KeywordHistoryServiceInterface> history_service,
+                     GoogleURLTracker* google_url_tracker,
                      rappor::RapporService* rappor_service,
                      const base::Closure& dsp_change_callback);
   // The following is for testing.
@@ -299,10 +321,8 @@ class TemplateURLService : public WebDataServiceConsumer,
   base::string16 GetKeywordShortName(const base::string16& keyword,
                                      bool* is_omnibox_api_extension_keyword);
 
-  // content::NotificationObserver implementation.
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
+  // Called by the history service when a URL is visited.
+  void OnHistoryURLVisited(const URLVisitedDetails& details);
 
   // KeyedService implementation.
   virtual void Shutdown() OVERRIDE;
@@ -515,8 +535,7 @@ class TemplateURLService : public WebDataServiceConsumer,
   // Iterates through the TemplateURLs to see if one matches the visited url.
   // For each TemplateURL whose url matches the visited url
   // SetKeywordSearchTermsForURL is invoked.
-  void UpdateKeywordSearchTermsForURL(
-      const history::URLVisitedDetails& details);
+  void UpdateKeywordSearchTermsForURL(const URLVisitedDetails& details);
 
   // If necessary, generates a visit for the site http:// + t_url.keyword().
   void AddTabToSearchVisit(const TemplateURL& t_url);
@@ -649,7 +668,6 @@ class TemplateURLService : public WebDataServiceConsumer,
   // engine.
   void UpdateExtensionDefaultSearchEngine();
 
-  content::NotificationRegistrar notification_registrar_;
   PrefChangeRegistrar pref_change_registrar_;
 
   // Mapping from keyword to the TemplateURL.
@@ -667,17 +685,19 @@ class TemplateURLService : public WebDataServiceConsumer,
   // header dependencies.
   scoped_ptr<SearchHostToURLsMap> provider_map_;
 
-  // Used to obtain the WebDataService.
-  // When Load is invoked, if we haven't yet loaded, the WebDataService is
-  // obtained from the Profile. This allows us to lazily access the database.
-  Profile* profile_;
-
   PrefService* prefs_;
 
-  rappor::RapporService* rappor_service_;
-
   scoped_ptr<SearchTermsData> search_terms_data_;

[Peter Kasting, 2014/07/02 20:45:57]

Tiny nit: The order of these members feels a little random... we have weak
pointers to other services (e.g. prefs_ and google_url_tracker_) mixed in with
strong pointers that we own.

I wouldn't necessarily worry about this in this change, but if you agree, it
might be worth a small followup change to just go through all this class'
members and attempt to create some sort of logical grouping/ordering.

[hashimoto, 2014/07/03 00:05:07]

My logic behind this ordering is based on how often tests pass NULL to the ctor.
(It should loosely reflect how important these classes are for
TemplateURLService)
PrefService* and SearchTermsData never be NULL.  Some tests pass non-NULL
pointers as KeywordWebDataService and KeywordHistoryServiceInterface when they
are needed.  GoogleURLTracker, RapporService and dsp_change_callback_ are almost
always NULL during tests.

If you prefer another ordering, I'd be happy to make another CL for it.

[Peter Kasting, 2014/07/03 00:12:31]

Hmm.  That ordering is not at all obvious to a reader.

If you want to stick with this, I'd at least add a comment explaining this. 
Personally, it seems highly unusual to me (and not necessarily very useful...
why would a reader care which members have a higher frequency of being NULL?),
but I don't care enough to demand something else.

[hashimoto, 2014/07/03 00:36:24]

Also this ordering divides these members into 3 categories, the current
browser's state (PrefService and SearchTermsData), dependencies to other
components (web_data_service_, history_service_ and google_url_tracker_) and
metrics/logging (RapporService and dsp_change_callback_).

I have no reason to stick with this, though.
The current ordering is the result of my not-perfectly-accomplished effort to
try to make some sense.
Should I order them based on the ownership model?

 
+  // Service used to store entries.
+  scoped_refptr<KeywordWebDataService> web_data_service_;
+
+  scoped_ptr<KeywordHistoryServiceInterface> history_service_;
+
+  GoogleURLTracker* google_url_tracker_;
+
+  rappor::RapporService* rappor_service_;
+
   // This closure is run when the default search provider is set to Google.
   base::Closure dsp_change_callback_;
 
@@ -692,12 +712,9 @@ class TemplateURLService : public WebDataServiceConsumer,
   // If non-zero, we're waiting on a load.
   KeywordWebDataService::Handle load_handle_;
 
-  // Service used to store entries.
-  scoped_refptr<KeywordWebDataService> web_data_service_;
-
   // All visits that occurred before we finished loading. Once loaded
   // UpdateKeywordSearchTermsForURL is invoked for each element of the vector.
-  std::vector<history::URLVisitedDetails> visits_to_add_;
+  std::vector<URLVisitedDetails> visits_to_add_;
 
   // Once loaded, the default search provider.  This is a pointer to a
   // TemplateURL owned by |template_urls_|.