bookmarks: moves creation of various state to BookmarkLoadDetails

In particular the permenent node creation moves to
BookmarkLoadDetails, and LoadBookmarks() becomes a bare function.

BUG=680698
TEST=covered by existing tests

Change-Id: Ifdc7ef84ee73ac7b59c122976a37cb85d5d27b4f
Reviewed-on: https://chromium-review.googlesource.com/1041575
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555859}

components/bookmarks/browser/bookmark_model.h

@@ -16,6 +16,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
 #include "base/synchronization/lock.h"
@@ -76,10 +77,9 @@ class BookmarkModel : public BookmarkUndoProvider,
   // KeyedService:
   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 |io_task_runner| and the completion
-  // callback will be called from |ui_task_runner|.
+  // Loads the bookmarks. This is called upon creation of the BookmarkModel. You
+  // need not invoke this directly. All load operations will be executed on
+  // |io_task_runner|. |ui_task_runner| is the task runner the model runs on.
   void Load(PrefService* pref_service,
             const base::FilePath& profile_path,
             const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
@@ -90,7 +90,7 @@ class BookmarkModel : public BookmarkUndoProvider,
 
   // Returns the root node. The 'bookmark bar' node and 'other' node are
   // children of the root node.
-  const BookmarkNode* root_node() const { return &root_; }
+  const BookmarkNode* root_node() const { return root_.get(); }
 
   // Returns the 'bookmark bar' node. This is NULL until loaded.
   const BookmarkNode* bookmark_bar_node() const { return bookmark_bar_node_; }
@@ -101,13 +101,15 @@ class BookmarkModel : public BookmarkUndoProvider,
   // Returns the 'mobile' node. This is NULL until loaded.
   const BookmarkNode* mobile_node() const { return mobile_node_; }
 
-  bool is_root_node(const BookmarkNode* node) const { return node == &root_; }
+  bool is_root_node(const BookmarkNode* node) const {
+    return node == root_.get();
+  }
 
   // Returns whether the given |node| is one of the permanent nodes - root node,
   // 'bookmark bar' node, 'other' node or 'mobile' node, or one of the root
   // nodes supplied by the |client_|.
   bool is_permanent_node(const BookmarkNode* node) const {
-    return node && (node == &root_ || node->parent() == &root_);
+    return node && (node == root_.get() || node->parent() == root_.get());
   }
 
   void AddObserver(BookmarkModelObserver* observer);
@@ -346,8 +348,7 @@ class BookmarkModel : public BookmarkUndoProvider,
   // This does NOT delete the node.
   void RemoveNode(BookmarkNode* node, std::set<GURL>* removed_urls);
 
-  // Invoked when loading is finished. Sets |loaded_| and notifies observers.
-  // BookmarkModel takes ownership of |details|.
+  // Called when done loading. Updates internal state and notifies observers.
   void DoneLoading(std::unique_ptr<BookmarkLoadDetails> details);
 
   // Populates |nodes_ordered_by_url_set_| from root.
@@ -380,10 +381,6 @@ class BookmarkModel : public BookmarkUndoProvider,
   // Returns true if the parent and index are valid.
   bool IsValidIndex(const BookmarkNode* parent, int index, bool allow_end);
 
-  // Creates one of the possible permanent nodes (bookmark bar node, other node
-  // and mobile node) from |type|.
-  BookmarkPermanentNode* CreatePermanentNode(BookmarkNode::Type type);
-
   // Notification that a favicon has finished loading. If we can decode the
   // favicon, FaviconLoaded is invoked.
   void OnFaviconDataAvailable(
@@ -414,27 +411,23 @@ class BookmarkModel : public BookmarkUndoProvider,
   // decoding since during decoding codec assigns node IDs.
   void set_next_node_id(int64_t id) { next_node_id_ = id; }
 
-  // Creates and returns a new BookmarkLoadDetails. It's up to the caller to
-  // delete the returned object.
-  std::unique_ptr<BookmarkLoadDetails> CreateLoadDetails();
-
   BookmarkUndoDelegate* undo_delegate() const;
 
   std::unique_ptr<BookmarkClient> client_;
 
   // Whether the initial set of data has been loaded.
-  bool loaded_;
+  bool loaded_ = false;
 
   // The root node. This contains the bookmark bar node, the 'other' node and
   // the mobile node as children.
-  BookmarkNode root_;
+  std::unique_ptr<BookmarkNode> root_;
 
-  BookmarkPermanentNode* bookmark_bar_node_;
-  BookmarkPermanentNode* other_node_;
-  BookmarkPermanentNode* mobile_node_;
+  BookmarkPermanentNode* bookmark_bar_node_ = nullptr;
+  BookmarkPermanentNode* other_node_ = nullptr;
+  BookmarkPermanentNode* mobile_node_ = nullptr;
 
   // The maximum ID assigned to the bookmark nodes in the model.
-  int64_t next_node_id_;
+  int64_t next_node_id_ = 1;
 
   // The observers.
   base::ObserverList<BookmarkModelObserver> observers_;
@@ -458,15 +451,17 @@ class BookmarkModel : public BookmarkUndoProvider,
   base::WaitableEvent loaded_signal_;
 
   // See description of IsDoingExtensiveChanges above.
-  int extensive_changes_;
+  int extensive_changes_ = 0;
 
   std::unique_ptr<BookmarkExpandedStateTracker> expanded_state_tracker_;
 
   std::set<std::string> non_cloned_keys_;
 
-  BookmarkUndoDelegate* undo_delegate_;
+  BookmarkUndoDelegate* undo_delegate_ = nullptr;
   std::unique_ptr<BookmarkUndoDelegate> empty_undo_delegate_;
 
+  base::WeakPtrFactory<BookmarkModel> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkModel);
 };
 

components/bookmarks/browser/bookmark_node.cc

@@ -26,19 +26,15 @@ const base::char16 kInvalidChars[] = {
 
 // BookmarkNode ---------------------------------------------------------------
 
+// static
 const int64_t BookmarkNode::kInvalidSyncTransactionVersion = -1;
 
-BookmarkNode::BookmarkNode(const GURL& url)
-    : url_(url) {
-  Initialize(0);
-}
+BookmarkNode::BookmarkNode(const GURL& url) : BookmarkNode(0, url, false) {}
 
-BookmarkNode::BookmarkNode(int64_t id, const GURL& url) : url_(url) {
-  Initialize(id);
-}
+BookmarkNode::BookmarkNode(int64_t id, const GURL& url)
+    : BookmarkNode(id, url, false) {}
 
-BookmarkNode::~BookmarkNode() {
-}
+BookmarkNode::~BookmarkNode() = default;
 
 void BookmarkNode::SetTitle(const base::string16& title) {
   // Replace newlines and other problematic whitespace characters in
@@ -111,16 +107,13 @@ const GURL& BookmarkNode::GetTitledUrlNodeUrl() const {
   return url_;
 }
 
-void BookmarkNode::Initialize(int64_t id) {
-  id_ = id;
-  type_ = url_.is_empty() ? FOLDER : URL;
-  date_added_ = base::Time::Now();
-  favicon_type_ = favicon_base::IconType::kInvalid;
-  favicon_state_ = INVALID_FAVICON;
-  favicon_load_task_id_ = base::CancelableTaskTracker::kBadTaskId;
-  meta_info_map_.reset();
-  sync_transaction_version_ = kInvalidSyncTransactionVersion;
-}
+BookmarkNode::BookmarkNode(int64_t id, const GURL& url, bool is_permanent_node)
+    : id_(id),
+      url_(url),
+      type_(url_.is_empty() ? FOLDER : URL),
+      date_added_(base::Time::Now()),
+      favicon_type_(favicon_base::IconType::kInvalid),
+      is_permanent_node_(is_permanent_node) {}
 
 void BookmarkNode::InvalidateFavicon() {
   icon_url_.reset();
@@ -132,10 +125,9 @@ void BookmarkNode::InvalidateFavicon() {
 // BookmarkPermanentNode -------------------------------------------------------
 
 BookmarkPermanentNode::BookmarkPermanentNode(int64_t id)
-    : BookmarkNode(id, GURL()), visible_(true) {}
+    : BookmarkNode(id, GURL(), true) {}
 
-BookmarkPermanentNode::~BookmarkPermanentNode() {
-}
+BookmarkPermanentNode::~BookmarkPermanentNode() = default;
 
 bool BookmarkPermanentNode::IsVisible() const {
   return visible_ || !empty();

components/bookmarks/browser/bookmark_node.h

@@ -53,6 +53,10 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode>, public TitledUrlNode {
 
   ~BookmarkNode() override;
 
+  // Returns true if the node is a BookmarkPermanentNode (which does not include
+  // the root).
+  bool is_permanent_node() const { return is_permanent_node_; }
+
   // Set the node's internal title. Note that this neither invokes observers
   // nor updates any bookmark model this node may be in. For that functionality,
   // BookmarkModel::SetTitle(..) should be used instead.
@@ -123,12 +127,12 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode>, public TitledUrlNode {
   // TODO(sky): Consider adding last visit time here, it'll greatly simplify
   // HistoryContentsProvider.
 
+ protected:
+  BookmarkNode(int64_t id, const GURL& url, bool is_permanent_node);
+
  private:
   friend class BookmarkModel;
 
-  // A helper function to initialize various fields during construction.
-  void Initialize(int64_t id);
-
   // Called when the favicon becomes invalid.
   void InvalidateFavicon();
 
@@ -182,18 +186,21 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode>, public TitledUrlNode {
   std::unique_ptr<GURL> icon_url_;
 
   // The loading state of the favicon.
-  FaviconState favicon_state_;
+  FaviconState favicon_state_ = INVALID_FAVICON;
 
   // If not base::CancelableTaskTracker::kBadTaskId, it indicates
   // we're loading the
   // favicon and the task is tracked by CancelabelTaskTracker.
-  base::CancelableTaskTracker::TaskId favicon_load_task_id_;
+  base::CancelableTaskTracker::TaskId favicon_load_task_id_ =
+      base::CancelableTaskTracker::kBadTaskId;
 
   // A map that stores arbitrary meta information about the node.
   std::unique_ptr<MetaInfoMap> meta_info_map_;
 
-  // The sync transaction version. Defaults to kInvalidSyncTransactionVersion.
-  int64_t sync_transaction_version_;
+  // The sync transaction version.
+  int64_t sync_transaction_version_ = kInvalidSyncTransactionVersion;
+
+  const bool is_permanent_node_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkNode);
 };
@@ -213,7 +220,7 @@ class BookmarkPermanentNode : public BookmarkNode {
   bool IsVisible() const override;
 
  private:
-  bool visible_;
+  bool visible_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkPermanentNode);
 };

components/bookmarks/browser/bookmark_storage.cc

@@ -16,11 +16,15 @@
 #include "base/json/json_string_value_serializer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_codec.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/browser/titled_url_index.h"
 #include "components/bookmarks/common/bookmark_constants.h"
+#include "components/strings/grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
 
 using base::TimeTicks;
 
@@ -51,10 +55,10 @@ void AddBookmarksToIndex(BookmarkLoadDetails* details,
   }
 }
 
-void LoadCallback(const base::FilePath& path,
-                  const base::WeakPtr<BookmarkStorage>& storage,
-                  std::unique_ptr<BookmarkLoadDetails> details,
-                  base::SequencedTaskRunner* task_runner) {
+}  // namespace
+
+void LoadBookmarks(const base::FilePath& path, BookmarkLoadDetails* details) {
+  base::AssertBlockingAllowed();
   bool load_index = false;
   bool bookmark_file_exists = base::PathExists(path);
   if (bookmark_file_exists) {
@@ -98,60 +102,82 @@ void LoadCallback(const base::FilePath& path,
 
   // Load any extra root nodes now, after the IDs have been potentially
   // reassigned.
-  details->LoadExtraNodes();
-
-  // Load the index if there are any bookmarks in the extra nodes.
-  const BookmarkPermanentNodeList& extra_nodes = details->extra_nodes();
-  for (size_t i = 0; i < extra_nodes.size(); ++i) {
-    if (!extra_nodes[i]->empty()) {
-      load_index = true;
-      break;
-    }
-  }
-
-  if (load_index) {
+  if (details->LoadExtraNodes()) {
     TimeTicks start_time = TimeTicks::Now();
-    AddBookmarksToIndex(details.get(), details->bb_node());
-    AddBookmarksToIndex(details.get(), details->other_folder_node());
-    AddBookmarksToIndex(details.get(), details->mobile_folder_node());
-    for (size_t i = 0; i < extra_nodes.size(); ++i)
-      AddBookmarksToIndex(details.get(), extra_nodes[i].get());
+    AddBookmarksToIndex(details, details->root_node());
     UMA_HISTOGRAM_TIMES("Bookmarks.CreateBookmarkIndexTime",
                         TimeTicks::Now() - start_time);
   }
-
-  task_runner->PostTask(
-      FROM_HERE, base::BindOnce(&BookmarkStorage::OnLoadFinished, storage,
-                                std::move(details)));
 }
 
-}  // namespace
-
 // BookmarkLoadDetails ---------------------------------------------------------
 
-BookmarkLoadDetails::BookmarkLoadDetails(
-    BookmarkPermanentNode* bb_node,
-    BookmarkPermanentNode* other_folder_node,
-    BookmarkPermanentNode* mobile_folder_node,
-    const LoadExtraCallback& load_extra_callback,
-    std::unique_ptr<TitledUrlIndex> index,
-    int64_t max_id)
-    : bb_node_(bb_node),
-      other_folder_node_(other_folder_node),
-      mobile_folder_node_(mobile_folder_node),
-      load_extra_callback_(load_extra_callback),
-      index_(std::move(index)),
+BookmarkLoadDetails::BookmarkLoadDetails(BookmarkClient* client)
+    : load_extra_callback_(client->GetLoadExtraNodesCallback()),
+      index_(std::make_unique<TitledUrlIndex>()),
       model_sync_transaction_version_(
-          BookmarkNode::kInvalidSyncTransactionVersion),
-      max_id_(max_id),
-      ids_reassigned_(false) {}
+          BookmarkNode::kInvalidSyncTransactionVersion) {
+  // WARNING: do NOT add |client| as a member. Much of this code runs on another
+  // thread, and |client_| is not thread safe, and/or may be destroyed before
+  // this.
+  root_node_ = std::make_unique<BookmarkNode>(GURL());
+  root_node_ptr_ = root_node_.get();
+  // WARNING: order is important here, various places assume the order is
+  // constant (but can vary between embedders with the initial visibility
+  // of permanent nodes).
+  bb_node_ = CreatePermanentNode(client, BookmarkNode::BOOKMARK_BAR);
+  other_folder_node_ = CreatePermanentNode(client, BookmarkNode::OTHER_NODE);
+  mobile_folder_node_ = CreatePermanentNode(client, BookmarkNode::MOBILE);
+}
 
 BookmarkLoadDetails::~BookmarkLoadDetails() {
 }
 
-void BookmarkLoadDetails::LoadExtraNodes() {
-  if (!load_extra_callback_.is_null())
-    extra_nodes_ = load_extra_callback_.Run(&max_id_);
+bool BookmarkLoadDetails::LoadExtraNodes() {
+  if (!load_extra_callback_)
+    return false;
+
+  BookmarkPermanentNodeList extra_nodes =
+      std::move(load_extra_callback_).Run(&max_id_);
+  bool has_non_empty_node = false;
+  for (auto& node : extra_nodes) {
+    if (node->child_count() != 0)
+      has_non_empty_node = true;
+    root_node_->Add(std::move(node), root_node_->child_count());
+  }
+  return has_non_empty_node;
+}
+
+BookmarkPermanentNode* BookmarkLoadDetails::CreatePermanentNode(
+    BookmarkClient* client,
+    BookmarkNode::Type type) {
+  DCHECK(type == BookmarkNode::BOOKMARK_BAR ||
+         type == BookmarkNode::OTHER_NODE || type == BookmarkNode::MOBILE);
+  std::unique_ptr<BookmarkPermanentNode> node =
+      std::make_unique<BookmarkPermanentNode>(max_id_++);
+  node->set_type(type);
+  node->set_visible(client->IsPermanentNodeVisible(node.get()));
+
+  int title_id;
+  switch (type) {
+    case BookmarkNode::BOOKMARK_BAR:
+      title_id = IDS_BOOKMARK_BAR_FOLDER_NAME;
+      break;
+    case BookmarkNode::OTHER_NODE:
+      title_id = IDS_BOOKMARK_BAR_OTHER_FOLDER_NAME;
+      break;
+    case BookmarkNode::MOBILE:
+      title_id = IDS_BOOKMARK_BAR_MOBILE_FOLDER_NAME;
+      break;
+    default:
+      NOTREACHED();
+      title_id = IDS_BOOKMARK_BAR_FOLDER_NAME;
+      break;
+  }
+  node->SetTitle(l10n_util::GetStringUTF16(title_id));
+  BookmarkPermanentNode* permanent_node = node.get();
+  root_node_->Add(std::move(node), root_node_->child_count());
+  return permanent_node;
 }
 
 // BookmarkStorage -------------------------------------------------------------
@@ -173,15 +199,6 @@ BookmarkStorage::~BookmarkStorage() {
     writer_.DoScheduledWrite();
 }
 
-void BookmarkStorage::LoadBookmarks(
-    std::unique_ptr<BookmarkLoadDetails> details,
-    const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
-  sequenced_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&LoadCallback, writer_.path(), weak_factory_.GetWeakPtr(),
-                     std::move(details), base::RetainedRef(task_runner)));
-}
-
 void BookmarkStorage::ScheduleSave() {
   switch (backup_state_) {
     case BACKUP_NONE:
@@ -222,14 +239,6 @@ bool BookmarkStorage::SerializeData(std::string* output) {
   return serializer.Serialize(*(value.get()));
 }
 
-void BookmarkStorage::OnLoadFinished(
-    std::unique_ptr<BookmarkLoadDetails> details) {
-  if (!model_)
-    return;
-
-  model_->DoneLoading(std::move(details));
-}
-
 bool BookmarkStorage::SaveNow() {
   if (!model_ || !model_->loaded()) {
     // We should only get here if we have a valid model and it's finished

components/bookmarks/browser/bookmark_storage.h

@@ -26,7 +26,9 @@ class SequencedTaskRunner;
 
 namespace bookmarks {
 
+class BookmarkClient;
 class BookmarkModel;
+class BookmarkNode;
 
 // A list of BookmarkPermanentNodes that owns them.
 using BookmarkPermanentNodeList =
@@ -35,7 +37,8 @@ using BookmarkPermanentNodeList =
 // A callback that generates a BookmarkPermanentNodeList, given a max ID to
 // use. The max ID argument will be updated after any new nodes have been
 // created and assigned IDs.
-using LoadExtraCallback = base::Callback<BookmarkPermanentNodeList(int64_t*)>;
+using LoadExtraCallback =
+    base::OnceCallback<BookmarkPermanentNodeList(int64_t*)>;
 
 // BookmarkLoadDetails is used by BookmarkStorage when loading bookmarks.
 // BookmarkModel creates a BookmarkLoadDetails and passes it (including
@@ -47,38 +50,21 @@ using LoadExtraCallback = base::Callback<BookmarkPermanentNodeList(int64_t*)>;
 // threading problems.
 class BookmarkLoadDetails {
  public:
-  BookmarkLoadDetails(BookmarkPermanentNode* bb_node,
-                      BookmarkPermanentNode* other_folder_node,
-                      BookmarkPermanentNode* mobile_folder_node,
-                      const LoadExtraCallback& load_extra_callback,
-                      std::unique_ptr<TitledUrlIndex> index,
-                      int64_t max_id);
+  explicit BookmarkLoadDetails(BookmarkClient* client);
   ~BookmarkLoadDetails();
 
-  void LoadExtraNodes();
+  // Loads the extra nodes and adds them to |root_|. Returns true if at least
+  // one node was added that has children.
+  bool LoadExtraNodes();
 
-  BookmarkPermanentNode* bb_node() { return bb_node_.get(); }
-  std::unique_ptr<BookmarkPermanentNode> owned_bb_node() {
-    return std::move(bb_node_);
-  }
-  BookmarkPermanentNode* mobile_folder_node() {
-    return mobile_folder_node_.get();
-  }
-  std::unique_ptr<BookmarkPermanentNode> owned_mobile_folder_node() {
-    return std::move(mobile_folder_node_);
-  }
-  BookmarkPermanentNode* other_folder_node() {
-    return other_folder_node_.get();
-  }
-  std::unique_ptr<BookmarkPermanentNode> owned_other_folder_node() {
-    return std::move(other_folder_node_);
-  }
-  const BookmarkPermanentNodeList& extra_nodes() {
-    return extra_nodes_;
-  }
-  BookmarkPermanentNodeList owned_extra_nodes() {
-    return std::move(extra_nodes_);
+  std::unique_ptr<BookmarkNode> owned_root_node() {
+    return std::move(root_node_);
   }
+  BookmarkNode* root_node() { return root_node_ptr_; }
+  BookmarkPermanentNode* bb_node() { return bb_node_; }
+  BookmarkPermanentNode* mobile_folder_node() { return mobile_folder_node_; }
+  BookmarkPermanentNode* other_folder_node() { return other_folder_node_; }
+
   TitledUrlIndex* index() { return index_.get(); }
   std::unique_ptr<TitledUrlIndex> owned_index() { return std::move(index_); }
 
@@ -120,22 +106,33 @@ class BookmarkLoadDetails {
   bool ids_reassigned() const { return ids_reassigned_; }
 
  private:
-  std::unique_ptr<BookmarkPermanentNode> bb_node_;
-  std::unique_ptr<BookmarkPermanentNode> other_folder_node_;
-  std::unique_ptr<BookmarkPermanentNode> mobile_folder_node_;
+  // Creates one of the possible permanent nodes (bookmark bar node, other node
+  // and mobile node) from |type|. The node is added to (and owned by) |root_|.
+  BookmarkPermanentNode* CreatePermanentNode(BookmarkClient* client,
+                                             BookmarkNode::Type type);
+
+  std::unique_ptr<BookmarkNode> root_node_;
+  BookmarkNode* root_node_ptr_;
+  BookmarkPermanentNode* bb_node_ = nullptr;
+  BookmarkPermanentNode* other_folder_node_ = nullptr;
+  BookmarkPermanentNode* mobile_folder_node_ = nullptr;
   LoadExtraCallback load_extra_callback_;
-  BookmarkPermanentNodeList extra_nodes_;
   std::unique_ptr<TitledUrlIndex> index_;
   BookmarkNode::MetaInfoMap model_meta_info_map_;
   int64_t model_sync_transaction_version_;
-  int64_t max_id_;
+  int64_t max_id_ = 1;
   std::string computed_checksum_;
   std::string stored_checksum_;
-  bool ids_reassigned_;
+  bool ids_reassigned_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkLoadDetails);
 };
 
+// Loads the bookmarks. This is intended to be called on the background thread.
+// Updates state in |details| based on the load.
+void LoadBookmarks(const base::FilePath& profile_path,
+                   BookmarkLoadDetails* details);
+
 // BookmarkStorage handles reading/write the bookmark bar model. The
 // BookmarkModel uses the BookmarkStorage to load bookmarks from disk, as well
 // as notifying the BookmarkStorage every time the model changes.
@@ -151,13 +148,6 @@ class BookmarkStorage : public base::ImportantFileWriter::DataSerializer {
                   base::SequencedTaskRunner* sequenced_task_runner);
   ~BookmarkStorage() override;
 
-  // Loads the bookmarks into the model, notifying the model when done. This
-  // takes ownership of |details| and send the |OnLoadFinished| callback from
-  // a task in |task_runner|. See BookmarkLoadDetails for details.
-  void LoadBookmarks(
-      std::unique_ptr<BookmarkLoadDetails> details,
-      const scoped_refptr<base::SequencedTaskRunner>& task_runner);
-
   // Schedules saving the bookmark bar model to disk.
   void ScheduleSave();
 
@@ -165,9 +155,6 @@ class BookmarkStorage : public base::ImportantFileWriter::DataSerializer {
   // a pending save, it is saved immediately.
   void BookmarkModelDeleted();
 
-  // Callback from backend after loading the bookmark file.
-  void OnLoadFinished(std::unique_ptr<BookmarkLoadDetails> details);
-
   // ImportantFileWriter::DataSerializer implementation.
   bool SerializeData(std::string* output) override;
 

components/bookmarks/test/test_bookmark_client.cc

@@ -30,10 +30,11 @@ std::unique_ptr<BookmarkModel> TestBookmarkClient::CreateModel() {
 // static
 std::unique_ptr<BookmarkModel> TestBookmarkClient::CreateModelWithClient(
     std::unique_ptr<BookmarkClient> client) {
+  BookmarkClient* client_ptr = client.get();
   std::unique_ptr<BookmarkModel> bookmark_model(
       new BookmarkModel(std::move(client)));
   std::unique_ptr<BookmarkLoadDetails> details =
-      bookmark_model->CreateLoadDetails();
+      std::make_unique<BookmarkLoadDetails>(client_ptr);
   details->LoadExtraNodes();
   bookmark_model->DoneLoading(std::move(details));
   return bookmark_model;