Introduce BookmarkClient interface to abstract embedder

chrome/browser/bookmarks/bookmark_model.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_BOOKMARKS_BOOKMARK_MODEL_H_
 #define CHROME_BROWSER_BOOKMARKS_BOOKMARK_MODEL_H_
 
 #include <map>
 #include <set>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
 #include "base/synchronization/lock.h"
 #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"
 #include "ui/gfx/image/image.h"
 #include "url/gurl.h"
 
 class BookmarkExpandedStateTracker;
 class BookmarkIndex;
 class BookmarkLoadDetails;
 class BookmarkModelObserver;
 class BookmarkStorage;
 struct BookmarkTitleMatch;
 class Profile;
 class ScopedGroupBookmarkActions;
 
 namespace base {
 class SequencedTaskRunner;
 }
 
-namespace chrome {
-struct FaviconImageResult;
-}
-
 // BookmarkModel --------------------------------------------------------------
 
 // BookmarkModel provides a directed acyclic graph of URLs and folders.
 // Three graphs are provided for the three entry points: those on the 'bookmarks
 // bar', those in the 'other bookmarks' folder and those in the 'mobile' folder.
 //
 // An observer may be attached to observe relevant events.
 //
 // 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_; }
 
   // Returns the root node. The 'bookmark bar' node and 'other' node are
   // children of the root node.
   const BookmarkNode* root_node() { return &root_; }
 
   // Returns the 'bookmark bar' node. This is NULL until loaded.
   const BookmarkNode* bookmark_bar_node() { return bookmark_bar_node_; }
@@ skipping 314 matching lines @@
 
   // See description of IsDoingExtensiveChanges above.
   int extensive_changes_;
 
   scoped_ptr<BookmarkExpandedStateTracker> expanded_state_tracker_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModel);
 };
 
 #endif  // CHROME_BROWSER_BOOKMARKS_BOOKMARK_MODEL_H_