Remove NOTIFICATION_HISTORY_URL_VISITED

chrome/browser/search_engines/chrome_template_url_service_client.cc

 // Copyright 2014 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.
 
 #include "chrome/browser/search_engines/chrome_template_url_service_client.h"
 
-#include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/history_service.h"
-#include "chrome/browser/history/history_service_factory.h"
-#include "chrome/browser/profiles/profile.h"
 #include "components/search_engines/template_url_service.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
 #include "extensions/common/constants.h"
 
-ChromeTemplateURLServiceClient::ChromeTemplateURLServiceClient(Profile* profile)
-    : profile_(profile),
-      owner_(NULL) {
-  DCHECK(profile);
-
-  // Register for notifications.
+ChromeTemplateURLServiceClient::ChromeTemplateURLServiceClient(
+    HistoryService* history_service)
+    : owner_(NULL), history_service_(history_service) {
+  // Register as HistoryServiceObserver.

[Peter Kasting, 2014/10/14 16:40:53]

Nit: This comment doesn't add anything to the code, I'd remove it.

[sdefresne, 2014/10/14 16:49:42]

Done.

   // TODO(sky): bug 1166191. The keywords should be moved into the history
   // db, which will mean we no longer need this notification and the history
   // backend can handle automatically adding the search terms as the user
   // navigates.
-  content::Source<Profile> profile_source(profile->GetOriginalProfile());
-  notification_registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED,
-                              profile_source);
+  if (history_service_)
+    history_service_->AddObserver(this);
 }
 
 ChromeTemplateURLServiceClient::~ChromeTemplateURLServiceClient() {
+  if (history_service_)
+    history_service_->RemoveObserver(this);

[Peter Kasting, 2014/10/14 16:40:53]

This change adds a lifetime ordering requirement that the history service always
outlive this class.  Is that guaranteed?

[sdefresne, 2014/10/14 16:49:42]

Yes. ChromeTemplateURLServiceClient is owned by TemplateURLService which is a
KeyedService and the TemplateURLServiceFactory has a dependency on
HistoryServiceFactory, so the TemplateURLService outlives the HistoryService.

[Peter Kasting, 2014/10/14 16:50:39]

OK.  I would not be opposed to a comment that explicitly spells that out.

[sdefresne, 2014/10/17 14:32:38]

My comment was correct but my analysis wasn't.

ChromeTemplateURLServiceClient currently outlives HistoryService. I've
introduced a Shutdown() method that will be called from the
TemplateUTLServiceFactory::Shutdown() to remove the observer while the
HistoryService is still alive.

 }
 
 void ChromeTemplateURLServiceClient::SetOwner(TemplateURLService* owner) {
   DCHECK(!owner_);
   owner_ = owner;
 }
 
 void ChromeTemplateURLServiceClient::DeleteAllSearchTermsForKeyword(
     TemplateURLID id) {
-  HistoryService* history_service =
-      HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS);
-  if (history_service)
-    history_service->DeleteAllSearchTermsForKeyword(id);
+  if (history_service_)
+    history_service_->DeleteAllSearchTermsForKeyword(id);
 }
 
 void ChromeTemplateURLServiceClient::SetKeywordSearchTermsForURL(
     const GURL& url,
     TemplateURLID id,
     const base::string16& term) {
-  HistoryService* history_service =
-      HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS);
-  if (history_service)
-    history_service->SetKeywordSearchTermsForURL(url, id, term);
+  if (history_service_)
+    history_service_->SetKeywordSearchTermsForURL(url, id, term);
 }
 
 void ChromeTemplateURLServiceClient::AddKeywordGeneratedVisit(const GURL& url) {
-  HistoryService* history_service =
-      HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS);
-  if (history_service)
-    history_service->AddPage(url, base::Time::Now(), NULL, 0, GURL(),
-                             history::RedirectList(),
-                             ui::PAGE_TRANSITION_KEYWORD_GENERATED,
-                             history::SOURCE_BROWSED, false);
+  if (history_service_)
+    history_service_->AddPage(url, base::Time::Now(), NULL, 0, GURL(),
+                              history::RedirectList(),
+                              ui::PAGE_TRANSITION_KEYWORD_GENERATED,
+                              history::SOURCE_BROWSED, false);
 }
 
 void ChromeTemplateURLServiceClient::RestoreExtensionInfoIfNecessary(
     TemplateURL* template_url) {
   const TemplateURLData& data = template_url->data();
   GURL url(data.url());
   if (url.SchemeIs(extensions::kExtensionScheme)) {
     const std::string& extension_id = url.host();
     template_url->set_extension_info(make_scoped_ptr(
         new TemplateURL::AssociatedExtensionInfo(
             TemplateURL::OMNIBOX_API_EXTENSION, extension_id)));
   }
 }
 
-void ChromeTemplateURLServiceClient::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_URL_VISITED);
-
+void ChromeTemplateURLServiceClient::OnURLVisited(
+    HistoryService* history_service,
+    ui::PageTransition transition,
+    const history::URLRow& row,
+    const history::RedirectList& redirects,
+    base::Time visit_time) {
+  DCHECK_EQ(history_service_, history_service);
   if (!owner_)
     return;
 
-  content::Details<history::URLVisitedDetails> history_details(details);
   TemplateURLService::URLVisitedDetails visited_details;
-  visited_details.url = history_details->row.url();
+  visited_details.url = row.url();
   visited_details.is_keyword_transition =
-      ui::PageTransitionStripQualifier(history_details->transition) ==
-      ui::PAGE_TRANSITION_KEYWORD;
+      ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_KEYWORD);
   owner_->OnHistoryURLVisited(visited_details);
 }