summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--chrome/browser/bookmarks/bookmark_codec.cc100
-rw-r--r--chrome/browser/bookmarks/bookmark_codec.h49
-rw-r--r--chrome/browser/bookmarks/bookmark_codec_unittest.cc140
3 files changed, 94 insertions, 195 deletions
diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc
index 92f6dcf..d666fe6 100644
--- a/chrome/browser/bookmarks/bookmark_codec.cc
+++ b/chrome/browser/bookmarks/bookmark_codec.cc
@@ -4,6 +4,8 @@
#include "chrome/browser/bookmarks/bookmark_codec.h"
+#include <algorithm>
+
#include "app/l10n_util.h"
#include "base/string_util.h"
#include "base/values.h"
@@ -13,59 +15,6 @@
using base::Time;
-UniqueIDGenerator::UniqueIDGenerator() {
- Reset();
-}
-
-int64 UniqueIDGenerator::GetUniqueID(int64 id) {
- // 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;
-
- return id;
-}
-
-bool UniqueIDGenerator::IsIdAssigned(int64 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(int64 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<int64>);
- for (int64 i = 0; i <= current_max_; ++i)
- assigned_ids_->insert(i);
- assigned_ids_->insert(id);
-}
-
-void UniqueIDGenerator::Reset() {
- current_max_ = 0;
- assigned_ids_.reset(NULL);
-}
-
const wchar_t* BookmarkCodec::kRootsKey = L"roots";
const wchar_t* BookmarkCodec::kRootFolderNameKey = L"bookmark_bar";
const wchar_t* BookmarkCodec::kOtherBookmarkFolderNameKey = L"other";
@@ -85,7 +34,9 @@ const wchar_t* BookmarkCodec::kTypeFolder = L"folder";
static const int kCurrentVersion = 1;
BookmarkCodec::BookmarkCodec()
- : ids_reassigned_(false) {
+ : ids_reassigned_(false),
+ ids_missing_(false),
+ maximum_id_(0) {
}
Value* BookmarkCodec::Encode(BookmarkModel* model) {
@@ -115,16 +66,17 @@ bool BookmarkCodec::Decode(BookmarkNode* bb_node,
BookmarkNode* other_folder_node,
int64* max_id,
const Value& value) {
- // TODO(munjal): Instead of paying the price of ID generator in al lcases, we
- // should only pay the price if we detect the file has changed and do a second
- // pass to reassign IDs. See issue 16357.
- id_generator_.Reset();
ids_reassigned_ = false;
+ ids_missing_ = false;
+ maximum_id_ = 0;
stored_checksum_.clear();
InitializeChecksum();
bool success = DecodeHelper(bb_node, other_folder_node, value);
FinalizeChecksum();
- *max_id = id_generator_.current_max() + 1;
+ // If either the checksums differ or some IDs were missing, reassign IDs.
+ if (ids_missing_ || computed_checksum() != stored_checksum())
+ ReassignIDs(bb_node, other_folder_node);
+ *max_id = maximum_id_ + 1;
return success;
}
@@ -230,17 +182,13 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
BookmarkNode* node) {
std::string id_string;
int64 id = 0;
- if (value.GetString(kIdKey, &id_string))
- if (!StringToInt64(id_string, &id))
- return false;
- int64 new_id = id_generator_.GetUniqueID(id);
- if (new_id != id)
- ids_reassigned_ = true;
- id = new_id;
+ if (!value.GetString(kIdKey, &id_string) || !StringToInt64(id_string, &id))
+ ids_missing_ = true;
+
+ maximum_id_ = std::max(maximum_id_, id);
std::wstring title;
- if (!value.GetString(kNameKey, &title))
- title = std::wstring();
+ value.GetString(kNameKey, &title);
std::wstring date_added_string;
if (!value.GetString(kDateAddedKey, &date_added_string))
@@ -283,7 +231,6 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
node = new BookmarkNode(id, GURL());
} else {
// If a new node is not created, explicitly assign ID to the existing one.
- DCHECK(id != 0);
node->set_id(id);
}
@@ -306,6 +253,21 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
return true;
}
+void BookmarkCodec::ReassignIDs(BookmarkNode* bb_node,
+ BookmarkNode* other_node) {
+ maximum_id_ = 0;
+ ReassignIDsHelper(bb_node);
+ ReassignIDsHelper(other_node);
+ ids_reassigned_ = true;
+}
+
+void BookmarkCodec::ReassignIDsHelper(BookmarkNode* node) {
+ DCHECK(node);
+ node->set_id(++maximum_id_);
+ for (int i = 0; i < node->GetChildCount(); ++i)
+ ReassignIDsHelper(node->GetChild(i));
+}
+
void BookmarkCodec::UpdateChecksum(const std::string& str) {
MD5Update(&md5_context_, str.data(), str.length() * sizeof(char));
}
diff --git a/chrome/browser/bookmarks/bookmark_codec.h b/chrome/browser/bookmarks/bookmark_codec.h
index 2c13cef..becdf4e 100644
--- a/chrome/browser/bookmarks/bookmark_codec.h
+++ b/chrome/browser/bookmarks/bookmark_codec.h
@@ -9,7 +9,6 @@
#ifndef CHROME_BROWSER_BOOKMARKS_BOOKMARK_CODEC_H_
#define CHROME_BROWSER_BOOKMARKS_BOOKMARK_CODEC_H_
-#include <set>
#include <string>
#include "base/basictypes.h"
@@ -22,39 +21,6 @@ class DictionaryValue;
class ListValue;
class Value;
-// Utility class to help assign unique 64-bit IDs.
-class UniqueIDGenerator {
- public:
- UniqueIDGenerator();
-
- // Checks whether the given ID can be used as a unique ID or not. If it can,
- // returns the id itself, otherwise generates a new unique id in a simple way
- // and returns that.
- // NOTE that if id is 0, a new unique id is returned.
- int64 GetUniqueID(int64 id);
-
- // Resets the ID generator to initial state.
- void Reset();
-
- // Returns the current maximum.
- int64 current_max() const { return current_max_; }
-
- private:
- // Checks if the given ID is already assigned.
- bool IsIdAssigned(int64 id) const;
-
- // Records the given ID as assigned.
- void RecordId(int64 id);
-
- // Maximum value we have seen so far.
- int64 current_max_;
-
- // All IDs assigned so far.
- scoped_ptr<std::set<int64> > assigned_ids_;
-
- DISALLOW_COPY_AND_ASSIGN(UniqueIDGenerator);
-};
-
// BookmarkCodec is responsible for encoding/decoding bookmarks into JSON
// values. BookmarkCodec is used by BookmarkService.
@@ -135,6 +101,12 @@ class BookmarkCodec {
bool DecodeChildren(const ListValue& child_value_list,
BookmarkNode* parent);
+ // Reassigns bookmark IDs for all nodes.
+ void ReassignIDs(BookmarkNode* bb_node, BookmarkNode* other_node);
+
+ // Helper to recursively reassign IDs.
+ void ReassignIDsHelper(BookmarkNode* node);
+
// 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.
@@ -161,12 +133,12 @@ class BookmarkCodec {
void InitializeChecksum();
void FinalizeChecksum();
- // Unique ID generator used during decoding.
- UniqueIDGenerator id_generator_;
-
// Whether or not IDs were reassigned by the codec.
bool ids_reassigned_;
+ // Whether or not IDs were missing for some bookmark nodes during decoding.
+ bool ids_missing_;
+
// MD5 context used to compute MD5 hash of all bookmark data.
MD5Context md5_context_;
@@ -174,6 +146,9 @@ class BookmarkCodec {
std::string computed_checksum_;
std::string stored_checksum_;
+ // Maximum ID assigned when decoding data.
+ int64 maximum_id_;
+
DISALLOW_COPY_AND_ASSIGN(BookmarkCodec);
};
diff --git a/chrome/browser/bookmarks/bookmark_codec_unittest.cc b/chrome/browser/bookmarks/bookmark_codec_unittest.cc
index ed3996e..0ba16a7 100644
--- a/chrome/browser/bookmarks/bookmark_codec_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_codec_unittest.cc
@@ -145,6 +145,21 @@ class BookmarkCodecTest : public testing::Test {
return model.release();
}
+
+ void CheckIDs(const BookmarkNode* node, std::set<int64>* assigned_ids) {
+ DCHECK(node);
+ int64 node_id = node->id();
+ EXPECT_TRUE(assigned_ids->find(node_id) == assigned_ids->end());
+ assigned_ids->insert(node_id);
+ for (int i = 0; i < node->GetChildCount(); ++i)
+ CheckIDs(node->GetChild(i), assigned_ids);
+ }
+
+ void ExpectIDsUnique(BookmarkModel* model) {
+ std::set<int64> assigned_ids;
+ CheckIDs(model->GetBookmarkBarNode(), &assigned_ids);
+ CheckIDs(model->other_node(), &assigned_ids);
+ }
};
TEST_F(BookmarkCodecTest, ChecksumEncodeDecodeTest) {
@@ -199,6 +214,42 @@ TEST_F(BookmarkCodecTest, ChecksumManualEditTest) {
*value.get(), enc_checksum, &dec_checksum, false));
}
+TEST_F(BookmarkCodecTest, ChecksumManualEditIDsTest) {
+ scoped_ptr<BookmarkModel> model_to_encode(CreateTestModel3());
+
+ // The test depends on existence of multiple children under bookmark bar, so
+ // make sure that's the case.
+ int bb_child_count = model_to_encode->GetBookmarkBarNode()->GetChildCount();
+ ASSERT_GT(bb_child_count, 1);
+
+ std::string enc_checksum;
+ scoped_ptr<Value> value(EncodeHelper(model_to_encode.get(), &enc_checksum));
+
+ EXPECT_TRUE(value.get() != NULL);
+
+ // Change IDs for all children of bookmark bar to be 1.
+ DictionaryValue* child_value;
+ for (int i = 0; i < bb_child_count; ++i) {
+ GetBookmarksBarChildValue(value.get(), i, &child_value);
+ std::string id;
+ ASSERT_TRUE(child_value->GetString(BookmarkCodec::kIdKey, &id));
+ ASSERT_TRUE(child_value->SetString(BookmarkCodec::kIdKey, "1"));
+ }
+
+ std::string dec_checksum;
+ scoped_ptr<BookmarkModel> decoded_model(DecodeHelper(
+ *value.get(), enc_checksum, &dec_checksum, true));
+
+ ExpectIDsUnique(decoded_model.get());
+
+ // add a few extra nodes to bookmark model and make sure IDs are still uniuqe.
+ const BookmarkNode* bb_node = decoded_model->GetBookmarkBarNode();
+ decoded_model->AddURL(bb_node, 0, L"new url1", GURL(L"http://newurl1.com"));
+ decoded_model->AddURL(bb_node, 0, L"new url2", GURL(L"http://newurl2.com"));
+
+ ExpectIDsUnique(decoded_model.get());
+}
+
TEST_F(BookmarkCodecTest, PersistIDsTest) {
scoped_ptr<BookmarkModel> model_to_encode(CreateTestModel3());
BookmarkCodec encoder;
@@ -230,92 +281,3 @@ TEST_F(BookmarkCodecTest, PersistIDsTest) {
&decoded_model2,
true);
}
-
-class UniqueIDGeneratorTest : public testing::Test {
- protected:
- void TestMixed(UniqueIDGenerator* gen) {
- // Few unique numbers.
- for (int64 i = 1; i <= 5; ++i) {
- EXPECT_EQ(i, gen->GetUniqueID(i));
- }
-
- // All numbers from 1 to 5 should produce numbers 6 to 10.
- for (int64 i = 1; i <= 5; ++i) {
- EXPECT_EQ(5 + i, gen->GetUniqueID(i));
- }
-
- // 10 should produce 11, then 11 should produce 12, and so on.
- for (int64 i = 1; i <= 5; ++i) {
- EXPECT_EQ(10 + i, gen->GetUniqueID(9 + i));
- }
-
- // Any numbers between 1 and 15 should produce a new numbers in sequence.
- EXPECT_EQ(16, gen->GetUniqueID(10));
- EXPECT_EQ(17, gen->GetUniqueID(2));
- EXPECT_EQ(18, gen->GetUniqueID(14));
- EXPECT_EQ(19, gen->GetUniqueID(7));
- EXPECT_EQ(20, gen->GetUniqueID(4));
-
- // Numbers not yet generated should work.
- EXPECT_EQ(100, gen->GetUniqueID(100));
- EXPECT_EQ(21, gen->GetUniqueID(21));
- EXPECT_EQ(200, gen->GetUniqueID(200));
-
- // Now any existing number should produce numbers starting from 201.
- EXPECT_EQ(201, gen->GetUniqueID(1));
- EXPECT_EQ(202, gen->GetUniqueID(20));
- EXPECT_EQ(203, gen->GetUniqueID(21));
- EXPECT_EQ(204, gen->GetUniqueID(100));
- EXPECT_EQ(205, gen->GetUniqueID(200));
- }
-};
-
-TEST_F(UniqueIDGeneratorTest, SerialNumbersTest) {
- UniqueIDGenerator gen;
- for (int64 i = 1; i <= 10; ++i) {
- EXPECT_EQ(i, gen.GetUniqueID(i));
- }
-}
-
-TEST_F(UniqueIDGeneratorTest, UniqueSortedNumbersTest) {
- UniqueIDGenerator gen;
- for (int64 i = 1; i <= 10; i += 2) {
- EXPECT_EQ(i, gen.GetUniqueID(i));
- }
-}
-
-TEST_F(UniqueIDGeneratorTest, UniqueUnsortedConsecutiveNumbersTest) {
- UniqueIDGenerator gen;
- int numbers[] = {2, 10, 6, 3, 8, 5, 1, 7, 4, 9};
- for (int64 i = 0; i < ARRAYSIZE(numbers); ++i) {
- EXPECT_EQ(numbers[i], gen.GetUniqueID(numbers[i]));
- }
-}
-
-TEST_F(UniqueIDGeneratorTest, UniqueUnsortedNumbersTest) {
- UniqueIDGenerator gen;
- int numbers[] = {20, 100, 60, 30, 80, 50, 10, 70, 40, 90};
- for (int64 i = 0; i < ARRAYSIZE(numbers); ++i) {
- EXPECT_EQ(numbers[i], gen.GetUniqueID(numbers[i]));
- }
-}
-
-TEST_F(UniqueIDGeneratorTest, AllDuplicatesTest) {
- UniqueIDGenerator gen;
- for (int64 i = 1; i <= 10; ++i) {
- EXPECT_EQ(i, gen.GetUniqueID(1));
- }
-}
-
-TEST_F(UniqueIDGeneratorTest, MixedTest) {
- UniqueIDGenerator gen;
- TestMixed(&gen);
-}
-
-TEST_F(UniqueIDGeneratorTest, ResetTest) {
- UniqueIDGenerator gen;
- for (int64 i = 0; i < 5; ++i) {
- TestMixed(&gen);
- gen.Reset();
- }
-}
generated by cgit v1.1 at 2025-02-10 08:31:40 +0000