Moves decoding and population of bookmark index to background thread.

BUG=6646
TEST=make sure bookmarks persist after running chrome.

Review URL: http://codereview.chromium.org/113768

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@16757 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 203 insertions, 111 deletions

diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc
index 6025994..5517782 100644
--- a/chrome/browser/bookmarks/bookmark_codec.cc
+++ b/chrome/browser/bookmarks/bookmark_codec.cc
@@ -114,13 +114,16 @@ Value* BookmarkCodec::Encode(BookmarkNode* bookmark_bar_node,
   return main;
 }
 
-bool BookmarkCodec::Decode(BookmarkModel* model, const Value& value) {
+bool BookmarkCodec::Decode(BookmarkNode* bb_node,
+                           BookmarkNode* other_folder_node,
+                           int* max_id,
+                           const Value& value) {
   id_generator_.Reset();
   stored_checksum_.clear();
   InitializeChecksum();
-  bool success = DecodeHelper(model, value);
+  bool success = DecodeHelper(bb_node, other_folder_node, max_id, value);
   FinalizeChecksum();
-  model->set_next_node_id(id_generator_.current_max() + 1);
+  *max_id = id_generator_.current_max() + 1;
   return success;
 }
 
@@ -155,7 +158,10 @@ Value* BookmarkCodec::EncodeNode(BookmarkNode* node) {
   return value;
 }
 
-bool BookmarkCodec::DecodeHelper(BookmarkModel* model, const Value& value) {
+bool BookmarkCodec::DecodeHelper(BookmarkNode* bb_node,
+                                 BookmarkNode* other_folder_node,
+                                 int* max_id,
+                                 const Value& value) {
   if (value.GetType() != Value::TYPE_DICTIONARY)
     return false;  // Unexpected type.
 
@@ -174,7 +180,6 @@ bool BookmarkCodec::DecodeHelper(BookmarkModel* model, const Value& value) {
       return false;
   }
 
-
   Value* roots;
   if (!d_value.Get(kRootsKey, &roots))
     return false;  // No roots.
@@ -191,25 +196,23 @@ bool BookmarkCodec::DecodeHelper(BookmarkModel* model, const Value& value) {
       other_folder_value->GetType() != Value::TYPE_DICTIONARY)
     return false;  // Invalid type for root folder and/or other folder.
 
-  DecodeNode(model, *static_cast<DictionaryValue*>(root_folder_value),
-             NULL, model->GetBookmarkBarNode());
-  DecodeNode(model, *static_cast<DictionaryValue*>(other_folder_value),
-             NULL, model->other_node());
+  DecodeNode(*static_cast<DictionaryValue*>(root_folder_value), NULL,
+             bb_node);
+  DecodeNode(*static_cast<DictionaryValue*>(other_folder_value), NULL,
+             other_folder_node);
   // Need to reset the type as decoding resets the type to FOLDER. Similarly
   // we need to reset the title as the title is persisted and restored from
   // the file.
-  model->GetBookmarkBarNode()->type_ = history::StarredEntry::BOOKMARK_BAR;
-  model->other_node()->type_ = history::StarredEntry::OTHER;
-  model->GetBookmarkBarNode()->SetTitle(
-      l10n_util::GetString(IDS_BOOMARK_BAR_FOLDER_NAME));
-  model->other_node()->SetTitle(
+  bb_node->type_ = history::StarredEntry::BOOKMARK_BAR;
+  other_folder_node->type_ = history::StarredEntry::OTHER;
+  bb_node->SetTitle(l10n_util::GetString(IDS_BOOMARK_BAR_FOLDER_NAME));
+  other_folder_node->SetTitle(
       l10n_util::GetString(IDS_BOOMARK_BAR_OTHER_FOLDER_NAME));
 
   return true;
 }
 
-bool BookmarkCodec::DecodeChildren(BookmarkModel* model,
-                                   const ListValue& child_value_list,
+bool BookmarkCodec::DecodeChildren(const ListValue& child_value_list,
                                    BookmarkNode* parent) {
   for (size_t i = 0; i < child_value_list.GetSize(); ++i) {
     Value* child_value;
@@ -219,7 +222,7 @@ bool BookmarkCodec::DecodeChildren(BookmarkModel* model,
     if (child_value->GetType() != Value::TYPE_DICTIONARY)
       return false;
 
-    if (!DecodeNode(model, *static_cast<DictionaryValue*>(child_value), parent,
+    if (!DecodeNode(*static_cast<DictionaryValue*>(child_value), parent,
                     NULL)) {
       return false;
     }
@@ -227,8 +230,7 @@ bool BookmarkCodec::DecodeChildren(BookmarkModel* model,
   return true;
 }
 
-bool BookmarkCodec::DecodeNode(BookmarkModel* model,
-                               const DictionaryValue& value,
+bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
                                BookmarkNode* parent,
                                BookmarkNode* node) {
   std::string id_string;
@@ -301,7 +303,7 @@ bool BookmarkCodec::DecodeNode(BookmarkModel* model,
       parent->Add(parent->GetChildCount(), node);
 
     UpdateChecksumWithFolderNode(id_string, title);
-    if (!DecodeChildren(model, *static_cast<ListValue*>(child_values), node))
+    if (!DecodeChildren(*static_cast<ListValue*>(child_values), node))
       return false;
   }
 
diff --git a/chrome/browser/bookmarks/bookmark_codec.h b/chrome/browser/bookmarks/bookmark_codec.h
index a87f751..e335565 100644
--- a/chrome/browser/bookmarks/bookmark_codec.h
+++ b/chrome/browser/bookmarks/bookmark_codec.h
@@ -81,11 +81,15 @@ class BookmarkCodec {
   Value* Encode(BookmarkNode* bookmark_bar_node,
                 BookmarkNode* other_folder_node);
 
-  // Decodes the previously encoded value to the specified model. Returns true
-  // on success, false otherwise. If there is an error (such as unexpected
-  // version) all children are removed from the bookmark bar and other folder
-  // nodes.
-  bool Decode(BookmarkModel* model, const Value& value);
+  // Decodes the previously encoded value to the specified nodes as well as
+  // setting |max_node_id| to the greatest node id. Returns true on success,
+  // false otherwise. If there is an error (such as unexpected version) all
+  // children are removed from the bookmark bar and other folder nodes. On exit
+  // |max_node_id| is set to the max id of the nodes.
+  bool Decode(BookmarkNode* bb_node,
+              BookmarkNode* other_folder_node,
+              int* max_node_id,
+              const Value& value);
 
   // Returns the checksum computed during last encoding/decoding call.
   const std::string& computed_checksum() const { return computed_checksum_; }
@@ -121,18 +125,19 @@ class BookmarkCodec {
   Value* EncodeNode(BookmarkNode* node);
 
   // Helper to perform decoding.
-  bool DecodeHelper(BookmarkModel* model, const Value& value);
+  bool DecodeHelper(BookmarkNode* bb_node,
+                    BookmarkNode* other_folder_node,
+                    int* max_id,
+                    const Value& value);
 
   // Decodes the children of the specified node. Returns true on success.
-  bool DecodeChildren(BookmarkModel* model,
-                      const ListValue& child_value_list,
+  bool DecodeChildren(const ListValue& child_value_list,
                       BookmarkNode* parent);
 
   // Decodes the supplied node from the supplied value. Child nodes are
   // created appropriately by way of DecodeChildren. If node is NULL a new
   // node is created and added to parent, otherwise node is used.
-  bool DecodeNode(BookmarkModel* model,
-                  const DictionaryValue& value,
+  bool DecodeNode(const DictionaryValue& value,
                   BookmarkNode* parent,
                   BookmarkNode* node);
 
diff --git a/chrome/browser/bookmarks/bookmark_codec_unittest.cc b/chrome/browser/bookmarks/bookmark_codec_unittest.cc
index 93a5d42..196f7be 100644
--- a/chrome/browser/bookmarks/bookmark_codec_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_codec_unittest.cc
@@ -100,6 +100,14 @@ class BookmarkCodecTest : public testing::Test {
     return value.release();
   }
 
+  bool Decode(BookmarkCodec* codec, BookmarkModel* model, const Value& value) {
+    int max_id;
+    bool result = codec->Decode(model->GetBookmarkBarNode(),
+                                model->other_node(), &max_id, value);
+    model->set_next_node_id(max_id);
+    return result;
+  }
+
   BookmarkModel* DecodeHelper(const Value& value,
                               const std::string& expected_stored_checksum,
                               std::string* computed_checksum,
@@ -110,7 +118,7 @@ class BookmarkCodecTest : public testing::Test {
     EXPECT_EQ("", decoder.stored_checksum());
 
     scoped_ptr<BookmarkModel> model(new BookmarkModel(NULL));
-    EXPECT_TRUE(decoder.Decode(model.get(), value));
+    EXPECT_TRUE(Decode(&decoder, model.get(), value));
 
     *computed_checksum = decoder.computed_checksum();
     const std::string& stored_checksum = decoder.stored_checksum();
@@ -192,7 +200,7 @@ TEST_F(BookmarkCodecTest, PersistIDsTest) {
 
   BookmarkModel decoded_model(NULL);
   BookmarkCodec decoder(true);
-  ASSERT_TRUE(decoder.Decode(&decoded_model, *model_value.get()));
+  ASSERT_TRUE(Decode(&decoder, &decoded_model, *model_value.get()));
   BookmarkModelTestUtils::AssertModelsEqual(model_to_encode.get(),
                                             &decoded_model,
                                             true);
@@ -211,7 +219,7 @@ TEST_F(BookmarkCodecTest, PersistIDsTest) {
 
   BookmarkModel decoded_model2(NULL);
   BookmarkCodec decoder2(true);
-  ASSERT_TRUE(decoder2.Decode(&decoded_model2, *model_value2.get()));
+  ASSERT_TRUE(Decode(&decoder2, &decoded_model2, *model_value2.get()));
   BookmarkModelTestUtils::AssertModelsEqual(&decoded_model,
                                             &decoded_model2,
                                             true);
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 4ea2a19..be1d566 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -8,6 +8,7 @@
 #include "base/gfx/png_decoder.h"
 #include "base/scoped_vector.h"
 #include "build/build_config.h"
+#include "chrome/browser/bookmarks/bookmark_index.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/bookmarks/bookmark_storage.h"
 #include "chrome/browser/browser_process.h"
@@ -89,20 +90,9 @@ BookmarkModel::BookmarkModel(Profile* profile)
       next_node_id_(1),
       observers_(ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY),
       loaded_signal_(TRUE, FALSE) {
-  // Create the bookmark bar and other bookmarks folders. These always exist.
-  CreateBookmarkNode();
-  CreateOtherBookmarksNode();
-
-  // And add them to the root.
-  //
-  // WARNING: order is important here, various places assume bookmark bar then
-  // other node.
-  root_.Add(0, bookmark_bar_node_);
-  root_.Add(1, other_node_);
-
   if (!profile_) {
     // Profile is null during testing.
-    DoneLoading();
+    DoneLoading(CreateLoadDetails());
   }
 }
 
@@ -134,7 +124,7 @@ void BookmarkModel::Load() {
 
   // Load the bookmarks. BookmarkStorage notifies us when done.
   store_ = new BookmarkStorage(profile_, this);
-  store_->LoadBookmarks();
+  store_->LoadBookmarks(CreateLoadDetails());
 }
 
 BookmarkNode* BookmarkModel::GetParentForNewNodes() {
@@ -210,11 +200,11 @@ void BookmarkModel::SetTitle(BookmarkNode* node,
 
   // The title index doesn't support changing the title, instead we remove then
   // add it back.
-  index_.Remove(node);
+  index_->Remove(node);
 
   node->SetTitle(title);
 
-  index_.Add(node);
+  index_->Add(node);
 
   if (store_.get())
     store_->ScheduleSave();
@@ -376,7 +366,10 @@ void BookmarkModel::GetBookmarksWithTitlesMatching(
     const std::wstring& text,
     size_t max_count,
     std::vector<bookmark_utils::TitleMatch>* matches) {
-  index_.GetBookmarksWithTitlesMatching(text, max_count, matches);
+  if (!loaded_)
+    return;
+
+  index_->GetBookmarksWithTitlesMatching(text, max_count, matches);
 }
 
 void BookmarkModel::ClearStore() {
@@ -416,7 +409,7 @@ void BookmarkModel::RemoveNode(BookmarkNode* node,
     nodes_ordered_by_url_set_.erase(i);
     removed_urls->insert(node->GetURL());
 
-    index_.Remove(node);
+    index_->Remove(node);
   }
 
   CancelPendingFavIconLoadRequests(node);
@@ -426,8 +419,26 @@ void BookmarkModel::RemoveNode(BookmarkNode* node,
     RemoveNode(node->GetChild(i), removed_urls);
 }
 
-void BookmarkModel::DoneLoading() {
-  DCHECK(!loaded_);
+void BookmarkModel::DoneLoading(
+    BookmarkStorage::LoadDetails* details_delete_me) {
+  DCHECK(details_delete_me);
+  scoped_ptr<BookmarkStorage::LoadDetails> details(details_delete_me);
+  if (loaded_) {
+    // We should only ever be loaded once.
+    NOTREACHED();
+    return;
+  }
+
+  bookmark_bar_node_ = details->bb_node();
+  other_node_ = details->other_folder_node();
+  next_node_id_ = details->max_id();
+  index_.reset(details->index());
+  details->release();
+
+  // WARNING: order is important here, various places assume bookmark bar then
+  // other node.
+  root_.Add(0, bookmark_bar_node_);
+  root_.Add(1, other_node_);
 
   {
     AutoLock url_lock(url_lock_);
@@ -513,7 +524,7 @@ BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeAdded(this, parent, index));
 
-  index_.Add(node);
+  index_->Add(node);
 
   if (node->GetType() == history::StarredEntry::URL && !was_bookmarked) {
     history::URLsStarredDetails details(true);
@@ -559,16 +570,16 @@ void BookmarkModel::SetDateGroupModified(BookmarkNode* parent,
     store_->ScheduleSave();
 }
 
-void BookmarkModel::CreateBookmarkNode() {
+BookmarkNode* BookmarkModel::CreateBookmarkNode() {
   history::StarredEntry entry;
   entry.type = history::StarredEntry::BOOKMARK_BAR;
-  bookmark_bar_node_ = CreateRootNodeFromStarredEntry(entry);
+  return CreateRootNodeFromStarredEntry(entry);
 }
 
-void BookmarkModel::CreateOtherBookmarksNode() {
+BookmarkNode* BookmarkModel::CreateOtherBookmarksNode() {
   history::StarredEntry entry;
   entry.type = history::StarredEntry::OTHER;
-  other_node_ = CreateRootNodeFromStarredEntry(entry);
+  return CreateRootNodeFromStarredEntry(entry);
 }
 
 BookmarkNode* BookmarkModel::CreateRootNodeFromStarredEntry(
@@ -671,3 +682,10 @@ void BookmarkModel::PopulateNodesByURL(BookmarkNode* node) {
 int BookmarkModel::generate_next_node_id() {
   return next_node_id_++;
 }
+
+BookmarkStorage::LoadDetails* BookmarkModel::CreateLoadDetails() {
+  BookmarkNode* bb_node = CreateBookmarkNode();
+  BookmarkNode* other_folder_node = CreateOtherBookmarksNode();
+  return new BookmarkStorage::LoadDetails(
+      bb_node, other_folder_node, new BookmarkIndex(), next_node_id_);
+}
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 6a27ba8..2e745a5 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -14,7 +14,6 @@
 #include "base/lock.h"
 #include "base/observer_list.h"
 #include "base/waitable_event.h"
-#include "chrome/browser/bookmarks/bookmark_index.h"
 #include "chrome/browser/bookmarks/bookmark_service.h"
 #include "chrome/browser/bookmarks/bookmark_storage.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
@@ -27,6 +26,7 @@
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
 class BookmarkEditorView;
+class BookmarkIndex;
 class BookmarkModel;
 class BookmarkCodec;
 class Profile;
@@ -205,8 +205,7 @@ class BookmarkModelObserver {
 // Profile.
 
 class BookmarkModel : public NotificationObserver, public BookmarkService {
-  friend class BookmarkCodec;
-  friend class BookmarkNode;
+  friend class BookmarkCodecTest;
   friend class BookmarkModelTest;
   friend class BookmarkStorage;
 
@@ -222,10 +221,10 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // the root node.
   BookmarkNode* root_node() { return &root_; }
 
-  // Returns the bookmark bar node.
+  // Returns the bookmark bar node. This is NULL until loaded.
   BookmarkNode* GetBookmarkBarNode() { return bookmark_bar_node_; }
 
-  // Returns the 'other' node.
+  // Returns the 'other' node. This is NULL until loaded.
   BookmarkNode* other_node() { return other_node_; }
 
   // Returns the parent the last node was added to. This never returns NULL
@@ -348,7 +347,8 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   void RemoveNode(BookmarkNode* node, std::set<GURL>* removed_urls);
 
   // Invoked when loading is finished. Sets loaded_ and notifies observers.
-  void DoneLoading();
+  // BookmarkModel takes ownership of |details|.
+  void DoneLoading(BookmarkStorage::LoadDetails* details);
 
   // Populates nodes_ordered_by_url_set_ from root.
   void PopulateNodesByURL(BookmarkNode* node);
@@ -376,8 +376,8 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
 
   // Creates the bookmark bar/other nodes. These call into
   // CreateRootNodeFromStarredEntry.
-  void CreateBookmarkNode();
-  void CreateOtherBookmarksNode();
+  BookmarkNode* CreateBookmarkNode();
+  BookmarkNode* CreateOtherBookmarksNode();
 
   // Creates a root node (either the bookmark bar node or other node) from the
   // specified starred entry.
@@ -414,6 +414,10 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // persisted.
   void set_next_node_id(int id) { next_node_id_ = id; }
 
+  // Creates and returns a new LoadDetails. It's up to the caller to delete
+  // the returned object.
+  BookmarkStorage::LoadDetails* CreateLoadDetails();
+
   NotificationRegistrar registrar_;
 
   Profile* profile_;
@@ -448,7 +452,7 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // Reads/writes bookmarks to disk.
   scoped_refptr<BookmarkStorage> store_;
 
-  BookmarkIndex index_;
+  scoped_ptr<BookmarkIndex> index_;
 
   base::WaitableEvent loaded_signal_;
 
diff --git a/chrome/browser/bookmarks/bookmark_storage.cc b/chrome/browser/bookmarks/bookmark_storage.cc
index e237d66..5680852 100644
--- a/chrome/browser/bookmarks/bookmark_storage.cc
+++ b/chrome/browser/bookmarks/bookmark_storage.cc
@@ -65,35 +65,66 @@ class FileDeleteTask : public Task {
 
 class BookmarkStorage::LoadTask : public Task {
  public:
-  LoadTask(const FilePath& path, MessageLoop* loop,
-           BookmarkStorage* storage)
+  LoadTask(const FilePath& path,
+           MessageLoop* loop,
+           BookmarkStorage* storage,
+           LoadDetails* details)
       : path_(path),
         loop_(loop),
-        storage_(storage) {
+        storage_(storage),
+        details_(details) {
   }
 
   virtual void Run() {
     bool bookmark_file_exists = file_util::PathExists(path_);
-    Value* root = NULL;
     if (bookmark_file_exists) {
       JSONFileValueSerializer serializer(path_);
-      root = serializer.Deserialize(NULL);
+      scoped_ptr<Value> root(serializer.Deserialize(NULL));
+
+      if (root.get()) {
+        // Building the index cane take a while, so we do it on the background
+        // thread.
+        int max_node_id = 0;
+        BookmarkCodec codec;
+        TimeTicks start_time = TimeTicks::Now();
+        codec.Decode(details_->bb_node(), details_->other_folder_node(),
+                     &max_node_id, *root.get());
+        details_->set_max_id(std::max(max_node_id, details_->max_id()));
+        UMA_HISTOGRAM_TIMES("Bookmarks.DecodeTime",
+                            TimeTicks::Now() - start_time);
+
+        start_time = TimeTicks::Now();
+        AddBookmarksToIndex(details_->bb_node());
+        AddBookmarksToIndex(details_->other_folder_node());
+        UMA_HISTOGRAM_TIMES("Bookmarks.CreateBookmarkIndexTime",
+                            TimeTicks::Now() - start_time);
+      }
     }
 
-    // BookmarkStorage takes ownership of root.
     if (loop_) {
       loop_->PostTask(FROM_HERE, NewRunnableMethod(
           storage_.get(), &BookmarkStorage::OnLoadFinished,
-          bookmark_file_exists, path_, root));
+          bookmark_file_exists, path_));
     } else {
-      storage_->OnLoadFinished(bookmark_file_exists, path_, root);
+      storage_->OnLoadFinished(bookmark_file_exists, path_);
     }
   }
 
  private:
+  // Adds node to the model's index, recursing through all children as well.
+  void AddBookmarksToIndex(BookmarkNode* node) {
+    if (node->is_url()) {
+      details_->index()->Add(node);
+    } else {
+      for (int i = 0; i < node->GetChildCount(); ++i)
+        AddBookmarksToIndex(node->GetChild(i));
+    }
+  }
+
   const FilePath path_;
   MessageLoop* loop_;
   scoped_refptr<BookmarkStorage> storage_;
+  LoadDetails* details_;
 
   DISALLOW_COPY_AND_ASSIGN(LoadTask);
 };
@@ -117,14 +148,18 @@ BookmarkStorage::~BookmarkStorage() {
     writer_.DoScheduledWrite();
 }
 
-void BookmarkStorage::LoadBookmarks() {
+void BookmarkStorage::LoadBookmarks(LoadDetails* details) {
+  DCHECK(!details_.get());
+  DCHECK(details);
+  details_.reset(details);
   DoLoadBookmarks(writer_.path());
 }
 
 void BookmarkStorage::DoLoadBookmarks(const FilePath& path) {
   Task* task = new LoadTask(path,
                             backend_thread() ? MessageLoop::current() : NULL,
-                            this);
+                            this,
+                            details_.get());
   RunTaskOnBackendThread(task);
 }
 
@@ -136,7 +171,7 @@ void BookmarkStorage::MigrateFromHistory() {
   if (!history) {
     // This happens in unit tests.
     if (model_)
-      model_->DoneLoading();
+      model_->DoneLoading(details_.release());
     return;
   }
   if (!history->backend_loaded()) {
@@ -177,10 +212,7 @@ bool BookmarkStorage::SerializeData(std::string* output) {
   return serializer.Serialize(*(value.get()));
 }
 
-void BookmarkStorage::OnLoadFinished(bool file_exists, const FilePath& path,
-                                     Value* root_value) {
-  scoped_ptr<Value> value_ref(root_value);
-
+void BookmarkStorage::OnLoadFinished(bool file_exists, const FilePath& path) {
   if (path == writer_.path() && !file_exists) {
     // The file doesn't exist. This means one of two things:
     // 1. A clean profile.
@@ -195,20 +227,7 @@ void BookmarkStorage::OnLoadFinished(bool file_exists, const FilePath& path,
   if (!model_)
     return;
 
-  if (root_value) {
-    TimeTicks start_time = TimeTicks::Now();
-    BookmarkCodec codec;
-    codec.Decode(model_, *root_value);
-    UMA_HISTOGRAM_TIMES("Bookmarks.DecodeTime",
-                        TimeTicks::Now() - start_time);
-
-    start_time = TimeTicks::Now();
-    AddBookmarksToIndex(model_->root_node());
-    UMA_HISTOGRAM_TIMES("Bookmarks.CreatebookmarkIndexTime",
-                        TimeTicks::Now() - start_time);
-  }
-
-  model_->DoneLoading();
+  model_->DoneLoading(details_.release());
 
   if (path == tmp_history_path_) {
     // We just finished migration from history. Save now to new file,
@@ -257,12 +276,3 @@ void BookmarkStorage::RunTaskOnBackendThread(Task* task) const {
     delete task;
   }
 }
-
-void BookmarkStorage::AddBookmarksToIndex(BookmarkNode* node) {
-  if (node->is_url()) {
-    model_->index_.Add(node);
-  } else {
-    for (int i = 0; i < node->GetChildCount(); ++i)
-      AddBookmarksToIndex(node->GetChild(i));
-  }
-}
diff --git a/chrome/browser/bookmarks/bookmark_storage.h b/chrome/browser/bookmarks/bookmark_storage.h
index bb6c0cc..f9a080d 100644
--- a/chrome/browser/bookmarks/bookmark_storage.h
+++ b/chrome/browser/bookmarks/bookmark_storage.h
@@ -7,6 +7,8 @@
 
 #include "base/file_path.h"
 #include "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+#include "chrome/browser/bookmarks/bookmark_index.h"
 #include "chrome/common/important_file_writer.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
@@ -30,12 +32,55 @@ class BookmarkStorage : public NotificationObserver,
                         public ImportantFileWriter::DataSerializer,
                         public base::RefCountedThreadSafe<BookmarkStorage> {
  public:
+  // LoadDetails is used by BookmarkStorage when loading bookmarks.
+  // BookmarkModel creates a LoadDetails and passes it (including ownership) to
+  // BookmarkStorage. BoomarkStorage loads the bookmarks (and index) in the
+  // background thread, then calls back to the BookmarkModel (on the main
+  // thread) when loading is done, passing ownership back to the BookmarkModel.
+  // While loading BookmarkModel does not maintain references to the contents
+  // of the LoadDetails, this ensures we don't have any threading problems.
+  class LoadDetails {
+   public:
+    LoadDetails(BookmarkNode* bb_node,
+                BookmarkNode* other_folder_node,
+                BookmarkIndex* index,
+                int max_id)
+        : bb_node_(bb_node),
+          other_folder_node_(other_folder_node),
+          index_(index),
+          max_id_(max_id) {
+    }
+
+    void release() {
+      bb_node_.release();
+      other_folder_node_.release();
+      index_.release();
+    }
+
+    BookmarkNode* bb_node() { return bb_node_.get(); }
+    BookmarkNode* other_folder_node() { return other_folder_node_.get(); }
+    BookmarkIndex* index() { return index_.get(); }
+
+    // Max id of the nodes.
+    void set_max_id(int max_id) { max_id_ = max_id; }
+    int max_id() const { return max_id_; }
+
+   private:
+    scoped_ptr<BookmarkNode> bb_node_;
+    scoped_ptr<BookmarkNode> other_folder_node_;
+    scoped_ptr<BookmarkIndex> index_;
+    int max_id_;
+
+    DISALLOW_COPY_AND_ASSIGN(LoadDetails);
+  };
+
   // Creates a BookmarkStorage for the specified model
   BookmarkStorage(Profile* profile, BookmarkModel* model);
   ~BookmarkStorage();
 
-  // Loads the bookmarks into the model, notifying the model when done.
-  void LoadBookmarks();
+  // Loads the bookmarks into the model, notifying the model when done. This
+  // takes ownership of |details|. See LoadDetails for details.
+  void LoadBookmarks(LoadDetails* details);
 
   // Schedules saving the bookmark bar model to disk.
   void ScheduleSave();
@@ -53,8 +98,8 @@ class BookmarkStorage : public NotificationObserver,
   // Callback from backend with the results of the bookmark file.
   // This may be called multiple times, with different paths. This happens when
   // we migrate bookmark data from database.
-  void OnLoadFinished(bool file_exists, const FilePath& path,
-                      Value* root_value);
+  void OnLoadFinished(bool file_exists,
+                      const FilePath& path);
 
   // Loads bookmark data from |file| and notifies the model when finished.
   void DoLoadBookmarks(const FilePath& file);
@@ -85,9 +130,6 @@ class BookmarkStorage : public NotificationObserver,
   // Returns the thread the backend is run on.
   const base::Thread* backend_thread() const { return backend_thread_; }
 
-  // Adds node to the model's index, recursing through all children as well.
-  void AddBookmarksToIndex(BookmarkNode* node);
-
   // Keep the pointer to profile, we may need it for migration from history.
   Profile* profile_;
 
@@ -107,6 +149,9 @@ class BookmarkStorage : public NotificationObserver,
   // Path to temporary file created during migrating bookmarks from history.
   const FilePath tmp_history_path_;
 
+  // See class description of LoadDetails for details on this.
+  scoped_ptr<LoadDetails> details_;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkStorage);
 };