Remove VisitedLink dependency on rest of chrome

chrome/browser/visitedlink/visitedlink_master.h

Index: chrome/browser/visitedlink/visitedlink_master.h
diff --git a/chrome/browser/visitedlink/visitedlink_master.h b/chrome/browser/visitedlink/visitedlink_master.h
index 039ca317f643bb5c46f94f2c030197eb5d64e292..b0aa6fc7b2003f834f0fad6d11f9ed6cc3316019 100644
--- a/chrome/browser/visitedlink/visitedlink_master.h
+++ b/chrome/browser/visitedlink/visitedlink_master.h
@@ -11,19 +11,26 @@
 #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 +38,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 +59,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 +78,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 +97,30 @@ 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.
+    virtual const GURL& NextURL() = 0;

[joth, 2012/12/29 18:32:06]

suggest returning GURL by value rather than const ref (esp on abstract methods)
-- avoids weird lifetime issues in the implementation. 

[boliu, 2012/12/30 02:08:15]

Done. But client code is still using references.

[joth, 2012/12/30 21:51:07]

OK... I think there's special magic in the language to keep the temporary return
value alive as long as the reference when you do this.  but my own preference
would be to just copy construct an instance. (RVO means there's probably no
difference in performance) 

[boliu, 2013/01/02 17:09:36]

Done.

+
+    // 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);

[joth, 2012/12/29 18:32:06]

the abstract iterator is OK but a little confusing to keep in mind along side
the similar delegate URLEnumerator class. presume they can't be combined?

[boliu, 2012/12/30 02:08:15]

I am going to argue no. Only after this CL did I learn the difference between
Iterator and Enumerator. Basically Iterator is created by the delegate and
passed into VisitedLink; Enumerator is created by VisitedLink and passed into
the delegate. (They also do slightly different things). So I think it'll be more
confusing if they are combined into the same interface (unless it lives
somewhere else that both delegate and VisitedLink depends on, like base)

[joth, 2012/12/30 21:51:07]

Right - URLEnumerator is actually visitor pattern. So it comes down to whether
you want push (visitor) or pull (iterator) semantics on your algorithm.
Looking at HistoryBackend::IteratorURLs, that would be hard to change to "pull",
so yeah looks like we need to keep both concepts.

(somewhat bugs me that  AddURLs takes a vector but DeleteURLs  takes an abstract
iterator, guess if needed we just change AddURLs to use the abstract iterator
too)

[boliu, 2013/01/02 17:09:36]

One more thing to do *after* refactoring and componentization.

 
   // 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_;