No chrome dependencies in TemplateURLService

chrome/browser/search_engines/template_url_service.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_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 #define CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 
 #include <list>
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback_list.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/prefs/pref_change_registrar.h"
 #include "chrome/browser/webdata/keyword_web_data_service.h"
 #include "components/google/core/browser/google_url_tracker.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/search_engines/default_search_manager.h"
 #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 {
 class RapporService;
 }
 
 namespace syncer {
 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.
 //
 // TemplateURLService stores a vector of TemplateURLs. The TemplateURLs are
 // persisted to the database maintained by KeywordWebDataService.
 // *ALL* mutations to the TemplateURLs must funnel through TemplateURLService.
 // 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
 // OnTemplateURLServiceChanged, or by a callback registered prior to calling
 // the Load method.
 //
 // TemplateURLService takes ownership of any TemplateURL passed to it. If there
 // is a KeywordWebDataService, deletion is handled by KeywordWebDataService,
 // otherwise TemplateURLService handles deletion.
 
 class TemplateURLService : public WebDataServiceConsumer,
                            public KeyedService,
-                           public content::NotificationObserver,
                            public syncer::SyncableService {
  public:
   typedef std::map<std::string, std::string> QueryTerms;
   typedef std::vector<TemplateURL*> TemplateURLVector;
   // Type for a static function pointer that acts as a time source.
   typedef base::Time(TimeProvider)();
   typedef std::map<std::string, syncer::SyncData> SyncDataMap;
   typedef base::CallbackList<void(void)>::Subscription Subscription;
 
   // Struct used for initializing the data store with fake data.
   // Each initializer is mapped to a TemplateURL.
   struct Initializer {
     const char* const keyword;
     const char* const url;
     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.
   TemplateURLService(const Initializer* initializers, const int count);
   virtual ~TemplateURLService();
 
   // Creates a TemplateURLData that was previously saved to |prefs| via
   // SaveDefaultSearchProviderToPrefs or set via policy.
   // Returns true if successful, false otherwise.
   // If the user or the policy has opted for no default search, this
@@ skipping 187 matching lines @@
   virtual void OnWebDataServiceRequestDone(
       KeywordWebDataService::Handle h,
       const WDTypedResult* result) OVERRIDE;
 
   // Returns the locale-direction-adjusted short name for the given keyword.
   // Also sets the out param to indicate whether the keyword belongs to an
   // Omnibox extension.
   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;
 
   // syncer::SyncableService implementation.
 
   // Returns all syncable TemplateURLs from this model as SyncData. This should
   // include every search engine and no Extension keywords.
   virtual syncer::SyncDataList GetAllSyncData(
       syncer::ModelType type) const OVERRIDE;
@@ skipping 192 matching lines @@
                                               PrefService* prefs);
 
   // If the TemplateURL's sync GUID matches the kSyncedDefaultSearchProviderGUID
   // preference it will be used to update the DSE in memory and as persisted in
   // preferences.
   void MaybeUpdateDSEAfterSync(TemplateURL* synced_turl);
 
   // 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);
 
   // Invoked when the Google base URL has changed. Updates the mapping for all
   // TemplateURLs that have a replacement term of {google:baseURL} or
   // {google:baseSuggestURL}.
   void GoogleBaseURLChanged();
 
   // Adds a new TemplateURL to this model. TemplateURLService will own the
@@ skipping 112 matching lines @@
   // Finds the extension-supplied TemplateURL that matches |data|, if any.
   TemplateURL* FindMatchingExtensionTemplateURL(const TemplateURLData& data,
                                                 TemplateURL::Type type);
 
   // Finds the most recently-installed NORMAL_CONTROLLED_BY_EXTENSION engine
   // that supports replacement and wants to be default, if any. Notifies the
   // DefaultSearchManager, which might change the effective default search
   // engine.
   void UpdateExtensionDefaultSearchEngine();
 
-  content::NotificationRegistrar notification_registrar_;
   PrefChangeRegistrar pref_change_registrar_;
 
   // Mapping from keyword to the TemplateURL.
   KeywordToTemplateMap keyword_to_template_map_;
 
   // Mapping from Sync GUIDs to the TemplateURL.
   GUIDToTemplateMap guid_to_template_map_;
 
   TemplateURLVector template_urls_;
 
   ObserverList<TemplateURLServiceObserver> model_observers_;
 
   // Maps from host to set of TemplateURLs whose search url host is host.
   // NOTE: This is always non-NULL; we use a scoped_ptr<> to avoid circular
   // 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_;
 
+  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_;
 
-  scoped_ptr<SearchTermsData> search_terms_data_;
-
   // This closure is run when the default search provider is set to Google.
   base::Closure dsp_change_callback_;
 
   // Whether the keywords have been loaded.
   bool loaded_;
 
   // Set when the web data service fails to load properly.  This prevents
   // further communication with sync or writing to prefs, so we don't persist
   // inconsistent state data anywhere.
   bool load_failed_;
 
   // 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_|.
   TemplateURL* default_search_provider_;
 
   // A temporary location for the DSE until Web Data has been loaded and it can
   // be merged into |template_urls_|.
   scoped_ptr<TemplateURL> initial_default_search_provider_;
 
   // Source of the default search provider.
@@ skipping 39 matching lines @@
 
   // Helper class to manage the default search engine.
   DefaultSearchManager default_search_manager_;
 
   scoped_ptr<GoogleURLTracker::Subscription> google_url_updated_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_