Introduce BookmarkClient interface to abstract embedder

chrome/browser/bookmarks/bookmark_model.h

Index: chrome/browser/bookmarks/bookmark_model.h
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 70ca10aa8bc690757f9006207985ccea9435bc64..43467dcf1445694794798b2a97ac08796fba7c54 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -19,8 +19,8 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/bookmarks/bookmark_service.h"
+#include "components/bookmarks/core/browser/bookmark_client.h"
 #include "components/bookmarks/core/browser/bookmark_node.h"
-#include "components/favicon_base/favicon_types.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -40,10 +40,6 @@ namespace base {
 class SequencedTaskRunner;
 }
 
-namespace chrome {
-struct FaviconImageResult;
-}
-
 // BookmarkModel --------------------------------------------------------------
 
 // BookmarkModel provides a directed acyclic graph of URLs and folders.
@@ -55,19 +51,35 @@ struct FaviconImageResult;
 // You should NOT directly create a BookmarkModel, instead go through the
 // BookmarkModelFactory.
 class BookmarkModel : public content::NotificationObserver,
+                      public BookmarkClient,

[sky, 2014/04/18 17:05:04]

I'm confused as to why you're making BookmarkModel implement BookmarkClient? If
only temporary, I would prefer having it on a separate object that BookmarkModel
owns. I like this since you internally have BookmarkModel::LoadFavicon calling
GetFaviconImageForURL(), which should really by going through BookmarkClient,
and not BookmarkModel.

I'm fine with this code existing in bookmark_model.h/.cc for now, but would
prefer we start down the long term path that we want to get to. Unless I'm
missing something.

[sdefresne, 2014/04/18 22:25:49]

My plan was to have this inheritance temporarily, and then to split the class in
a second step as I feared that having the client a separate class would make the
CL extra large and I prefer working with small CL that do as few modification as
possible.

I've done extracted the ChromeBookmarkClient now, and as I feared this turned
the CL into a monster CL. I think it is going to be a pain to submit through the
CQ, but after that, the last steps of the componentization should be relatively
painless.

                       public BookmarkService,
                       public KeyedService {
  public:
   explicit BookmarkModel(Profile* profile);
   virtual ~BookmarkModel();
 
+  virtual base::CancelableTaskTracker::TaskId GetFaviconImageForURL(
+      const GURL& page_url,
+      int icon_types,
+      int desired_size_in_dip,
+      const FaviconImageCallback& callback,
+      base::CancelableTaskTracker* tracker) OVERRIDE;
+
+  virtual void GetTypedCountForNodes(
+      const NodeSet& nodes,
+      NodeTypedCountPairs* node_typed_count_pairs) OVERRIDE;
+
+  virtual void RecordAction(const base::UserMetricsAction& action) OVERRIDE;
+
   // Invoked prior to destruction to release any necessary resources.
   virtual void Shutdown() OVERRIDE;
 
   // Loads the bookmarks. This is called upon creation of the
   // BookmarkModel. You need not invoke this directly.
-  // All load operations will be executed on |task_runner|.
-  void Load(const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+  // All load operations will be executed on |io_task_runner|, and callback
+  // will be called from |io_task_runner|.
+  void Load(const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
+            const scoped_refptr<base::SequencedTaskRunner>& ui_task_runner);
 
   // Returns true if the model finished loading.
   bool loaded() const { return loaded_; }