Move the bookmark node iD generation to bookmark model isntead of

bookmark node. This will also make the IDs more dense.

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

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

7 files changed, 87 insertions, 56 deletions

diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc
index 731de81..6e80666 100644
--- a/chrome/browser/bookmarks/bookmark_codec.cc
+++ b/chrome/browser/bookmarks/bookmark_codec.cc
@@ -18,21 +18,52 @@ UniqueIDGenerator::UniqueIDGenerator() {
 }
 
 int UniqueIDGenerator::GetUniqueID(int id) {
-  if (assigned_ids_.find(id) != assigned_ids_.end())
+  // If the given ID is already assigned, generate new ID.
+  if (IsIdAssigned(id))
     id = current_max_ + 1;
 
+  // Record the new ID as assigned.
+  RecordId(id);
+
   if (id > current_max_)
     current_max_ = id;
 
-  assigned_ids_.insert(id);
   return id;
 }
 
+bool UniqueIDGenerator::IsIdAssigned(int id) const {
+  // If the set is already instantiated, then use the set to determine if the
+  // given ID is assigned. Otherwise use the current maximum to determine if the
+  // given ID is assigned.
+  if (assigned_ids_.get())
+    return assigned_ids_->find(id) != assigned_ids_->end();
+  else
+    return id <= current_max_;
+}
+
+void UniqueIDGenerator::RecordId(int id) {
+  // If the set is instantiated, then use the set.
+  if (assigned_ids_.get()) {
+    assigned_ids_->insert(id);
+    return;
+  }
+
+  // The set is not yet instantiated. If the ID is current_max_ + 1, then just
+  // update the current_max_. Otherwise, instantiate the set and add all IDs
+  // from 0 to current_max_ to it.
+  if (id == current_max_ + 1) {
+    ++current_max_;
+    return;
+  }
+  assigned_ids_.reset(new std::set<int>);
+  for (int i = 0; i <= current_max_; ++i)
+    assigned_ids_->insert(i);
+  assigned_ids_->insert(id);
+}
+
 void UniqueIDGenerator::Reset() {
   current_max_ = 0;
-  assigned_ids_.clear();
-  // 0 should always be considered as an ID that's already generated.
-  assigned_ids_.insert(0);
+  assigned_ids_.reset(NULL);
 }
 
 const wchar_t* BookmarkCodec::kRootsKey = L"roots";
@@ -89,7 +120,7 @@ bool BookmarkCodec::Decode(BookmarkModel* model, const Value& value) {
   InitializeChecksum();
   bool success = DecodeHelper(model, value);
   FinalizeChecksum();
-  BookmarkNode::SetNextId(id_generator_.current_max() + 1);
+  model->set_next_node_id(id_generator_.current_max() + 1);
   return success;
 }
 
@@ -206,8 +237,8 @@ bool BookmarkCodec::DecodeNode(BookmarkModel* model,
     if (value.GetString(kIdKey, &id_string))
       if (!StringToInt(id_string, &id))
         return false;
-    id = id_generator_.GetUniqueID(id);
   }
+  id = id_generator_.GetUniqueID(id);
 
   std::wstring title;
   if (!value.GetString(kNameKey, &title))
diff --git a/chrome/browser/bookmarks/bookmark_codec.h b/chrome/browser/bookmarks/bookmark_codec.h
index 3af303d..a87f751 100644
--- a/chrome/browser/bookmarks/bookmark_codec.h
+++ b/chrome/browser/bookmarks/bookmark_codec.h
@@ -13,6 +13,7 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/scoped_ptr.h"
 #include "base/md5.h"
 
 class BookmarkModel;
@@ -39,10 +40,16 @@ class UniqueIDGenerator {
   int current_max() const { return current_max_; }
 
  private:
+  // Checks if the given ID is already assigned.
+  bool IsIdAssigned(int id) const;
+
+  // Records the given ID as assigned.
+  void RecordId(int id);
+
   // Maximum value we have seen so far.
   int current_max_;
   // All IDs assigned so far.
-  std::set<int> assigned_ids_;
+  scoped_ptr<std::set<int> > assigned_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(UniqueIDGenerator);
 };
diff --git a/chrome/browser/bookmarks/bookmark_codec_unittest.cc b/chrome/browser/bookmarks/bookmark_codec_unittest.cc
index 1b7a4d3..93a5d42 100644
--- a/chrome/browser/bookmarks/bookmark_codec_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_codec_unittest.cc
@@ -190,9 +190,6 @@ TEST_F(BookmarkCodecTest, PersistIDsTest) {
   BookmarkCodec encoder(true);
   scoped_ptr<Value> model_value(encoder.Encode(model_to_encode.get()));
 
-  // Set the next id to 1 to simulate fresh Chrome start.
-  BookmarkNode::SetNextId(1);
-
   BookmarkModel decoded_model(NULL);
   BookmarkCodec decoder(true);
   ASSERT_TRUE(decoder.Decode(&decoded_model, *model_value.get()));
@@ -212,9 +209,6 @@ TEST_F(BookmarkCodecTest, PersistIDsTest) {
   BookmarkCodec encoder2(true);
   scoped_ptr<Value> model_value2(encoder2.Encode(&decoded_model));
 
-  // Set the next id to 1 to simulate fresh Chrome start.
-  BookmarkNode::SetNextId(1);
-
   BookmarkModel decoded_model2(NULL);
   BookmarkCodec decoder2(true);
   ASSERT_TRUE(decoder2.Decode(&decoded_model2, *model_value2.get()));
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 646aa3f..209cc2b 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -19,19 +19,6 @@ using base::Time;
 
 // BookmarkNode ---------------------------------------------------------------
 
-namespace {
-
-// ID for BookmarkNodes.
-// Various places assume an invalid id if == 0, for that reason we start with 1.
-int next_id_ = 1;
-
-}
-
-// static
-void BookmarkNode::SetNextId(int next_id) {
-  next_id_ = next_id;
-}
-
 const SkBitmap& BookmarkNode::GetFavIcon() {
   if (!loaded_favicon_) {
     loaded_favicon_ = true;
@@ -52,7 +39,7 @@ BookmarkNode::BookmarkNode(BookmarkModel* model, int id, const GURL& url)
 
 void BookmarkNode::Initialize(BookmarkModel* model, int id) {
   model_ = model;
-  id_ = id == 0 ? next_id_++ : id;
+  id_ = id;
   loaded_favicon_ = false;
   favicon_load_handle_ = 0;
   type_ = !url_.is_empty() ? history::StarredEntry::URL :
@@ -108,9 +95,9 @@ BookmarkModel::BookmarkModel(Profile* profile)
       ALLOW_THIS_IN_INITIALIZER_LIST(root_(this, GURL())),
       bookmark_bar_node_(NULL),
       other_node_(NULL),
+      next_node_id_(1),
       observers_(ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY),
-      loaded_signal_(TRUE, FALSE)
-{
+      loaded_signal_(TRUE, FALSE) {
   // Create the bookmark bar and other bookmarks folders. These always exist.
   CreateBookmarkNode();
   CreateOtherBookmarksNode();
@@ -289,7 +276,9 @@ BookmarkNode* BookmarkModel::AddGroup(
     return NULL;
   }
 
-  BookmarkNode* new_node = new BookmarkNode(this, GURL());
+  BookmarkNode* new_node = new BookmarkNode(this,
+                                            generate_next_node_id(),
+                                            GURL());
   new_node->date_group_modified_ = Time::Now();
   new_node->SetTitle(title);
   new_node->type_ = history::StarredEntry::USER_GROUP;
@@ -320,7 +309,7 @@ BookmarkNode* BookmarkModel::AddURLWithCreationTime(
 
   SetDateGroupModified(parent, creation_time);
 
-  BookmarkNode* new_node = new BookmarkNode(this, url);
+  BookmarkNode* new_node = new BookmarkNode(this, generate_next_node_id(), url);
   new_node->SetTitle(title);
   new_node->date_added_ = creation_time;
   new_node->type_ = history::StarredEntry::URL;
@@ -578,7 +567,7 @@ BookmarkNode* BookmarkModel::CreateRootNodeFromStarredEntry(
     const history::StarredEntry& entry) {
   DCHECK(entry.type == history::StarredEntry::BOOKMARK_BAR ||
          entry.type == history::StarredEntry::OTHER);
-  BookmarkNode* node = new BookmarkNode(this, GURL());
+  BookmarkNode* node = new BookmarkNode(this, generate_next_node_id(), GURL());
   node->Reset(entry);
   if (entry.type == history::StarredEntry::BOOKMARK_BAR)
     node->SetTitle(l10n_util::GetString(IDS_BOOMARK_BAR_FOLDER_NAME));
@@ -670,3 +659,7 @@ void BookmarkModel::PopulateNodesByURL(BookmarkNode* node) {
   for (int i = 0; i < node->GetChildCount(); ++i)
     PopulateNodesByURL(node->GetChild(i));
 }
+
+int BookmarkModel::generate_next_node_id() {
+  return next_node_id_++;
+}
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 4b987d9..c0451e5 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -45,16 +45,13 @@ class BookmarkNode : public views::TreeNode<BookmarkNode> {
   FRIEND_TEST(BookmarkCodecTest, PersistIDsTest);
   FRIEND_TEST(BookmarkEditorViewTest, ChangeParentAndURL);
   FRIEND_TEST(BookmarkEditorViewTest, EditURLKeepsPosition);
+  FRIEND_TEST(BookmarkMenuBridgeTest, TestAddNodeToMenu);
   FRIEND_TEST(BookmarkEditorGtkTest, ChangeParentAndURL);
   FRIEND_TEST(BookmarkEditorGtkTest, EditURLKeepsPosition);
   FRIEND_TEST(BookmarkModelTest, MostRecentlyAddedEntries);
   FRIEND_TEST(BookmarkModelTest, GetMostRecentlyAddedNodeForURL);
 
  public:
-  // Creates a new node with the given properties. If the ID is not given or is
-  // 0, it is automatically assigned.
-  BookmarkNode(BookmarkModel* model, const GURL& url);
-  BookmarkNode(BookmarkModel* model, int id, const GURL& url);
   virtual ~BookmarkNode() {}
 
   // Returns the favicon for the this node. If the favicon has not yet been
@@ -100,8 +97,9 @@ class BookmarkNode : public views::TreeNode<BookmarkNode> {
   // HistoryContentsProvider.
 
  private:
-  // Sets the next ID for bookmark node ID generation.
-  static void SetNextId(int next_id);
+  // Creates a new node with the given properties.
+  BookmarkNode(BookmarkModel* model, const GURL& url);
+  BookmarkNode(BookmarkModel* model, int id, const GURL& url);
 
   // helper to initialize various fields during construction.
   void Initialize(BookmarkModel* model, int id);
@@ -207,6 +205,7 @@ class BookmarkModelObserver {
 // Profile.
 
 class BookmarkModel : public NotificationObserver, public BookmarkService {
+  friend class BookmarkCodec;
   friend class BookmarkNode;
   friend class BookmarkModelTest;
   friend class BookmarkStorage;
@@ -397,6 +396,15 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
+  // Generates and returns the next node ID.
+  int generate_next_node_id();
+
+  // Sets the maximum node ID to the given value.
+  // This is used by BookmarkCodec to report the maximum ID after it's done
+  // decoding since during decoding codec can assign IDs to nodes if IDs are
+  // persisted.
+  void set_next_node_id(int id) { next_node_id_ = id; }
+
   Profile* profile_;
 
   // Whether the initial set of data has been loaded.
@@ -409,6 +417,9 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   BookmarkNode* bookmark_bar_node_;
   BookmarkNode* other_node_;
 
+  // The maximum ID assigned to the bookmark nodes in the model.
+  int next_node_id_;
+
   // The observers.
   ObserverList<BookmarkModelObserver> observers_;
 
diff --git a/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm b/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
index 2fe6127..6cf2efb 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
@@ -66,6 +66,7 @@ TEST_F(BookmarkMenuBridgeTest, TestClearBookmarkMenu) {
 // Test that AddNodeToMenu() properly adds bookmark nodes as menus,
 // including the recursive case.
 TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
+  std::wstring empty;
   Profile* profile = browser_test_helper_.profile();
 
   scoped_ptr<BookmarkMenuBridge> bridge(new BookmarkMenuBridge());
@@ -73,8 +74,9 @@ TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"foo"] autorelease];
 
-  BookmarkModel* model = new BookmarkModel(profile);
-  BookmarkNode* root = new BookmarkNode(model, GURL());
+  BookmarkModel* model = profile->GetBookmarkModel();
+  BookmarkNode* bookmark_bar = model->GetBookmarkBarNode();
+  BookmarkNode* root = model->AddGroup(bookmark_bar, 0, empty);
   EXPECT_TRUE(model && root);
 
   const char* short_url = "http://foo/";
@@ -86,16 +88,12 @@ TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
   // 3 nodes; middle one has a child, last one has a HUGE URL
   // Set their titles to be the same as the URLs
   BookmarkNode* node = NULL;
-  root->Add(0, new BookmarkNode(model, GURL(short_url)));
-  root->Add(1, new BookmarkNode(model, GURL()));
-  root->Add(2, new BookmarkNode(model, GURL(long_url)));
-
-  root->GetChild(0)->SetTitle(ASCIIToWide(short_url));
-  root->GetChild(2)->SetTitle(ASCIIToWide(long_url));
+  model->AddURL(root, 0, ASCIIToWide(short_url), GURL(short_url));
+  node = model->AddGroup(root, 1, empty);
+  model->AddURL(root, 2, ASCIIToWide(long_url), GURL(long_url));
 
   // And the submenu fo the middle one
-  node = new BookmarkNode(model, GURL("http://sub"));
-  root->GetChild(1)->Add(0, node);
+  model->AddURL(node, 0, empty, GURL("http://sub"));
 
   // Add to the NSMenu, then confirm it looks good
   AddNodeToMenu(bridge.get(), root, menu);
@@ -131,7 +129,4 @@ TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
   // e.g. http://foo becomes http://foo/)
   EXPECT_GE([[[menu itemAtIndex:0] toolTip] length], (2*strlen(short_url) - 5));
   EXPECT_GE([[[menu itemAtIndex:2] toolTip] length], (2*strlen(long_url) - 5));
-
-  delete root;  // deletes all its kids
-  delete model;
 }
diff --git a/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm b/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm
index d70f51a..90895fc 100644
--- a/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm
@@ -20,17 +20,17 @@
 
 - (id)init {
   if ((self = [super init])) {
+    std::wstring empty;
     helper_ = new BrowserTestHelper();
     BookmarkModel* model = helper_->browser()->profile()->GetBookmarkModel();
-    nodes_[0] = new BookmarkNode(model, GURL("http://0.com"));
-    nodes_[1] = new BookmarkNode(model, GURL("http://1.com"));
+    BookmarkNode* bookmark_bar = model->GetBookmarkBarNode();
+    nodes_[0] = model->AddURL(bookmark_bar, 0, empty, GURL("http://0.com"));
+    nodes_[1] = model->AddURL(bookmark_bar, 1, empty, GURL("http://1.com"));
   }
   return self;
 }
 
 - (void)dealloc {
-  delete nodes_[0];
-  delete nodes_[1];
   delete helper_;
   [super dealloc];
 }