Remove VisitedLink dependency on rest of chrome

This is in preparation of componentizing VisitedLink implementation and
be shared with Android WebView.

* Added VisitedLinkDelegate which HistoryService implements.
* Removed VisitedLinkMasterFactory and have HistoryService directly own
  VisitedLinkMaster.
* Introduce URLIterator interface for DeleteURLs.

Last committed in https://src.chromium.org/viewvc/chrome?view=rev&revision=175186

Reverted in https://src.chromium.org/viewvc/chrome?view=rev&revision=175217
due to memory leaks.

Before this patch, VisitedLink was a ProfileKeyedService and some using
HistoryService did not create VisitedLink at all. After this patch,
VisitedLink is created and owned directly by HistoryService, so they are
now created for some tests depending on which HistoryService constructor
is used.

First version was leaking URLEnumerator in HistoryService::RebuildTable
which was doing manual ref counting (AddRef/Release) calls. Some tests do
not start the background thread at all, so URLEnumerator::OnComplete is
never called to delete itself.

Fixed by properly ref-counting URLEnumerator with scoped_refptr in this
path.

BUG=168716

Review URL: https://chromiumcodereview.appspot.com/11573060

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175906 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/history/DEPS

@@ -51,6 +51,8 @@ include_rules = [
   "!chrome/browser/ui/profile_error_dialog.h",
   "!chrome/browser/ui/webui/ntp/most_visited_handler.h",
   "!chrome/browser/ui/webui/ntp/new_tab_ui.h",
+  # TODO(boliu): Allow visitedlink after they are componentized.
+  "!chrome/browser/visitedlink/visitedlink_delegate.h",
   "!chrome/browser/visitedlink/visitedlink_master.h",
 ]
 

chrome/browser/history/history.cc

@@ -111,6 +111,29 @@ void RunWithFaviconResults(
   callback.Run(results->bitmap_results, results->size_map);
 }
 
+// Extract history::URLRows into GURLs for VisitedLinkMaster.
+class URLIteratorFromURLRows : public VisitedLinkMaster::URLIterator {
+ public:
+  explicit URLIteratorFromURLRows(const history::URLRows& url_rows)
+      : itr_(url_rows.begin()),
+        end_(url_rows.end()) {
+  }
+
+  virtual const GURL& NextURL() OVERRIDE {
+    return (itr_++)->url();
+  }
+
+  virtual bool HasNextURL() const OVERRIDE {
+    return itr_ != end_;
+  }
+
+ private:
+  history::URLRows::const_iterator itr_;
+  history::URLRows::const_iterator end_;
+
+  DISALLOW_COPY_AND_ASSIGN(URLIteratorFromURLRows);
+};
+
 }  // namespace
 
 // Sends messages from the backend to us on the main thread. This must be a
@@ -208,6 +231,8 @@ HistoryService::HistoryService(Profile* profile)
     : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       thread_(new base::Thread(kHistoryThreadName)),
       profile_(profile),
+      visitedlink_master_(new VisitedLinkMaster(
+          profile, ALLOW_THIS_IN_INITIALIZER_LIST(this))),
       backend_loaded_(false),
       current_backend_id_(-1),
       bookmark_service_(NULL),
@@ -472,10 +497,8 @@ void HistoryService::AddPage(const history::HistoryAddPageArgs& add_page_args) {
     return;
 
   // Add link & all redirects to visited link list.
-  VisitedLinkMaster* visited_links;
-  if (profile_ &&
-      (visited_links = VisitedLinkMaster::FromProfile(profile_))) {
-    visited_links->AddURL(add_page_args.url);
+  if (visitedlink_master_) {
+    visitedlink_master_->AddURL(add_page_args.url);
 
     if (!add_page_args.redirects.empty()) {
       // We should not be asked to add a page in the middle of a redirect chain.
@@ -485,7 +508,7 @@ void HistoryService::AddPage(const history::HistoryAddPageArgs& add_page_args) {
       // We need the !redirects.empty() condition above since size_t is unsigned
       // and will wrap around when we subtract one from a 0 size.
       for (size_t i = 0; i < add_page_args.redirects.size() - 1; i++)
-        visited_links->AddURL(add_page_args.redirects[i]);
+        visitedlink_master_->AddURL(add_page_args.redirects[i]);
     }
   }
 
@@ -530,11 +553,8 @@ void HistoryService::AddPageWithDetails(const GURL& url,
     return;
 
   // Add to the visited links system.
-  VisitedLinkMaster* visited_links;
-  if (profile_ &&
-      (visited_links = VisitedLinkMaster::FromProfile(profile_))) {
-    visited_links->AddURL(url);
-  }
+  if (visitedlink_master_)
+    visitedlink_master_->AddURL(url);
 
   history::URLRow row(url);
   row.set_title(title);
@@ -554,16 +574,14 @@ void HistoryService::AddPagesWithDetails(const history::URLRows& info,
                                          history::VisitSource visit_source) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // Add to the visited links system.
-  VisitedLinkMaster* visited_links;
-  if (profile_ &&
-      (visited_links = VisitedLinkMaster::FromProfile(profile_))) {
+  if (visitedlink_master_) {
     std::vector<GURL> urls;
     urls.reserve(info.size());
     for (history::URLRows::const_iterator i = info.begin(); i != info.end();
          ++i)
       urls.push_back(i->url());
 
-    visited_links->AddURLs(urls);
+    visitedlink_master_->AddURLs(urls);
   }
 
   ScheduleAndForget(PRIORITY_NORMAL,
@@ -720,11 +738,6 @@ void HistoryService::SetImportedFavicons(
                     &HistoryBackend::SetImportedFavicons, favicon_usage);
 }
 
-void HistoryService::IterateURLs(URLEnumerator* enumerator) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::IterateURLs, enumerator);
-}
-
 HistoryService::Handle HistoryService::QueryURL(
     const GURL& url,
     bool want_visits,
@@ -902,17 +915,16 @@ void HistoryService::Observe(int type,
       // delete notifications are by time. We would also like to be more
       // respectful of privacy and never tell the user something is gone when it
       // isn't. Therefore, we update the delete URLs after the fact.
-      if (!profile_)
-        return;  // No profile, probably unit testing.
-      content::Details<history::URLsDeletedDetails> deleted_details(details);
-      VisitedLinkMaster* visited_links =
-          VisitedLinkMaster::FromProfile(profile_);
-      if (!visited_links)
-        return;  // Nobody to update.
-      if (deleted_details->all_history)
-        visited_links->DeleteAllURLs();
-      else  // Delete individual ones.
-        visited_links->DeleteURLs(deleted_details->rows);
+      if (visitedlink_master_) {
+        content::Details<history::URLsDeletedDetails> deleted_details(details);
+
+        if (deleted_details->all_history) {
+          visitedlink_master_->DeleteAllURLs();
+        } else {
+          URLIteratorFromURLRows iterator(deleted_details->rows);
+          visitedlink_master_->DeleteURLs(&iterator);
+        }
+      }
       break;
     }
 
@@ -926,6 +938,23 @@ void HistoryService::Observe(int type,
   }
 }
 
+bool HistoryService::AreEquivalentContexts(content::BrowserContext* context1,
+                                           content::BrowserContext* context2) {
+  DCHECK(context1);
+  DCHECK(context2);
+
+  Profile* profile1 = Profile::FromBrowserContext(context1);
+  Profile* profile2 = Profile::FromBrowserContext(context2);
+
+  return profile1->IsSameProfile(profile2);
+}
+
+void HistoryService::RebuildTable(
+    const scoped_refptr<URLEnumerator>& enumerator) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::IterateURLs, enumerator);
+}
+
 bool HistoryService::Init(const FilePath& history_dir,
                           BookmarkService* bookmark_service,
                           bool no_db) {
@@ -949,6 +978,12 @@ bool HistoryService::Init(const FilePath& history_dir,
 
   // Create the history backend.
   LoadBackendIfNecessary();
+
+  if (visitedlink_master_) {
+    bool result = visitedlink_master_->Init();
+    DCHECK(result);
+  }
+
   return true;
 }
 

chrome/browser/history/history.h

@@ -25,6 +25,7 @@
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "chrome/browser/search_engines/template_url_id.h"
+#include "chrome/browser/visitedlink/visitedlink_delegate.h"
 #include "chrome/common/cancelable_task_tracker.h"
 #include "chrome/common/ref_counted_util.h"
 #include "content/public/browser/notification_observer.h"
@@ -45,6 +46,7 @@ class HistoryURLProvider;
 class PageUsageData;
 class PageUsageRequest;
 class Profile;
+class VisitedLinkMaster;
 struct HistoryURLProviderParams;
 
 namespace base {
@@ -107,7 +109,8 @@ class HistoryDBTask : public base::RefCountedThreadSafe<HistoryDBTask> {
 class HistoryService : public CancelableRequestProvider,
                        public content::NotificationObserver,
                        public syncer::SyncableService,
-                       public ProfileKeyedService {
+                       public ProfileKeyedService,
+                       public VisitedLinkDelegate {
  public:
   // Miscellaneous commonly-used types.
   typedef std::vector<PageUsageData*> PageUsageDataList;
@@ -261,31 +264,6 @@ class HistoryService : public CancelableRequestProvider,
 
   // Querying ------------------------------------------------------------------
 
-  // Callback class that a client can implement to iterate over URLs. The
-  // callbacks WILL BE CALLED ON THE BACKGROUND THREAD! Your implementation
-  // should handle this appropriately.
-  class URLEnumerator {
-   public:
-    // Indicates that a URL is available. There will be exactly one call for
-    // every URL in history.
-    virtual void OnURL(const history::URLRow& url_row) = 0;
-
-    // Indicates we are done iterating over URLs. Once called, there will be no
-    // more callbacks made. This call is guaranteed to occur, even if there are
-    // no URLs. If all URLs were iterated, success will be true.
-    virtual void OnComplete(bool success) = 0;
-
-   protected:
-    virtual ~URLEnumerator() {}
-  };
-
-  // Enumerate all URLs in history. The given iterator will be owned by the
-  // caller, so the caller should ensure it exists until OnComplete is called.
-  // You should not generally use this since it will be slow to slurp all URLs
-  // in from the database. It is designed for rebuilding the visited link
-  // database from history.
-  void IterateURLs(URLEnumerator* iterator);
-
   // Returns the information about the requested URL. If the URL is found,
   // success will be true and the information will be in the URLRow parameter.
   // On success, the visits, if requested, will be sorted by date. If they have
@@ -679,6 +657,13 @@ class HistoryService : public CancelableRequestProvider,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
+  // Implementation of VisitedLinkDelegate.
+  virtual bool AreEquivalentContexts(
+      content::BrowserContext* context1,
+      content::BrowserContext* context2) OVERRIDE;
+  virtual void RebuildTable(
+      const scoped_refptr<URLEnumerator>& enumerator) OVERRIDE;
+
   // Low-level Init().  Same as the public version, but adds a |no_db| parameter
   // that is only set by unittests which causes the backend to not init its DB.
   bool Init(const FilePath& history_dir,
@@ -1103,6 +1088,10 @@ class HistoryService : public CancelableRequestProvider,
   // The profile, may be null when testing.
   Profile* profile_;
 
+  // Used for propagating link highlighting data across renderers. May be null
+  // in tests.
+  scoped_ptr<VisitedLinkMaster> visitedlink_master_;
+
   // Has the backend finished loading? The backend is loaded once Init has
   // completed.
   bool backend_loaded_;

chrome/browser/history/history_backend.cc

@@ -1058,13 +1058,14 @@ void HistoryBackend::AddPageNoVisitForBookmark(const GURL& url,
   db_->AddURL(url_info);
 }
 
-void HistoryBackend::IterateURLs(HistoryService::URLEnumerator* iterator) {
+void HistoryBackend::IterateURLs(
+    const scoped_refptr<VisitedLinkDelegate::URLEnumerator>& iterator) {
   if (db_.get()) {
     HistoryDatabase::URLEnumerator e;
     if (db_->InitURLEnumeratorForEverything(&e)) {
       URLRow info;
       while (e.GetNextURL(&info)) {
-        iterator->OnURL(info);
+        iterator->OnURL(info.url());
       }
       iterator->OnComplete(true);  // Success.
       return;

chrome/browser/history/history_backend.h

@@ -173,7 +173,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   void ScheduleAutocomplete(HistoryURLProvider* provider,
                             HistoryURLProviderParams* params);
 
-  void IterateURLs(HistoryService::URLEnumerator* enumerator);
+  void IterateURLs(
+      const scoped_refptr<VisitedLinkDelegate::URLEnumerator>& enumerator);
   void QueryURL(scoped_refptr<QueryURLRequest> request,
                 const GURL& url,
                 bool want_visits);

chrome/browser/profiles/profile_dependency_manager.cc

@@ -74,7 +74,6 @@
 #include "chrome/browser/ui/webui/chrome_url_data_manager_factory.h"
 #include "chrome/browser/ui/webui/ntp/ntp_resource_cache_factory.h"
 #include "chrome/browser/user_style_sheet_watcher_factory.h"
-#include "chrome/browser/visitedlink/visitedlink_master_factory.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
 
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
@@ -323,7 +322,6 @@ void ProfileDependencyManager::AssertFactoriesBuilt() {
 #endif
   TokenServiceFactory::GetInstance();
   UserStyleSheetWatcherFactory::GetInstance();
-  VisitedLinkMasterFactory::GetInstance();
   WebDataServiceFactory::GetInstance();
 #if defined(ENABLE_WEB_INTENTS)
   WebIntentsRegistryFactory::GetInstance();

chrome/browser/visitedlink/visitedlink_delegate.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_VISITEDLINK_VISITEDLINK_DELEGATE_H_
+#define CHROME_BROWSER_VISITEDLINK_VISITEDLINK_DELEGATE_H_
+
+#include "base/memory/ref_counted.h"
+
+class GURL;
+
+namespace content {
+class BrowserContext;
+}  // namespace content
+
+// Delegate class that clients of VisitedLinkMaster must implement.
+class VisitedLinkDelegate {
+ public:
+  // Returns true when the two BrowserContexts are equivalent.
+  virtual bool AreEquivalentContexts(content::BrowserContext* context1,
+                                     content::BrowserContext* context2) = 0;
+
+  // See RebuildTable.
+  class URLEnumerator : public base::RefCountedThreadSafe<URLEnumerator> {
+   public:
+    // Call this with each URL to rebuild the table.
+    virtual void OnURL(const GURL& url) = 0;
+
+    // This must be called by Delegate after RebuildTable is called. |success|
+    // indicates all URLs have been returned successfully. The URLEnumerator
+    // object cannot be used by the delegate after this call.
+    virtual void OnComplete(bool success) = 0;
+
+   protected:
+    virtual ~URLEnumerator() {}
+
+   private:
+    friend class base::RefCountedThreadSafe<URLEnumerator>;
+  };
+
+  // Delegate class is responsible for persisting the list of visited URLs
+  // across browser runs. This is called by VisitedLinkMaster to repopulate
+  // its internal table. Note that methods on enumerator can be called on any
+  // thread but the delegate is responsible for synchronizating the calls.
+  virtual void RebuildTable(const scoped_refptr<URLEnumerator>& enumerator) = 0;
+
+ protected:
+
+  virtual ~VisitedLinkDelegate() {}
+};
+
+#endif  // CHROME_BROWSER_VISITEDLINK_VISITEDLINK_DELEGATE_H_

chrome/browser/visitedlink/visitedlink_event_listener.cc

@@ -5,8 +5,7 @@
 #include "chrome/browser/visitedlink/visitedlink_event_listener.h"
 
 #include "base/shared_memory.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/visitedlink/visitedlink_master_factory.h"
+#include "chrome/browser/visitedlink/visitedlink_delegate.h"
 #include "chrome/common/render_messages.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
@@ -108,8 +107,11 @@ class VisitedLinkUpdater {
   VisitedLinkCommon::Fingerprints pending_;
 };
 
-VisitedLinkEventListener::VisitedLinkEventListener(Profile* profile)
-    : profile_(profile) {
+VisitedLinkEventListener::VisitedLinkEventListener(
+    VisitedLinkMaster* master,
+    content::BrowserContext* browser_context)
+    : master_(master),
+      browser_context_(browser_context) {
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
@@ -134,12 +136,8 @@ void VisitedLinkEventListener::NewTable(base::SharedMemory* table_memory) {
         content::RenderProcessHost::FromID(i->first);
     if (!process)
       continue;
-    Profile* profile = Profile::FromBrowserContext(
-        process->GetBrowserContext());
-    VisitedLinkMaster* master =
-        VisitedLinkMasterFactory::GetForProfile(profile);
-    if (master && master->shared_memory() == table_memory)
-      i->second->SendVisitedLinkTable(table_memory);
+
+    i->second->SendVisitedLinkTable(table_memory);
   }
 }
 
@@ -181,22 +179,18 @@ void VisitedLinkEventListener::Observe(
     case content::NOTIFICATION_RENDERER_PROCESS_CREATED: {
       content::RenderProcessHost* process =
           content::Source<content::RenderProcessHost>(source).ptr();
-      Profile* profile =
-          Profile::FromBrowserContext(process->GetBrowserContext());
-      if (!profile_->IsSameProfile(profile))
+      if (!master_->GetDelegate()->AreEquivalentContexts(
+          browser_context_, process->GetBrowserContext()))
         return;
-      updaters_[process->GetID()] =
-          make_linked_ptr(new VisitedLinkUpdater(process->GetID()));
 
-      // Initialize support for visited links. Send the renderer process its
-      // initial set of visited links.
-      VisitedLinkMaster* master =
-        VisitedLinkMasterFactory::GetForProfile(profile);
-      if (!master)
+      // Happens on browser start up.
+      if (!master_->shared_memory())
         return;
 
+      updaters_[process->GetID()] =
+          make_linked_ptr(new VisitedLinkUpdater(process->GetID()));
       updaters_[process->GetID()]->SendVisitedLinkTable(
-          master->shared_memory());
+          master_->shared_memory());
       break;
     }
     case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {

chrome/browser/visitedlink/visitedlink_event_listener.h

@@ -17,17 +17,21 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
-class Profile;
 class VisitedLinkUpdater;
 
 namespace base {
 class SharedMemory;
 }
 
+namespace content {
+class BrowserContext;
+}  // namespace content
+
 class VisitedLinkEventListener : public VisitedLinkMaster::Listener,
                                  public content::NotificationObserver {
  public:
-  explicit VisitedLinkEventListener(Profile* profile);
+  VisitedLinkEventListener(VisitedLinkMaster* master,
+                           content::BrowserContext* browser_context);
   virtual ~VisitedLinkEventListener();
 
   virtual void NewTable(base::SharedMemory* table_memory) OVERRIDE;
@@ -51,7 +55,11 @@ class VisitedLinkEventListener : public VisitedLinkMaster::Listener,
   typedef std::map<int, linked_ptr<VisitedLinkUpdater> > Updaters;
   Updaters updaters_;
 
-  Profile* profile_;
+  VisitedLinkMaster* master_;
+
+  // Used to filter RENDERER_PROCESS_CREATED notifications to renderers that
+  // belong to this BrowserContext.
+  content::BrowserContext* browser_context_;
 
   DISALLOW_COPY_AND_ASSIGN(VisitedLinkEventListener);
 };

chrome/browser/visitedlink/visitedlink_master.cc

@@ -24,11 +24,11 @@
 #include "base/rand_util.h"
 #include "base/string_util.h"
 #include "base/threading/thread_restrictions.h"
-#include "chrome/browser/history/history.h"
-#include "chrome/browser/history/history_service_factory.h"
-#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/visitedlink/visitedlink_delegate.h"
 #include "chrome/browser/visitedlink/visitedlink_event_listener.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "googleurl/src/gurl.h"
 
 using content::BrowserThread;
 using file_util::ScopedFILE;
@@ -138,8 +138,7 @@ void AsyncClose(FILE** file) {
 // which case it notifies the builder via DisownMaster(). The builder will
 // delete itself once rebuilding is complete, and not execute any callback.
 class VisitedLinkMaster::TableBuilder
-    : public HistoryService::URLEnumerator,
-      public base::RefCountedThreadSafe<TableBuilder> {
+    : public VisitedLinkDelegate::URLEnumerator {
  public:
   TableBuilder(VisitedLinkMaster* master,
                const uint8 salt[LINK_SALT_LENGTH]);
@@ -150,14 +149,12 @@ class VisitedLinkMaster::TableBuilder
   // table will be being rebuilt simultaneously on the other thread.
   void DisownMaster();
 
-  // HistoryService::URLEnumerator
-  virtual void OnURL(const history::URLRow& url_row);
+  // VisitedLinkDelegate::URLEnumerator
+  virtual void OnURL(const GURL& url);
   virtual void OnComplete(bool succeed);
 
  private:
-  friend class base::RefCountedThreadSafe<TableBuilder>;
-
-  ~TableBuilder() {}
+  virtual ~TableBuilder() {}
 
   // OnComplete mashals to this function on the main thread to do the
   // notification.
@@ -174,30 +171,34 @@ class VisitedLinkMaster::TableBuilder
 
   // Stores the fingerprints we computed on the background thread.
   VisitedLinkCommon::Fingerprints fingerprints_;
+
+  DISALLOW_COPY_AND_ASSIGN(TableBuilder);
 };
 
 // VisitedLinkMaster ----------------------------------------------------------
 
-VisitedLinkMaster::VisitedLinkMaster(Profile* profile)
-    : profile_(profile) {
-  listener_.reset(new VisitedLinkEventListener(profile));
-  DCHECK(listener_.get());
+VisitedLinkMaster::VisitedLinkMaster(content::BrowserContext* browser_context,
+                                     VisitedLinkDelegate* delegate)
+    : browser_context_(browser_context),
+      delegate_(delegate),
+      listener_(new VisitedLinkEventListener(
+          ALLOW_THIS_IN_INITIALIZER_LIST(this), browser_context)) {
   InitMembers();
 }
 
 VisitedLinkMaster::VisitedLinkMaster(Listener* listener,
-                                     HistoryService* history_service,
+                                     VisitedLinkDelegate* delegate,
                                      bool suppress_rebuild,
                                      const FilePath& filename,
                                      int32 default_table_size)
-    : profile_(NULL) {
+    : browser_context_(NULL),
+      delegate_(delegate) {
   listener_.reset(listener);
   DCHECK(listener_.get());
   InitMembers();
 
   database_name_override_ = filename;
   table_size_override_ = default_table_size;
-  history_service_override_ = history_service;
   suppress_rebuild_ = suppress_rebuild;
 }
 
@@ -220,7 +221,6 @@ void VisitedLinkMaster::InitMembers() {
   shared_memory_serial_ = 0;
   used_items_ = 0;
   table_size_override_ = 0;
-  history_service_override_ = NULL;
   suppress_rebuild_ = false;
   sequence_token_ = BrowserThread::GetBlockingPool()->GetSequenceToken();
 
@@ -241,7 +241,9 @@ bool VisitedLinkMaster::Init() {
 
 VisitedLinkMaster::Hash VisitedLinkMaster::TryToAddURL(const GURL& url) {
   // Extra check that we are not incognito. This should not happen.
-  if (profile_ && profile_->IsOffTheRecord()) {
+  // TODO(boliu): Move this check to HistoryService when IsOffTheRecord is
+  // removed from BrowserContext.
+  if (browser_context_ && browser_context_->IsOffTheRecord()) {
     NOTREACHED();
     return null_hash_;
   }
@@ -320,10 +322,12 @@ void VisitedLinkMaster::DeleteAllURLs() {
   listener_->Reset();
 }
 
-void VisitedLinkMaster::DeleteURLs(const history::URLRows& rows) {
-  typedef std::set<GURL>::const_iterator SetIterator;
+VisitedLinkDelegate* VisitedLinkMaster::GetDelegate() {
+  return delegate_;
+}
 
-  if (rows.empty())
+void VisitedLinkMaster::DeleteURLs(URLIterator* urls) {
+  if (!urls->HasNextURL())
     return;
 
   listener_->Reset();
@@ -331,9 +335,8 @@ void VisitedLinkMaster::DeleteURLs(const history::URLRows& rows) {
   if (table_builder_) {
     // A rebuild is in progress, save this deletion in the temporary list so
     // it can be added once rebuild is complete.
-    for (history::URLRows::const_iterator i = rows.begin(); i != rows.end();
-         ++i) {
-      const GURL& url(i->url());
+    while (urls->HasNextURL()) {
+      const GURL& url(urls->NextURL());
       if (!url.is_valid())
         continue;
 
@@ -357,9 +360,8 @@ void VisitedLinkMaster::DeleteURLs(const history::URLRows& rows) {
 
   // Compute the deleted URLs' fingerprints and delete them
   std::set<Fingerprint> deleted_fingerprints;
-  for (history::URLRows::const_iterator i = rows.begin(); i != rows.end();
-       ++i) {
-    const GURL& url(i->url());
+  while (urls->HasNextURL()) {
+    const GURL& url(urls->NextURL());
     if (!url.is_valid())
       continue;
     deleted_fingerprints.insert(
@@ -581,7 +583,7 @@ bool VisitedLinkMaster::InitFromScratch(bool suppress_rebuild) {
   // to disk. We don't want to save explicitly here, since the rebuild may
   // not complete, leaving us with an empty but valid visited link database.
   // In the future, we won't know we need to try rebuilding again.
-  return RebuildTableFromHistory();
+  return RebuildTableFromDelegate();
 }
 
 bool VisitedLinkMaster::ReadFileHeader(FILE* file,
@@ -641,10 +643,10 @@ bool VisitedLinkMaster::GetDatabaseFileName(FilePath* filename) {
     return true;
   }
 
-  if (!profile_ || profile_->GetPath().empty())
+  if (!browser_context_ || browser_context_->GetPath().empty())
     return false;
 
-  FilePath profile_dir = profile_->GetPath();
+  FilePath profile_dir = browser_context_->GetPath();
   *filename = profile_dir.Append(FILE_PATH_LITERAL("Visited Links"));
   return true;
 }
@@ -810,31 +812,12 @@ uint32 VisitedLinkMaster::NewTableSizeForCount(int32 item_count) const {
 }
 
 // See the TableBuilder definition in the header file for how this works.
-bool VisitedLinkMaster::RebuildTableFromHistory() {
+bool VisitedLinkMaster::RebuildTableFromDelegate() {
   DCHECK(!table_builder_);
-  if (table_builder_)
-    return false;
-
-  HistoryService* history_service = history_service_override_;
-  if (!history_service && profile_) {
-    history_service =
-        HistoryServiceFactory::GetForProfile(profile_,
-                                             Profile::EXPLICIT_ACCESS);
-  }
-
-  if (!history_service) {
-    DLOG(WARNING) << "Attempted to rebuild visited link table, but couldn't "
-                     "obtain a HistoryService.";
-    return false;
-  }
 
   // TODO(brettw) make sure we have reasonable salt!
   table_builder_ = new TableBuilder(this, salt_);
-
-  // Make sure the table builder stays live during the call, even if the
-  // master is deleted. This is balanced in TableBuilder::OnCompleteMainThread.
-  table_builder_->AddRef();
-  history_service->IterateURLs(table_builder_);
+  delegate_->RebuildTable(table_builder_);
   return true;
 }
 
@@ -958,8 +941,7 @@ void VisitedLinkMaster::TableBuilder::DisownMaster() {
   master_ = NULL;
 }
 
-void VisitedLinkMaster::TableBuilder::OnURL(const history::URLRow& url_row) {
-  const GURL& url(url_row.url());
+void VisitedLinkMaster::TableBuilder::OnURL(const GURL& url) {
   if (!url.is_empty()) {
     fingerprints_.push_back(VisitedLinkMaster::ComputeURLFingerprint(
         url.spec().data(), url.spec().length(), salt_));
@@ -980,8 +962,4 @@ void VisitedLinkMaster::TableBuilder::OnComplete(bool success) {
 void VisitedLinkMaster::TableBuilder::OnCompleteMainThread() {
   if (master_)
     master_->OnTableRebuildComplete(success_, fingerprints_);
-
-  // WILL (generally) DELETE THIS! This balances the AddRef in
-  // VisitedLinkMaster::RebuildTableFromHistory.
-  Release();
 }

chrome/browser/visitedlink/visitedlink_master.h

@@ -11,19 +11,25 @@
 #include <set>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/callback_forward.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
-#include "base/memory/ref_counted.h"
 #include "base/shared_memory.h"
 #include "base/threading/sequenced_worker_pool.h"
-#include "chrome/browser/history/history.h"
-#include "chrome/browser/history/history_types.h"
-#include "chrome/browser/profiles/profile_keyed_service.h"
 #include "chrome/common/visitedlink_common.h"
 
+#if defined(UNIT_TEST) || defined(PERF_TEST) || !defined(NDEBUG)
+#include "base/logging.h"
+#endif
+
 class GURL;
-class Profile;
+class VisitedLinkDelegate;
+
+namespace content {
+class BrowserContext;
+}  // namespace content
+
 
 // Controls the link coloring database. The master controls all writing to the
 // database as well as disk I/O. There should be only one master.
@@ -31,8 +37,7 @@ class Profile;
 // This class will defer writing operations to the file thread. This means that
 // class destruction, the file may still be open since operations are pending on
 // another thread.
-class VisitedLinkMaster : public VisitedLinkCommon,
-                          public ProfileKeyedService  {
+class VisitedLinkMaster : public VisitedLinkCommon {
  public:
   // Listens to the link coloring database events. The master is given this
   // event as a constructor argument and dispatches events using it.
@@ -53,7 +58,8 @@ class VisitedLinkMaster : public VisitedLinkCommon,
     virtual void Reset() = 0;
   };
 
-  explicit VisitedLinkMaster(Profile* profile);
+  VisitedLinkMaster(content::BrowserContext* browser_context,
+                    VisitedLinkDelegate* delegate);
 
   // In unit test mode, we allow the caller to optionally specify the database
   // filename so that it can be run from a unit test. The directory where this
@@ -71,15 +77,12 @@ class VisitedLinkMaster : public VisitedLinkCommon,
   // history if the file can't be loaded. This should generally be set for
   // testing except when you want to test the rebuild process explicitly.
   VisitedLinkMaster(Listener* listener,
-                    HistoryService* history_service,
+                    VisitedLinkDelegate* delegate,
                     bool suppress_rebuild,
                     const FilePath& filename,
                     int32 default_table_size);
   virtual ~VisitedLinkMaster();
 
-  // Return the VisitedLinkMaster instance for a profile.
-  static VisitedLinkMaster* FromProfile(Profile* profile);
-
   // Must be called immediately after object creation. Nothing else will work
   // until this is called. Returns true on success, false means that this
   // object won't work.
@@ -93,13 +96,31 @@ class VisitedLinkMaster : public VisitedLinkCommon,
   // Adds a set of URLs to the table.
   void AddURLs(const std::vector<GURL>& url);
 
+  // See DeleteURLs.
+  class URLIterator {
+   public:
+    // HasNextURL must return true when this is called. Returns the next URL
+    // then advances the iterator. Note that the returned reference is only
+    // valid until the next call of NextURL.
+    virtual const GURL& NextURL() = 0;
+
+    // Returns true if still has URLs to be iterated.
+    virtual bool HasNextURL() const = 0;
+
+   protected:
+    virtual ~URLIterator() {}
+  };
+
   // Deletes the specified URLs from |rows| from the table.
-  void DeleteURLs(const history::URLRows& rows);
+  void DeleteURLs(URLIterator* iterator);
 
   // Clears the visited links table by deleting the file from disk. Used as
   // part of history clearing.
   void DeleteAllURLs();
 
+  // Returns the Delegate of this Master.
+  VisitedLinkDelegate* GetDelegate();
+
 #if defined(UNIT_TEST) || !defined(NDEBUG) || defined(PERF_TEST)
   // This is a debugging function that can be called to double-check internal
   // data structures. It will assert if the check fails.
@@ -289,7 +310,7 @@ class VisitedLinkMaster : public VisitedLinkCommon,
   //
   // Returns true on success. Failure means we're not attempting to rebuild
   // the database because something failed.
-  bool RebuildTableFromHistory();
+  bool RebuildTableFromDelegate();
 
   // Callback that the table rebuilder uses when the rebuild is complete.
   // |success| is true if the fingerprint generation succeeded, in which case
@@ -320,9 +341,13 @@ class VisitedLinkMaster : public VisitedLinkCommon,
   bool posted_asynchronous_operation_;
 #endif
 
-  // Reference to the user profile that this object belongs to
+  // Reference to the browser context that this object belongs to
   // (it knows the path to where the data is stored)
-  Profile* profile_;
+  content::BrowserContext* browser_context_;
+
+  // Client owns the delegate and is responsible for it being valid through
+  // the life time this VisitedLinkMaster.
+  VisitedLinkDelegate* delegate_;
 
   // VisitedLinkEventListener to handle incoming events.
   scoped_ptr<Listener> listener_;
@@ -380,10 +405,6 @@ class VisitedLinkMaster : public VisitedLinkCommon,
   // When nonzero, overrides the table size for new databases for testing
   int32 table_size_override_;
 
-  // When non-NULL, overrides the history service to use rather than as the
-  // BrowserProcess. This is provided for unit tests.
-  HistoryService* history_service_override_;
-
   // When set, indicates the task that should be run after the next rebuild from
   // history is complete.
   base::Closure rebuild_complete_task_;

chrome/browser/visitedlink/visitedlink_master_factory.cc

-// 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.
-
-#include "chrome/browser/visitedlink/visitedlink_master_factory.h"
-
-#include "base/memory/singleton.h"
-#include "chrome/browser/profiles/profile_dependency_manager.h"
-#include "chrome/browser/visitedlink/visitedlink_master.h"
-
-// static
-VisitedLinkMaster* VisitedLinkMaster::FromProfile(Profile* profile) {
-  return VisitedLinkMasterFactory::GetForProfile(profile);
-}
-
-// static
-VisitedLinkMaster* VisitedLinkMasterFactory::GetForProfile(Profile* profile) {
-  return static_cast<VisitedLinkMaster*>(
-      GetInstance()->GetServiceForProfile(profile, true));
-}
-
-// static
-VisitedLinkMasterFactory* VisitedLinkMasterFactory::GetInstance() {
-  return Singleton<VisitedLinkMasterFactory>::get();
-}
-
-VisitedLinkMasterFactory::VisitedLinkMasterFactory()
-    : ProfileKeyedServiceFactory(
-          "VisitedLinkMaster", ProfileDependencyManager::GetInstance()) {
-}
-
-VisitedLinkMasterFactory::~VisitedLinkMasterFactory() {
-}
-
-ProfileKeyedService*
-VisitedLinkMasterFactory::BuildServiceInstanceFor(Profile* profile) const {
-  VisitedLinkMaster* visitedlink_master(new VisitedLinkMaster(profile));
-  if (visitedlink_master->Init())
-    return visitedlink_master;
-  return NULL;
-}
-
-bool VisitedLinkMasterFactory::ServiceIsNULLWhileTesting() const {
-  return true;
-}

chrome/browser/visitedlink/visitedlink_master_factory.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_VISITEDLINK_VISITEDLINK_MASTER_FACTORY_H_
-#define CHROME_BROWSER_VISITEDLINK_VISITEDLINK_MASTER_FACTORY_H_
-
-#include "base/memory/singleton.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/profiles/profile_keyed_service_factory.h"
-
-template <typename T> struct DefaultSingletonTraits;
-
-class VisitedLinkMaster;
-
-// Singleton that owns all VisitedLinkMaster and associates them with
-// Profiles.
-class VisitedLinkMasterFactory : public ProfileKeyedServiceFactory {
- public:
-  static VisitedLinkMaster* GetForProfile(Profile* profile);
-
-  static VisitedLinkMasterFactory* GetInstance();
-
- private:
-  friend struct DefaultSingletonTraits<VisitedLinkMasterFactory>;
-
-  VisitedLinkMasterFactory();
-  virtual ~VisitedLinkMasterFactory();
-
-  // ProfileKeyedServiceFactory:
-  virtual ProfileKeyedService* BuildServiceInstanceFor(
-      Profile* profile) const OVERRIDE;
-  virtual bool ServiceIsNULLWhileTesting() const OVERRIDE;
-
-  DISALLOW_COPY_AND_ASSIGN(VisitedLinkMasterFactory);
-};
-
-#endif  // CHROME_BROWSER_VISITEDLINK_VISITEDLINK_MASTER_FACTORY_H_

chrome/browser/visitedlink/visitedlink_perftest.cc

@@ -13,6 +13,7 @@
 #include "base/stringprintf.h"
 #include "base/test/test_file_util.h"
 #include "chrome/browser/visitedlink/visitedlink_master.h"
+#include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::TimeDelta;

chrome/chrome_browser.gypi

@@ -2165,12 +2165,11 @@
         'browser/value_store/value_store.h',
         'browser/view_type_utils.cc',
         'browser/view_type_utils.h',
+        'browser/visitedlink/visitedlink_delegate.h',
         'browser/visitedlink/visitedlink_event_listener.cc',
         'browser/visitedlink/visitedlink_event_listener.h',
         'browser/visitedlink/visitedlink_master.cc',
         'browser/visitedlink/visitedlink_master.h',
-        'browser/visitedlink/visitedlink_master_factory.cc',
-        'browser/visitedlink/visitedlink_master_factory.h',
         'browser/web_applications/web_app.cc',
         'browser/web_applications/web_app.h',
         'browser/web_applications/web_app_android.cc',