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..3680aef7cfe588fd3b855e3f7cde0e130ac1199a 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"
@@ -33,17 +33,15 @@ class BookmarkLoadDetails;
 class BookmarkModelObserver;
 class BookmarkStorage;
 struct BookmarkTitleMatch;
+class PrefService;
 class Profile;
 class ScopedGroupBookmarkActions;
 
 namespace base {
+class FilePath;
 class SequencedTaskRunner;
 }
 
-namespace chrome {
-struct FaviconImageResult;
-}
-
 // BookmarkModel --------------------------------------------------------------
 
 // BookmarkModel provides a directed acyclic graph of URLs and folders.
@@ -54,20 +52,22 @@ struct FaviconImageResult;
 //
 // You should NOT directly create a BookmarkModel, instead go through the
 // BookmarkModelFactory.
-class BookmarkModel : public content::NotificationObserver,
-                      public BookmarkService,
-                      public KeyedService {
+class BookmarkModel : public BookmarkService {
  public:
-  explicit BookmarkModel(Profile* profile);
+  explicit BookmarkModel(BookmarkClient* client);
   virtual ~BookmarkModel();
 
   // Invoked prior to destruction to release any necessary resources.
-  virtual void Shutdown() OVERRIDE;
+  void Shutdown();
 
   // 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(PrefService* pref_service,
+            const base::FilePath& profile_path,
+            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_; }
@@ -248,8 +248,10 @@ class BookmarkModel : public content::NotificationObserver,
   void SetNodeSyncTransactionVersion(const BookmarkNode* node,
                                      int64 sync_transaction_version);
 
-  // Returns the profile that corresponds to this BookmarkModel.
-  Profile* profile() { return profile_; }
+  // Returns the BookmarkClient that corresponds to this BookmarkModel.
+  BookmarkClient* client() { return client_; }
+
+  void OnFaviconChanged(const std::set<GURL>& urls);
 
  private:
   friend class BookmarkCodecTest;
@@ -296,10 +298,6 @@ class BookmarkModel : public content::NotificationObserver,
   // Remove |node| from |nodes_ordered_by_url_set_|.
   void RemoveNodeFromURLSet(BookmarkNode* node);
 
-  // Notifies the history backend about urls of removed bookmarks.
-  void NotifyHistoryAboutRemovedBookmarks(
-      const std::set<GURL>& removed_bookmark_urls) const;
-
   // Adds the |node| at |parent| in the specified |index| and notifies its
   // observers.
   BookmarkNode* AddNode(BookmarkNode* parent,
@@ -337,11 +335,6 @@ class BookmarkModel : public content::NotificationObserver,
   void BeginGroupedChanges();
   void EndGroupedChanges();
 
-  // content::NotificationObserver:
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
-
   // Generates and returns the next node ID.
   int64 generate_next_node_id();
 
@@ -354,9 +347,7 @@ class BookmarkModel : public content::NotificationObserver,
   // delete the returned object.
   BookmarkLoadDetails* CreateLoadDetails();
 
-  content::NotificationRegistrar registrar_;
-
-  Profile* profile_;
+  BookmarkClient* client_;
 
   // Whether the initial set of data has been loaded.
   bool loaded_;
@@ -401,4 +392,58 @@ class BookmarkModel : public content::NotificationObserver,
   DISALLOW_COPY_AND_ASSIGN(BookmarkModel);
 };
 
+class ChromeBookmarkClient : public content::NotificationObserver,

[tfarina, 2014/04/22 17:22:34]

No, don't do that. Instead move this class declaration into
chrome_bookmark_client.h please.

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

Done.

+                             public BookmarkClient,

[tfarina, 2014/04/22 17:22:34]

Can you make BookmarkClient be the first in the inheritance list? I.e., swap its
place with NotificationObserver. I'd also put NotificationObserver as the third.

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

Done.

+                             public KeyedService {
+ public:
+  ChromeBookmarkClient(Profile* profile);

[tfarina, 2014/04/22 17:22:34]

explicit

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

Done.

+  virtual ~ChromeBookmarkClient();
+
+  // Returns the BookmarkModel that corresponds to this ChromeBookmarkClient.
+  BookmarkModel* model() { return model_.get(); }
+
+  // Returns the Profile that corresponds to this ChromeBookmarkClient.
+  Profile* profile() { return profile_; }
+
+  // KeyedService:
+  virtual void Shutdown() OVERRIDE;

[tfarina, 2014/04/22 17:22:34]

Please, make sure the overrides follow the same sequence of the inherited
classes.

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

Done.

+
+  // content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // BookmarkClient:
+  virtual void OnLoad() OVERRIDE;
+

[tfarina, 2014/04/22 17:22:34]

Do not put blank lines between these overrides. We usually group methods
overridden from the same class together.

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

Done.

+  virtual void OnClearStore() OVERRIDE;
+
+  virtual void NotifyHistoryAboutRemovedBookmarks(
+      const std::set<GURL>& removed_bookmarks_urls) const OVERRIDE;
+
+  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 bool SupportsTypedCountForNodes() OVERRIDE;
+
+  virtual void GetTypedCountForNodes(
+      const NodeSet& nodes,
+      NodeTypedCountPairs* node_typed_count_pairs) OVERRIDE;
+
+  virtual void RecordAction(const base::UserMetricsAction& action) OVERRIDE;
+
+ private:
+  Profile* profile_;
+
+  content::NotificationRegistrar registrar_;
+
+  scoped_ptr<BookmarkModel> model_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeBookmarkClient);
+};
+
 #endif  // CHROME_BROWSER_BOOKMARKS_BOOKMARK_MODEL_H_