diff options
author | yfriedman@chromium.org <yfriedman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-05-10 18:57:49 +0000 |
---|---|---|
committer | yfriedman@chromium.org <yfriedman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-05-10 18:57:49 +0000 |
commit | 08873a8668f8ec74d3f7ccb7f64971b11d57176c (patch) | |
tree | 3173e698cc11856c935375e753d561b8f43546cc /chrome | |
parent | 64b59db5208271c0fe9ac18d83a81f0a1ea11768 (diff) | |
download | chromium_src-08873a8668f8ec74d3f7ccb7f64971b11d57176c.zip chromium_src-08873a8668f8ec74d3f7ccb7f64971b11d57176c.tar.gz chromium_src-08873a8668f8ec74d3f7ccb7f64971b11d57176c.tar.bz2 |
Initial implementation of "Synced Bookmarks" folder.
Mostly ensures that sycned bookmarks are correctly treated as an immutable
folder. Some of the UI-bits maybe incomplete. For example, if
enable-synced-bookmarks-folder is set, then synced bookmarks will appear in the
bookmark manager page and some components of the UI but it's not on the bookmark
bar or anything like that. This change also ensures that the synced bookmark
folder matches a sync folder if one is available.
BUG=
TEST=
Review URL: http://codereview.chromium.org/6931018
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@84829 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
27 files changed, 497 insertions, 132 deletions
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 508e39ef..b946546 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -5796,6 +5796,9 @@ Keep your key file in a safe place. You will need it to create new versions of y <message name="IDS_BOOMARK_BAR_FOLDER_NAME" desc="Name shown in the tree for the bookmarks bar folder"> Bookmarks bar </message> + <message name="IDS_BOOMARK_BAR_SYNCED_FOLDER_NAME" desc="Name shown in the tree for the synced bookmarks folder"> + Synced bookmarks + </message> <message name="IDS_BOOMARK_BAR_OTHER_FOLDER_NAME" desc="Name shown in the tree for the other bookmarks folder"> Other bookmarks </message> @@ -5804,6 +5807,9 @@ Keep your key file in a safe place. You will need it to create new versions of y <message name="IDS_BOOMARK_BAR_FOLDER_NAME" desc="In Title Case: Name shown in the tree for the bookmarks bar folder"> Bookmarks Bar </message> + <message name="IDS_BOOMARK_BAR_SYNCED_FOLDER_NAME" desc="In Title Case: Name shown in the tree for the synced bookmarks folder"> + Synced Bookmarks + </message> <message name="IDS_BOOMARK_BAR_OTHER_FOLDER_NAME" desc="In Title Case: Name shown in the tree for the other bookmarks folder"> Other Bookmarks </message> diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc index dd145b3..dee4f84 100644 --- a/chrome/browser/bookmarks/bookmark_codec.cc +++ b/chrome/browser/bookmarks/bookmark_codec.cc @@ -19,6 +19,7 @@ using base::Time; const char* BookmarkCodec::kRootsKey = "roots"; const char* BookmarkCodec::kRootFolderNameKey = "bookmark_bar"; const char* BookmarkCodec::kOtherBookmarkFolderNameKey = "other"; +const char* BookmarkCodec::kSyncedBookmarkFolderNameKey = "synced"; const char* BookmarkCodec::kVersionKey = "version"; const char* BookmarkCodec::kChecksumKey = "checksum"; const char* BookmarkCodec::kIdKey = "id"; @@ -43,16 +44,19 @@ BookmarkCodec::BookmarkCodec() BookmarkCodec::~BookmarkCodec() {} Value* BookmarkCodec::Encode(BookmarkModel* model) { - return Encode(model->GetBookmarkBarNode(), model->other_node()); + return Encode(model->GetBookmarkBarNode(), model->other_node(), + model->synced_node()); } Value* BookmarkCodec::Encode(const BookmarkNode* bookmark_bar_node, - const BookmarkNode* other_folder_node) { + const BookmarkNode* other_folder_node, + const BookmarkNode* synced_folder_node) { ids_reassigned_ = false; InitializeChecksum(); DictionaryValue* roots = new DictionaryValue(); roots->Set(kRootFolderNameKey, EncodeNode(bookmark_bar_node)); roots->Set(kOtherBookmarkFolderNameKey, EncodeNode(other_folder_node)); + roots->Set(kSyncedBookmarkFolderNameKey, EncodeNode(synced_folder_node)); DictionaryValue* main = new DictionaryValue(); main->SetInteger(kVersionKey, kCurrentVersion); @@ -67,6 +71,7 @@ Value* BookmarkCodec::Encode(const BookmarkNode* bookmark_bar_node, bool BookmarkCodec::Decode(BookmarkNode* bb_node, BookmarkNode* other_folder_node, + BookmarkNode* synced_folder_node, int64* max_id, const Value& value) { ids_.clear(); @@ -75,12 +80,13 @@ bool BookmarkCodec::Decode(BookmarkNode* bb_node, maximum_id_ = 0; stored_checksum_.clear(); InitializeChecksum(); - bool success = DecodeHelper(bb_node, other_folder_node, value); + bool success = DecodeHelper(bb_node, other_folder_node, synced_folder_node, + value); FinalizeChecksum(); // If either the checksums differ or some IDs were missing/not unique, // reassign IDs. if (!ids_valid_ || computed_checksum() != stored_checksum()) - ReassignIDs(bb_node, other_folder_node); + ReassignIDs(bb_node, other_folder_node, synced_folder_node); *max_id = maximum_id_ + 1; return success; } @@ -115,6 +121,7 @@ Value* BookmarkCodec::EncodeNode(const BookmarkNode* node) { bool BookmarkCodec::DecodeHelper(BookmarkNode* bb_node, BookmarkNode* other_folder_node, + BookmarkNode* synced_folder_node, const Value& value) { if (value.GetType() != Value::TYPE_DICTIONARY) return false; // Unexpected type. @@ -144,24 +151,34 @@ bool BookmarkCodec::DecodeHelper(BookmarkNode* bb_node, DictionaryValue* roots_d_value = static_cast<DictionaryValue*>(roots); Value* root_folder_value; Value* other_folder_value; + Value* synced_folder_value; if (!roots_d_value->Get(kRootFolderNameKey, &root_folder_value) || root_folder_value->GetType() != Value::TYPE_DICTIONARY || !roots_d_value->Get(kOtherBookmarkFolderNameKey, &other_folder_value) || - other_folder_value->GetType() != Value::TYPE_DICTIONARY) - return false; // Invalid type for root folder and/or other folder. + other_folder_value->GetType() != Value::TYPE_DICTIONARY || + !roots_d_value->Get(kSyncedBookmarkFolderNameKey, &synced_folder_value) || + synced_folder_value->GetType() != Value::TYPE_DICTIONARY) { + return false; // Invalid type for root folder and/or synced and/or other + // folder. + } DecodeNode(*static_cast<DictionaryValue*>(root_folder_value), NULL, bb_node); DecodeNode(*static_cast<DictionaryValue*>(other_folder_value), NULL, other_folder_node); + DecodeNode(*static_cast<DictionaryValue*>(synced_folder_value), NULL, + synced_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. bb_node->set_type(BookmarkNode::BOOKMARK_BAR); other_folder_node->set_type(BookmarkNode::OTHER_NODE); + synced_folder_node->set_type(BookmarkNode::SYNCED); bb_node->set_title(l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_FOLDER_NAME)); other_folder_node->set_title( l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_OTHER_FOLDER_NAME)); + synced_folder_node->set_title( + l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_SYNCED_FOLDER_NAME)); return true; } @@ -289,10 +306,12 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value, } void BookmarkCodec::ReassignIDs(BookmarkNode* bb_node, - BookmarkNode* other_node) { + BookmarkNode* other_node, + BookmarkNode* synced_node) { maximum_id_ = 0; ReassignIDsHelper(bb_node); ReassignIDsHelper(other_node); + ReassignIDsHelper(synced_node); ids_reassigned_ = true; } diff --git a/chrome/browser/bookmarks/bookmark_codec.h b/chrome/browser/bookmarks/bookmark_codec.h index fe57d1f..2d29df8 100644 --- a/chrome/browser/bookmarks/bookmark_codec.h +++ b/chrome/browser/bookmarks/bookmark_codec.h @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -46,7 +46,8 @@ class BookmarkCodec { // This method is public for use by StarredURLDatabase in migrating the // bookmarks out of the database. Value* Encode(const BookmarkNode* bookmark_bar_node, - const BookmarkNode* other_folder_node); + const BookmarkNode* other_folder_node, + const BookmarkNode* synced_folder_node); // 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, @@ -55,6 +56,7 @@ class BookmarkCodec { // |max_node_id| is set to the max id of the nodes. bool Decode(BookmarkNode* bb_node, BookmarkNode* other_folder_node, + BookmarkNode* synced_folder_node, int64* max_node_id, const Value& value); @@ -76,6 +78,7 @@ class BookmarkCodec { static const char* kRootsKey; static const char* kRootFolderNameKey; static const char* kOtherBookmarkFolderNameKey; + static const char* kSyncedBookmarkFolderNameKey; static const char* kVersionKey; static const char* kChecksumKey; static const char* kIdKey; @@ -98,6 +101,7 @@ class BookmarkCodec { // Helper to perform decoding. bool DecodeHelper(BookmarkNode* bb_node, BookmarkNode* other_folder_node, + BookmarkNode* synced_folder_node, const Value& value); // Decodes the children of the specified node. Returns true on success. @@ -105,7 +109,9 @@ class BookmarkCodec { BookmarkNode* parent); // Reassigns bookmark IDs for all nodes. - void ReassignIDs(BookmarkNode* bb_node, BookmarkNode* other_node); + void ReassignIDs(BookmarkNode* bb_node, + BookmarkNode* other_node, + BookmarkNode* synced_node); // Helper to recursively reassign IDs. void ReassignIDsHelper(BookmarkNode* node); diff --git a/chrome/browser/bookmarks/bookmark_codec_unittest.cc b/chrome/browser/bookmarks/bookmark_codec_unittest.cc index 09b9bd4..4569fb5 100644 --- a/chrome/browser/bookmarks/bookmark_codec_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_codec_unittest.cc @@ -111,6 +111,7 @@ class BookmarkCodecTest : public testing::Test { int64 max_id; bool result = codec->Decode(AsMutable(model->GetBookmarkBarNode()), AsMutable(model->other_node()), + AsMutable(model->synced_node()), &max_id, value); model->set_next_node_id(max_id); return result; @@ -161,6 +162,7 @@ class BookmarkCodecTest : public testing::Test { std::set<int64> assigned_ids; CheckIDs(model->GetBookmarkBarNode(), &assigned_ids); CheckIDs(model->other_node(), &assigned_ids); + CheckIDs(model->synced_node(), &assigned_ids); } }; diff --git a/chrome/browser/bookmarks/bookmark_html_writer.cc b/chrome/browser/bookmarks/bookmark_html_writer.cc index faaec66..85c48ed 100644 --- a/chrome/browser/bookmarks/bookmark_html_writer.cc +++ b/chrome/browser/bookmarks/bookmark_html_writer.cc @@ -114,12 +114,16 @@ class Writer : public Task { DictionaryValue* roots_d_value = static_cast<DictionaryValue*>(roots); Value* root_folder_value; Value* other_folder_value; + Value* synced_folder_value; if (!roots_d_value->Get(BookmarkCodec::kRootFolderNameKey, &root_folder_value) || root_folder_value->GetType() != Value::TYPE_DICTIONARY || !roots_d_value->Get(BookmarkCodec::kOtherBookmarkFolderNameKey, &other_folder_value) || - other_folder_value->GetType() != Value::TYPE_DICTIONARY) { + other_folder_value->GetType() != Value::TYPE_DICTIONARY || + !roots_d_value->Get(BookmarkCodec::kSyncedBookmarkFolderNameKey, + &synced_folder_value) || + synced_folder_value->GetType() != Value::TYPE_DICTIONARY) { NOTREACHED(); return; // Invalid type for root folder and/or other folder. } @@ -129,7 +133,9 @@ class Writer : public Task { if (!WriteNode(*static_cast<DictionaryValue*>(root_folder_value), BookmarkNode::BOOKMARK_BAR) || !WriteNode(*static_cast<DictionaryValue*>(other_folder_value), - BookmarkNode::OTHER_NODE)) { + BookmarkNode::OTHER_NODE) || + !WriteNode(*static_cast<DictionaryValue*>(synced_folder_value), + BookmarkNode::SYNCED)) { return; } @@ -286,10 +292,11 @@ class Writer : public Task { NOTREACHED(); return false; } - if (folder_type != BookmarkNode::OTHER_NODE) { - // The other folder name is not written out. This gives the effect of - // making the contents of the 'other folder' be a sibling to the bookmark - // bar folder. + if (folder_type != BookmarkNode::OTHER_NODE && + folder_type != BookmarkNode::SYNCED) { + // The other/synced folder name are not written out. This gives the effect + // of making the contents of the 'other folder' be a sibling to the + // bookmark bar folder. if (!WriteIndent() || !Write(kFolderStart) || !WriteTime(date_added_string) || @@ -329,7 +336,8 @@ class Writer : public Task { return false; } } - if (folder_type != BookmarkNode::OTHER_NODE) { + if (folder_type != BookmarkNode::OTHER_NODE && + folder_type != BookmarkNode::SYNCED) { // Close out the folder. DecrementIndent(); if (!WriteIndent() || @@ -383,6 +391,7 @@ BookmarkFaviconFetcher::~BookmarkFaviconFetcher() { void BookmarkFaviconFetcher::ExportBookmarks() { ExtractUrls(profile_->GetBookmarkModel()->GetBookmarkBarNode()); ExtractUrls(profile_->GetBookmarkModel()->other_node()); + ExtractUrls(profile_->GetBookmarkModel()->synced_node()); if (!bookmark_urls_.empty()) { FetchNextFavicon(); } else { diff --git a/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc b/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc index f203a04..0acb593 100644 --- a/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc @@ -171,6 +171,8 @@ TEST_F(BookmarkHTMLWriterTest, Test) { // F3 // F4 // url1 + // Synced + // url1 string16 f1_title = ASCIIToUTF16("F\"&;<1\""); string16 f2_title = ASCIIToUTF16("F2"); string16 f3_title = ASCIIToUTF16("F 3"); @@ -208,6 +210,7 @@ TEST_F(BookmarkHTMLWriterTest, Test) { model->AddURLWithCreationTime(f4, 0, url1_title, url1, t1); model->AddURLWithCreationTime(model->GetBookmarkBarNode(), 2, url4_title, url4, t4); + model->AddURLWithCreationTime(model->synced_node(), 0, url1_title, url1, t1); // Write to a temp file. BookmarksObserver observer(&message_loop); @@ -232,8 +235,8 @@ TEST_F(BookmarkHTMLWriterTest, Test) { NULL, &favicons); - // Check loaded favicon (url1 is represents by 3 separate bookmarks). - EXPECT_EQ(3U, favicons.size()); + // Check loaded favicon (url1 is represented by 4 separate bookmarks). + EXPECT_EQ(4U, favicons.size()); for (size_t i = 0; i < favicons.size(); i++) { if (url1_favicon == favicons[i].favicon_url) { EXPECT_EQ(1U, favicons[i].urls.size()); @@ -245,7 +248,7 @@ TEST_F(BookmarkHTMLWriterTest, Test) { } // Verify we got back what we wrote. - ASSERT_EQ(7U, parsed_bookmarks.size()); + ASSERT_EQ(8U, parsed_bookmarks.size()); // Windows and ChromeOS builds use Sentence case. string16 bookmark_folder_name = l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_FOLDER_NAME); @@ -263,4 +266,6 @@ TEST_F(BookmarkHTMLWriterTest, Test) { string16(), string16(), string16()); AssertBookmarkEntryEquals(parsed_bookmarks[6], false, url1, url1_title, t1, f3_title, f4_title, string16()); + AssertBookmarkEntryEquals(parsed_bookmarks[7], false, url1, url1_title, t1, + string16(), string16(), string16()); } diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc index b372855..e3de7c2 100644 --- a/chrome/browser/bookmarks/bookmark_model.cc +++ b/chrome/browser/bookmarks/bookmark_model.cc @@ -8,6 +8,7 @@ #include <functional> #include "base/callback.h" +#include "base/command_line.h" #include "base/memory/scoped_vector.h" #include "build/build_config.h" #include "chrome/browser/bookmarks/bookmark_index.h" @@ -16,6 +17,7 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/history/history_notifications.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/common/chrome_switches.h" #include "content/common/notification_service.h" #include "grit/generated_resources.h" #include "ui/base/l10n/l10n_util.h" @@ -75,6 +77,9 @@ void BookmarkNode::Reset(const history::StarredEntry& entry) { case history::StarredEntry::BOOKMARK_BAR: type_ = BookmarkNode::BOOKMARK_BAR; break; + case history::StarredEntry::SYNCED: + type_ = BookmarkNode::SYNCED; + break; case history::StarredEntry::OTHER: type_ = BookmarkNode::OTHER_NODE; break; @@ -124,6 +129,7 @@ BookmarkModel::BookmarkModel(Profile* profile) root_(GURL()), bookmark_bar_node_(NULL), other_node_(NULL), + synced_node_(NULL), next_node_id_(1), observers_(ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY), loaded_signal_(TRUE, FALSE) { @@ -259,7 +265,8 @@ void BookmarkModel::SetTitle(const BookmarkNode* node, const string16& title) { if (node->GetTitle() == title) return; - if (node == bookmark_bar_node_ || node == other_node_) { + if (node == bookmark_bar_node_ || node == other_node_ || + node == synced_node_) { NOTREACHED(); return; } @@ -568,12 +575,17 @@ void BookmarkModel::DoneLoading( } bookmark_bar_node_ = details->release_bb_node(); other_node_ = details->release_other_folder_node(); + synced_node_ = details->release_synced_folder_node(); index_.reset(details->release_index()); // WARNING: order is important here, various places assume bookmark bar then // other node. root_.Add(bookmark_bar_node_, 0); root_.Add(other_node_, 1); + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder)) { + root_.Add(synced_node_, 2); + } { base::AutoLock url_lock(url_lock_); @@ -719,14 +731,24 @@ BookmarkNode* BookmarkModel::CreateOtherBookmarksNode() { return CreateRootNodeFromStarredEntry(entry); } +BookmarkNode* BookmarkModel::CreateSyncedBookmarksNode() { + history::StarredEntry entry; + entry.type = history::StarredEntry::SYNCED; + return CreateRootNodeFromStarredEntry(entry); +} + BookmarkNode* BookmarkModel::CreateRootNodeFromStarredEntry( const history::StarredEntry& entry) { DCHECK(entry.type == history::StarredEntry::BOOKMARK_BAR || - entry.type == history::StarredEntry::OTHER); + entry.type == history::StarredEntry::OTHER || + entry.type == history::StarredEntry::SYNCED); BookmarkNode* node = new BookmarkNode(generate_next_node_id(), GURL()); node->Reset(entry); if (entry.type == history::StarredEntry::BOOKMARK_BAR) { node->set_title(l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_FOLDER_NAME)); + } else if (entry.type == history::StarredEntry::SYNCED) { + node->set_title(l10n_util::GetStringUTF16( + IDS_BOOMARK_BAR_SYNCED_FOLDER_NAME)); } else { node->set_title( l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_OTHER_FOLDER_NAME)); @@ -826,6 +848,8 @@ void BookmarkModel::SetFileChanged() { BookmarkLoadDetails* BookmarkModel::CreateLoadDetails() { BookmarkNode* bb_node = CreateBookmarkNode(); BookmarkNode* other_folder_node = CreateOtherBookmarksNode(); + BookmarkNode* synced_folder_node = CreateSyncedBookmarksNode(); return new BookmarkLoadDetails( - bb_node, other_folder_node, new BookmarkIndex(profile()), next_node_id_); + bb_node, other_folder_node, synced_folder_node, + new BookmarkIndex(profile()), next_node_id_); } diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h index 0e4ada68..8767645 100644 --- a/chrome/browser/bookmarks/bookmark_model.h +++ b/chrome/browser/bookmarks/bookmark_model.h @@ -50,7 +50,8 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> { URL, FOLDER, BOOKMARK_BAR, - OTHER_NODE + OTHER_NODE, + SYNCED }; // Creates a new node with the specified url and id of 0 explicit BookmarkNode(const GURL& url); @@ -193,6 +194,9 @@ class BookmarkModel : public NotificationObserver, public BookmarkService { // Returns the 'other' node. This is NULL until loaded. const BookmarkNode* other_node() { return other_node_; } + // Returns the 'synced' node. This is NULL until loaded. + const BookmarkNode* synced_node() { return synced_node_; } + // Returns the parent the last node was added to. This never returns NULL // (as long as the model is loaded). const BookmarkNode* GetParentForNewNodes(); @@ -311,6 +315,9 @@ class BookmarkModel : public NotificationObserver, public BookmarkService { bool is_bookmark_bar_node(const BookmarkNode* node) const { return node == bookmark_bar_node_; } + bool is_synced_bookmarks_node(const BookmarkNode* node) const { + return node == synced_node_; + } bool is_other_bookmarks_node(const BookmarkNode* node) const { return node == other_node_; } @@ -319,7 +326,8 @@ class BookmarkModel : public NotificationObserver, public BookmarkService { bool is_permanent_node(const BookmarkNode* node) const { return is_root(node) || is_bookmark_bar_node(node) || - is_other_bookmarks_node(node); + is_other_bookmarks_node(node) || + is_synced_bookmarks_node(node); } // Sets the store to NULL, making it so the BookmarkModel does not persist @@ -380,10 +388,11 @@ class BookmarkModel : public NotificationObserver, public BookmarkService { // Returns true if the parent and index are valid. bool IsValidIndex(const BookmarkNode* parent, int index, bool allow_end); - // Creates the bookmark bar/other nodes. These call into + // Creates the bookmark bar/synced/other nodes. These call into // CreateRootNodeFromStarredEntry. BookmarkNode* CreateBookmarkNode(); BookmarkNode* CreateOtherBookmarksNode(); + BookmarkNode* CreateSyncedBookmarksNode(); // Creates a root node (either the bookmark bar node or other node) from the // specified starred entry. @@ -439,6 +448,7 @@ class BookmarkModel : public NotificationObserver, public BookmarkService { BookmarkNode* bookmark_bar_node_; BookmarkNode* other_node_; + BookmarkNode* synced_node_; // The maximum ID assigned to the bookmark nodes in the model. int64 next_node_id_; diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc index 5d1a0a3..5d0aa5f 100644 --- a/chrome/browser/bookmarks/bookmark_model_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc @@ -6,6 +6,7 @@ #include <string> #include "base/base_paths.h" +#include "base/command_line.h" #include "base/file_util.h" #include "base/hash_tables.h" #include "base/path_service.h" @@ -20,6 +21,7 @@ #include "chrome/browser/history/history_notifications.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_paths.h" +#include "chrome/common/chrome_switches.h" #include "chrome/test/model_test_utils.h" #include "chrome/test/testing_browser_process_test.h" #include "chrome/test/testing_profile.h" @@ -83,12 +85,14 @@ class BookmarkModelTest : public TestingBrowserProcessTest, int index2; }; - BookmarkModelTest() : model(NULL) { - model.AddObserver(this); + BookmarkModelTest() { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kEnableSyncedBookmarksFolder); + model_.reset(new BookmarkModel(NULL)); + model_->AddObserver(this); ClearCounts(); } - void Loaded(BookmarkModel* model) { // We never load from the db, so that this should never get invoked. NOTREACHED(); @@ -152,7 +156,7 @@ class BookmarkModelTest : public TestingBrowserProcessTest, ASSERT_EQ(reordered_count, reordered_count_); } - BookmarkModel model; + scoped_ptr<BookmarkModel> model_; int moved_count; @@ -168,25 +172,32 @@ class BookmarkModelTest : public TestingBrowserProcessTest, }; TEST_F(BookmarkModelTest, InitialState) { - const BookmarkNode* bb_node = model.GetBookmarkBarNode(); + const BookmarkNode* bb_node = model_->GetBookmarkBarNode(); ASSERT_TRUE(bb_node != NULL); EXPECT_EQ(0, bb_node->child_count()); EXPECT_EQ(BookmarkNode::BOOKMARK_BAR, bb_node->type()); - const BookmarkNode* other_node = model.other_node(); + const BookmarkNode* other_node = model_->other_node(); ASSERT_TRUE(other_node != NULL); EXPECT_EQ(0, other_node->child_count()); EXPECT_EQ(BookmarkNode::OTHER_NODE, other_node->type()); + const BookmarkNode* synced_node = model_->synced_node(); + ASSERT_TRUE(synced_node != NULL); + EXPECT_EQ(0, synced_node->child_count()); + EXPECT_EQ(BookmarkNode::SYNCED, synced_node->type()); + EXPECT_TRUE(bb_node->id() != other_node->id()); + EXPECT_TRUE(bb_node->id() != synced_node->id()); + EXPECT_TRUE(other_node->id() != synced_node->id()); } TEST_F(BookmarkModelTest, AddURL) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); const string16 title(ASCIIToUTF16("foo")); const GURL url("http://foo.com"); - const BookmarkNode* new_node = model.AddURL(root, 0, title, url); + const BookmarkNode* new_node = model_->AddURL(root, 0, title, url); AssertObserverCount(1, 0, 0, 0, 0); observer_details.AssertEquals(root, NULL, 0, -1); @@ -194,17 +205,38 @@ TEST_F(BookmarkModelTest, AddURL) { ASSERT_EQ(title, new_node->GetTitle()); ASSERT_TRUE(url == new_node->GetURL()); ASSERT_EQ(BookmarkNode::URL, new_node->type()); - ASSERT_TRUE(new_node == model.GetMostRecentlyAddedNodeForURL(url)); + ASSERT_TRUE(new_node == model_->GetMostRecentlyAddedNodeForURL(url)); EXPECT_TRUE(new_node->id() != root->id() && - new_node->id() != model.other_node()->id()); + new_node->id() != model_->other_node()->id() && + new_node->id() != model_->synced_node()->id()); +} + +TEST_F(BookmarkModelTest, AddURLToSyncedBookmarks) { + const BookmarkNode* root = model_->synced_node(); + const string16 title(ASCIIToUTF16("foo")); + const GURL url("http://foo.com"); + + const BookmarkNode* new_node = model_->AddURL(root, 0, title, url); + AssertObserverCount(1, 0, 0, 0, 0); + observer_details.AssertEquals(root, NULL, 0, -1); + + ASSERT_EQ(1, root->child_count()); + ASSERT_EQ(title, new_node->GetTitle()); + ASSERT_TRUE(url == new_node->GetURL()); + ASSERT_EQ(BookmarkNode::URL, new_node->type()); + ASSERT_TRUE(new_node == model_->GetMostRecentlyAddedNodeForURL(url)); + + EXPECT_TRUE(new_node->id() != root->id() && + new_node->id() != model_->other_node()->id() && + new_node->id() != model_->synced_node()->id()); } TEST_F(BookmarkModelTest, AddFolder) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); const string16 title(ASCIIToUTF16("foo")); - const BookmarkNode* new_node = model.AddFolder(root, 0, title); + const BookmarkNode* new_node = model_->AddFolder(root, 0, title); AssertObserverCount(1, 0, 0, 0, 0); observer_details.AssertEquals(root, NULL, 0, -1); @@ -213,93 +245,94 @@ TEST_F(BookmarkModelTest, AddFolder) { ASSERT_EQ(BookmarkNode::FOLDER, new_node->type()); EXPECT_TRUE(new_node->id() != root->id() && - new_node->id() != model.other_node()->id()); + new_node->id() != model_->other_node()->id() && + new_node->id() != model_->synced_node()->id()); // Add another folder, just to make sure folder_ids are incremented correctly. ClearCounts(); - model.AddFolder(root, 0, title); + model_->AddFolder(root, 0, title); AssertObserverCount(1, 0, 0, 0, 0); observer_details.AssertEquals(root, NULL, 0, -1); } TEST_F(BookmarkModelTest, RemoveURL) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); const string16 title(ASCIIToUTF16("foo")); const GURL url("http://foo.com"); - model.AddURL(root, 0, title, url); + model_->AddURL(root, 0, title, url); ClearCounts(); - model.Remove(root, 0); + model_->Remove(root, 0); ASSERT_EQ(0, root->child_count()); AssertObserverCount(0, 0, 1, 0, 0); observer_details.AssertEquals(root, NULL, 0, -1); // Make sure there is no mapping for the URL. - ASSERT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == NULL); + ASSERT_TRUE(model_->GetMostRecentlyAddedNodeForURL(url) == NULL); } TEST_F(BookmarkModelTest, RemoveFolder) { - const BookmarkNode* root = model.GetBookmarkBarNode(); - const BookmarkNode* folder = model.AddFolder(root, 0, ASCIIToUTF16("foo")); + const BookmarkNode* root = model_->GetBookmarkBarNode(); + const BookmarkNode* folder = model_->AddFolder(root, 0, ASCIIToUTF16("foo")); ClearCounts(); // Add a URL as a child. const string16 title(ASCIIToUTF16("foo")); const GURL url("http://foo.com"); - model.AddURL(folder, 0, title, url); + model_->AddURL(folder, 0, title, url); ClearCounts(); // Now remove the folder. - model.Remove(root, 0); + model_->Remove(root, 0); ASSERT_EQ(0, root->child_count()); AssertObserverCount(0, 0, 1, 0, 0); observer_details.AssertEquals(root, NULL, 0, -1); // Make sure there is no mapping for the URL. - ASSERT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == NULL); + ASSERT_TRUE(model_->GetMostRecentlyAddedNodeForURL(url) == NULL); } TEST_F(BookmarkModelTest, SetTitle) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); string16 title(ASCIIToUTF16("foo")); const GURL url("http://foo.com"); - const BookmarkNode* node = model.AddURL(root, 0, title, url); + const BookmarkNode* node = model_->AddURL(root, 0, title, url); ClearCounts(); title = ASCIIToUTF16("foo2"); - model.SetTitle(node, title); + model_->SetTitle(node, title); AssertObserverCount(0, 0, 0, 1, 0); observer_details.AssertEquals(node, NULL, -1, -1); EXPECT_EQ(title, node->GetTitle()); } TEST_F(BookmarkModelTest, SetURL) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); const string16 title(ASCIIToUTF16("foo")); GURL url("http://foo.com"); - const BookmarkNode* node = model.AddURL(root, 0, title, url); + const BookmarkNode* node = model_->AddURL(root, 0, title, url); ClearCounts(); url = GURL("http://foo2.com"); - model.SetURL(node, url); + model_->SetURL(node, url); AssertObserverCount(0, 0, 0, 1, 0); observer_details.AssertEquals(node, NULL, -1, -1); EXPECT_EQ(url, node->GetURL()); } TEST_F(BookmarkModelTest, Move) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); const string16 title(ASCIIToUTF16("foo")); const GURL url("http://foo.com"); - const BookmarkNode* node = model.AddURL(root, 0, title, url); - const BookmarkNode* folder1 = model.AddFolder(root, 0, ASCIIToUTF16("foo")); + const BookmarkNode* node = model_->AddURL(root, 0, title, url); + const BookmarkNode* folder1 = model_->AddFolder(root, 0, ASCIIToUTF16("foo")); ClearCounts(); - model.Move(node, folder1, 0); + model_->Move(node, folder1, 0); AssertObserverCount(0, 1, 0, 0, 0); observer_details.AssertEquals(root, folder1, 1, 0); @@ -311,17 +344,17 @@ TEST_F(BookmarkModelTest, Move) { // And remove the folder. ClearCounts(); - model.Remove(root, 0); + model_->Remove(root, 0); AssertObserverCount(0, 0, 1, 0, 0); observer_details.AssertEquals(root, NULL, 0, -1); - EXPECT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == NULL); + EXPECT_TRUE(model_->GetMostRecentlyAddedNodeForURL(url) == NULL); EXPECT_EQ(0, root->child_count()); } TEST_F(BookmarkModelTest, Copy) { - const BookmarkNode* root = model.GetBookmarkBarNode(); + const BookmarkNode* root = model_->GetBookmarkBarNode(); static const std::string model_string("a 1:[ b c ] d 2:[ e f g ] h "); - model_test_utils::AddNodesFromModelString(model, root, model_string); + model_test_utils::AddNodesFromModelString(*model_, root, model_string); // Validate initial model. std::string actualModelString = model_test_utils::ModelStringFromNode(root); @@ -330,42 +363,42 @@ TEST_F(BookmarkModelTest, Copy) { // Copy 'd' to be after '1:b': URL item from bar to folder. const BookmarkNode* nodeToCopy = root->GetChild(2); const BookmarkNode* destination = root->GetChild(1); - model.Copy(nodeToCopy, destination, 1); + model_->Copy(nodeToCopy, destination, 1); actualModelString = model_test_utils::ModelStringFromNode(root); EXPECT_EQ("a 1:[ b d c ] d 2:[ e f g ] h ", actualModelString); // Copy '1:d' to be after 'a': URL item from folder to bar. const BookmarkNode* folder = root->GetChild(1); nodeToCopy = folder->GetChild(1); - model.Copy(nodeToCopy, root, 1); + model_->Copy(nodeToCopy, root, 1); actualModelString = model_test_utils::ModelStringFromNode(root); EXPECT_EQ("a d 1:[ b d c ] d 2:[ e f g ] h ", actualModelString); // Copy '1' to be after '2:e': Folder from bar to folder. nodeToCopy = root->GetChild(2); destination = root->GetChild(4); - model.Copy(nodeToCopy, destination, 1); + model_->Copy(nodeToCopy, destination, 1); actualModelString = model_test_utils::ModelStringFromNode(root); EXPECT_EQ("a d 1:[ b d c ] d 2:[ e 1:[ b d c ] f g ] h ", actualModelString); // Copy '2:1' to be after '2:f': Folder within same folder. folder = root->GetChild(4); nodeToCopy = folder->GetChild(1); - model.Copy(nodeToCopy, folder, 3); + model_->Copy(nodeToCopy, folder, 3); actualModelString = model_test_utils::ModelStringFromNode(root); EXPECT_EQ("a d 1:[ b d c ] d 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] h ", actualModelString); // Copy first 'd' to be after 'h': URL item within the bar. nodeToCopy = root->GetChild(1); - model.Copy(nodeToCopy, root, 6); + model_->Copy(nodeToCopy, root, 6); actualModelString = model_test_utils::ModelStringFromNode(root); EXPECT_EQ("a d 1:[ b d c ] d 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] h d ", actualModelString); // Copy '2' to be after 'a': Folder within the bar. nodeToCopy = root->GetChild(4); - model.Copy(nodeToCopy, root, 1); + model_->Copy(nodeToCopy, root, 1); actualModelString = model_test_utils::ModelStringFromNode(root); EXPECT_EQ("a 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] d 1:[ b d c ] " "d 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] h d ", @@ -374,34 +407,45 @@ TEST_F(BookmarkModelTest, Copy) { // Tests that adding a URL to a folder updates the last modified time. TEST_F(BookmarkModelTest, ParentForNewNodes) { - ASSERT_EQ(model.GetBookmarkBarNode(), model.GetParentForNewNodes()); + ASSERT_EQ(model_->GetBookmarkBarNode(), model_->GetParentForNewNodes()); + + const string16 title(ASCIIToUTF16("foo")); + const GURL url("http://foo.com"); + + model_->AddURL(model_->other_node(), 0, title, url); + ASSERT_EQ(model_->other_node(), model_->GetParentForNewNodes()); +} + +// Tests that adding a URL to a folder updates the last modified time. +TEST_F(BookmarkModelTest, ParentForNewSyncedNodes) { + ASSERT_EQ(model_->GetBookmarkBarNode(), model_->GetParentForNewNodes()); const string16 title(ASCIIToUTF16("foo")); const GURL url("http://foo.com"); - model.AddURL(model.other_node(), 0, title, url); - ASSERT_EQ(model.other_node(), model.GetParentForNewNodes()); + model_->AddURL(model_->synced_node(), 0, title, url); + ASSERT_EQ(model_->synced_node(), model_->GetParentForNewNodes()); } // Make sure recently modified stays in sync when adding a URL. TEST_F(BookmarkModelTest, MostRecentlyModifiedFolders) { // Add a folder. - const BookmarkNode* folder = model.AddFolder(model.other_node(), 0, - ASCIIToUTF16("foo")); + const BookmarkNode* folder = model_->AddFolder(model_->other_node(), 0, + ASCIIToUTF16("foo")); // Add a URL to it. - model.AddURL(folder, 0, ASCIIToUTF16("blah"), GURL("http://foo.com")); + model_->AddURL(folder, 0, ASCIIToUTF16("blah"), GURL("http://foo.com")); // Make sure folder is in the most recently modified. std::vector<const BookmarkNode*> most_recent_folders = - bookmark_utils::GetMostRecentlyModifiedFolders(&model, 1); + bookmark_utils::GetMostRecentlyModifiedFolders(model_.get(), 1); ASSERT_EQ(1U, most_recent_folders.size()); ASSERT_EQ(folder, most_recent_folders[0]); // Nuke the folder and do another fetch, making sure folder isn't in the // returned list. - model.Remove(folder->parent(), 0); + model_->Remove(folder->parent(), 0); most_recent_folders = - bookmark_utils::GetMostRecentlyModifiedFolders(&model, 1); + bookmark_utils::GetMostRecentlyModifiedFolders(model_.get(), 1); ASSERT_EQ(1U, most_recent_folders.size()); ASSERT_TRUE(most_recent_folders[0] != folder); } @@ -411,19 +455,19 @@ TEST_F(BookmarkModelTest, MostRecentlyAddedEntries) { // Add a couple of nodes such that the following holds for the time of the // nodes: n1 > n2 > n3 > n4. Time base_time = Time::Now(); - BookmarkNode* n1 = AsMutable(model.AddURL(model.GetBookmarkBarNode(), + BookmarkNode* n1 = AsMutable(model_->AddURL(model_->GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), GURL("http://foo.com/0"))); - BookmarkNode* n2 = AsMutable(model.AddURL(model.GetBookmarkBarNode(), + BookmarkNode* n2 = AsMutable(model_->AddURL(model_->GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), GURL("http://foo.com/1"))); - BookmarkNode* n3 = AsMutable(model.AddURL(model.GetBookmarkBarNode(), + BookmarkNode* n3 = AsMutable(model_->AddURL(model_->GetBookmarkBarNode(), 2, ASCIIToUTF16("blah"), GURL("http://foo.com/2"))); - BookmarkNode* n4 = AsMutable(model.AddURL(model.GetBookmarkBarNode(), + BookmarkNode* n4 = AsMutable(model_->AddURL(model_->GetBookmarkBarNode(), 3, ASCIIToUTF16("blah"), GURL("http://foo.com/3"))); @@ -434,7 +478,7 @@ TEST_F(BookmarkModelTest, MostRecentlyAddedEntries) { // Make sure order is honored. std::vector<const BookmarkNode*> recently_added; - bookmark_utils::GetMostRecentlyAddedEntries(&model, 2, &recently_added); + bookmark_utils::GetMostRecentlyAddedEntries(model_.get(), 2, &recently_added); ASSERT_EQ(2U, recently_added.size()); ASSERT_TRUE(n1 == recently_added[0]); ASSERT_TRUE(n2 == recently_added[1]); @@ -442,7 +486,7 @@ TEST_F(BookmarkModelTest, MostRecentlyAddedEntries) { // swap 1 and 2, then check again. recently_added.clear(); SwapDateAdded(n1, n2); - bookmark_utils::GetMostRecentlyAddedEntries(&model, 4, &recently_added); + bookmark_utils::GetMostRecentlyAddedEntries(model_.get(), 4, &recently_added); ASSERT_EQ(4U, recently_added.size()); ASSERT_TRUE(n2 == recently_added[0]); ASSERT_TRUE(n1 == recently_added[1]); @@ -456,38 +500,38 @@ TEST_F(BookmarkModelTest, GetMostRecentlyAddedNodeForURL) { // nodes: n1 > n2 Time base_time = Time::Now(); const GURL url("http://foo.com/0"); - BookmarkNode* n1 = AsMutable(model.AddURL( - model.GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), url)); - BookmarkNode* n2 = AsMutable(model.AddURL( - model.GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), url)); + BookmarkNode* n1 = AsMutable(model_->AddURL( + model_->GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), url)); + BookmarkNode* n2 = AsMutable(model_->AddURL( + model_->GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), url)); n1->set_date_added(base_time + TimeDelta::FromDays(4)); n2->set_date_added(base_time + TimeDelta::FromDays(3)); // Make sure order is honored. - ASSERT_EQ(n1, model.GetMostRecentlyAddedNodeForURL(url)); + ASSERT_EQ(n1, model_->GetMostRecentlyAddedNodeForURL(url)); // swap 1 and 2, then check again. SwapDateAdded(n1, n2); - ASSERT_EQ(n2, model.GetMostRecentlyAddedNodeForURL(url)); + ASSERT_EQ(n2, model_->GetMostRecentlyAddedNodeForURL(url)); } // Makes sure GetBookmarks removes duplicates. TEST_F(BookmarkModelTest, GetBookmarksWithDups) { const GURL url("http://foo.com/0"); - model.AddURL(model.GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), url); - model.AddURL(model.GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), url); + model_->AddURL(model_->GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), url); + model_->AddURL(model_->GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), url); std::vector<GURL> urls; - model.GetBookmarks(&urls); + model_->GetBookmarks(&urls); EXPECT_EQ(1U, urls.size()); ASSERT_TRUE(urls[0] == url); } TEST_F(BookmarkModelTest, HasBookmarks) { const GURL url("http://foo.com/"); - model.AddURL(model.GetBookmarkBarNode(), 0, ASCIIToUTF16("bar"), url); + model_->AddURL(model_->GetBookmarkBarNode(), 0, ASCIIToUTF16("bar"), url); - EXPECT_TRUE(model.HasBookmarks()); + EXPECT_TRUE(model_->HasBookmarks()); } namespace { @@ -527,8 +571,8 @@ class StarredListener : public NotificationObserver { TEST_F(BookmarkModelTest, NotifyURLsStarred) { StarredListener listener; const GURL url("http://foo.com/0"); - const BookmarkNode* n1 = model.AddURL( - model.GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), url); + const BookmarkNode* n1 = model_->AddURL( + model_->GetBookmarkBarNode(), 0, ASCIIToUTF16("blah"), url); // Starred notification should be sent. EXPECT_EQ(1, listener.notification_count_); @@ -540,23 +584,23 @@ TEST_F(BookmarkModelTest, NotifyURLsStarred) { // Add another bookmark for the same URL. This should not send any // notification. - const BookmarkNode* n2 = model.AddURL( - model.GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), url); + const BookmarkNode* n2 = model_->AddURL( + model_->GetBookmarkBarNode(), 1, ASCIIToUTF16("blah"), url); EXPECT_EQ(0, listener.notification_count_); // Remove n2. - model.Remove(n2->parent(), 1); + model_->Remove(n2->parent(), 1); n2 = NULL; // Shouldn't have received any notification as n1 still exists with the same // URL. EXPECT_EQ(0, listener.notification_count_); - EXPECT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == n1); + EXPECT_TRUE(model_->GetMostRecentlyAddedNodeForURL(url) == n1); // Remove n1. - model.Remove(n1->parent(), 0); + model_->Remove(n1->parent(), 0); // Now we should get the notification. EXPECT_EQ(1, listener.notification_count_); @@ -760,6 +804,8 @@ TEST_F(BookmarkModelTestWithProfile, CreateAndRestore) { const std::string bbn_contents; // Structure of the children of the other node. const std::string other_contents; + // Structure of the children of the synced node. + const std::string synced_contents; } data[] = { // See PopulateNodeFromString for a description of these strings. { "", "" }, @@ -787,11 +833,16 @@ TEST_F(BookmarkModelTestWithProfile, CreateAndRestore) { PopulateNodeFromString(data[i].other_contents, &other); PopulateBookmarkNode(&other, bb_model_, bb_model_->other_node()); + TestNode synced; + PopulateNodeFromString(data[i].synced_contents, &synced); + PopulateBookmarkNode(&synced, bb_model_, bb_model_->synced_node()); + profile_->CreateBookmarkModel(false); BlockTillBookmarkModelLoaded(); VerifyModelMatchesNode(&bbn, bb_model_->GetBookmarkBarNode()); VerifyModelMatchesNode(&other, bb_model_->other_node()); + VerifyModelMatchesNode(&synced, bb_model_->synced_node()); VerifyNoDuplicateIDs(bb_model_); } } @@ -861,6 +912,9 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile { ASSERT_TRUE(child->GetURL() == GURL("http://www.google.com/intl/en/about.html")); + parent = bb_model_->synced_node(); + ASSERT_EQ(0, parent->child_count()); + ASSERT_TRUE(bb_model_->IsBookmarked(GURL("http://www.google.com"))); } @@ -970,8 +1024,8 @@ TEST_F(BookmarkModelTest, Sort) { // 'C' and 'a' are folders. TestNode bbn; PopulateNodeFromString("B [ a ] d [ a ]", &bbn); - const BookmarkNode* parent = model.GetBookmarkBarNode(); - PopulateBookmarkNode(&bbn, &model, parent); + const BookmarkNode* parent = model_->GetBookmarkBarNode(); + PopulateBookmarkNode(&bbn, model_.get(), parent); BookmarkNode* child1 = AsMutable(parent->GetChild(1)); child1->set_title(ASCIIToUTF16("a")); @@ -983,7 +1037,7 @@ TEST_F(BookmarkModelTest, Sort) { ClearCounts(); // Sort the children of the bookmark bar node. - model.SortChildren(parent); + model_->SortChildren(parent); // Make sure we were notified. AssertObserverCount(0, 0, 0, 0, 1); diff --git a/chrome/browser/bookmarks/bookmark_storage.cc b/chrome/browser/bookmarks/bookmark_storage.cc index 2bb6305..050ee115 100644 --- a/chrome/browser/bookmarks/bookmark_storage.cc +++ b/chrome/browser/bookmarks/bookmark_storage.cc @@ -69,7 +69,7 @@ class BookmarkStorage::LoadTask : public Task { BookmarkCodec codec; TimeTicks start_time = TimeTicks::Now(); codec.Decode(details_->bb_node(), details_->other_folder_node(), - &max_node_id, *root.get()); + details_->synced_folder_node(), &max_node_id, *root.get()); details_->set_max_id(std::max(max_node_id, details_->max_id())); details_->set_computed_checksum(codec.computed_checksum()); details_->set_stored_checksum(codec.stored_checksum()); @@ -80,6 +80,7 @@ class BookmarkStorage::LoadTask : public Task { start_time = TimeTicks::Now(); AddBookmarksToIndex(details_->bb_node()); AddBookmarksToIndex(details_->other_folder_node()); + AddBookmarksToIndex(details_->synced_folder_node()); UMA_HISTOGRAM_TIMES("Bookmarks.CreateBookmarkIndexTime", TimeTicks::Now() - start_time); } @@ -115,10 +116,12 @@ class BookmarkStorage::LoadTask : public Task { BookmarkLoadDetails::BookmarkLoadDetails(BookmarkNode* bb_node, BookmarkNode* other_folder_node, + BookmarkNode* synced_folder_node, BookmarkIndex* index, int64 max_id) : bb_node_(bb_node), other_folder_node_(other_folder_node), + synced_folder_node_(synced_folder_node), index_(index), max_id_(max_id), ids_reassigned_(false) { diff --git a/chrome/browser/bookmarks/bookmark_storage.h b/chrome/browser/bookmarks/bookmark_storage.h index cf1d4b1..847dafd 100644 --- a/chrome/browser/bookmarks/bookmark_storage.h +++ b/chrome/browser/bookmarks/bookmark_storage.h @@ -30,12 +30,17 @@ class BookmarkLoadDetails { public: BookmarkLoadDetails(BookmarkNode* bb_node, BookmarkNode* other_folder_node, + BookmarkNode* synced_folder_node, BookmarkIndex* index, int64 max_id); ~BookmarkLoadDetails(); BookmarkNode* bb_node() { return bb_node_.get(); } BookmarkNode* release_bb_node() { return bb_node_.release(); } + BookmarkNode* synced_folder_node() { return synced_folder_node_.get(); } + BookmarkNode* release_synced_folder_node() { + return synced_folder_node_.release(); + } BookmarkNode* other_folder_node() { return other_folder_node_.get(); } BookmarkNode* release_other_folder_node() { return other_folder_node_.release(); @@ -66,6 +71,7 @@ class BookmarkLoadDetails { private: scoped_ptr<BookmarkNode> bb_node_; scoped_ptr<BookmarkNode> other_folder_node_; + scoped_ptr<BookmarkNode> synced_folder_node_; scoped_ptr<BookmarkIndex> index_; int64 max_id_; std::string computed_checksum_; diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc index 22a2d21..aed5d4b 100644 --- a/chrome/browser/bookmarks/bookmark_utils.cc +++ b/chrome/browser/bookmarks/bookmark_utils.cc @@ -479,6 +479,17 @@ std::vector<const BookmarkNode*> GetMostRecentlyModifiedFolders( if (nodes.size() < max_count && find(nodes.begin(), nodes.end(), model->other_node()) == nodes.end()) { nodes.push_back(model->other_node()); + + if (nodes.size() < max_count && + find(nodes.begin(), nodes.end(), + model->synced_node()) == nodes.end()) { + nodes.push_back(model->synced_node()); + if (nodes.size() < max_count && + std::find(nodes.begin(), nodes.end(), + model->synced_node()) == nodes.end()) { + nodes.push_back(model->synced_node()); + } + } } } return nodes; diff --git a/chrome/browser/bookmarks/recently_used_folders_combo_model.cc b/chrome/browser/bookmarks/recently_used_folders_combo_model.cc index 3bc18e2..c625d2d 100644 --- a/chrome/browser/bookmarks/recently_used_folders_combo_model.cc +++ b/chrome/browser/bookmarks/recently_used_folders_combo_model.cc @@ -4,7 +4,9 @@ #include "chrome/browser/bookmarks/recently_used_folders_combo_model.h" +#include "base/command_line.h" #include "chrome/browser/bookmarks/bookmark_utils.h" +#include "chrome/common/chrome_switches.h" #include "grit/generated_resources.h" #include "ui/base/l10n/l10n_util.h" @@ -26,11 +28,13 @@ RecentlyUsedFoldersComboModel::RecentlyUsedFoldersComboModel( // We special case the placement of these, so remove them from the list, then // fix up the order. RemoveNode(model->GetBookmarkBarNode()); + RemoveNode(model->synced_node()); RemoveNode(model->other_node()); RemoveNode(node->parent()); // Make the parent the first item, unless it's the bookmark bar or other node. if (node->parent() != model->GetBookmarkBarNode() && + node->parent() != model->synced_node() && node->parent() != model->other_node()) { nodes_.insert(nodes_.begin(), node->parent()); } @@ -42,6 +46,10 @@ RecentlyUsedFoldersComboModel::RecentlyUsedFoldersComboModel( // And put the bookmark bar and other nodes at the end of the list. nodes_.push_back(model->GetBookmarkBarNode()); nodes_.push_back(model->other_node()); + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder)) { + nodes_.push_back(model->synced_node()); + } std::vector<const BookmarkNode*>::iterator it = std::find(nodes_.begin(), nodes_.end(), diff --git a/chrome/browser/extensions/extension_bookmark_helpers.cc b/chrome/browser/extensions/extension_bookmark_helpers.cc index a66839c8..92e5519 100644 --- a/chrome/browser/extensions/extension_bookmark_helpers.cc +++ b/chrome/browser/extensions/extension_bookmark_helpers.cc @@ -92,6 +92,7 @@ bool RemoveNode(BookmarkModel* model, } if (node == model->root_node() || node == model->other_node() || + node == model->synced_node() || node == model->GetBookmarkBarNode()) { *error = keys::kModifySpecialError; return false; diff --git a/chrome/browser/history/history_types.h b/chrome/browser/history/history_types.h index 44c422e..b09e5b2 100644 --- a/chrome/browser/history/history_types.h +++ b/chrome/browser/history/history_types.h @@ -282,6 +282,9 @@ struct StarredEntry { // User created folder. USER_FOLDER, + // The synced folder. + SYNCED, + // The "other bookmarks" folder that holds uncategorized bookmarks. OTHER }; diff --git a/chrome/browser/history/starred_url_database.cc b/chrome/browser/history/starred_url_database.cc index 5336e07..d0cc85c 100644 --- a/chrome/browser/history/starred_url_database.cc +++ b/chrome/browser/history/starred_url_database.cc @@ -66,6 +66,9 @@ void FillInStarredEntry(const sql::Statement& s, StarredEntry* entry) { case 3: entry->type = history::StarredEntry::OTHER; break; + case 4: + entry->type = history::StarredEntry::SYNCED; + break; default: NOTREACHED(); break; @@ -213,6 +216,9 @@ StarID StarredURLDatabase::CreateStarredEntryRow(URLID url_id, case history::StarredEntry::OTHER: statement.BindInt(0, 3); break; + case history::StarredEntry::SYNCED: + statement.BindInt(0, 4); + break; default: NOTREACHED(); } @@ -435,6 +441,29 @@ bool StarredURLDatabase::EnsureStarredIntegrityImpl( return false; } + // Make sure the synced node exists. + StarredNode* synced_node = GetNodeByType(*roots, StarredEntry::SYNCED); + if (!synced_node) { + LOG(WARNING) << "No bookmark synced folder in database"; + StarredEntry entry; + entry.folder_id = GetMaxFolderID() + 1; + // TODO (yfriedman): Is this index right? + if (entry.folder_id == 1) { + NOTREACHED() << "Unable to get new id for synced bookmarks folder"; + return false; + } + entry.id = CreateStarredEntryRow( + 0, entry.folder_id, 0, UTF8ToUTF16("synced"), base::Time::Now(), 0, + history::StarredEntry::SYNCED); + if (!entry.id) { + NOTREACHED() << "Unable to create synced bookmarks folder"; + return false; + } + entry.type = StarredEntry::SYNCED; + StarredNode* synced_node = new StarredNode(entry); + roots->insert(synced_node); + } + // Make sure the other node exists. StarredNode* other_node = GetNodeByType(*roots, StarredEntry::OTHER); if (!other_node) { @@ -545,11 +574,17 @@ bool StarredURLDatabase::MigrateBookmarksToFileImpl(const FilePath& path) { entry.type = history::StarredEntry::OTHER; BookmarkNode other_node(0, GURL()); other_node.Reset(entry); + entry.type = history::StarredEntry::SYNCED; + BookmarkNode synced_node(0, GURL()); + synced_node.Reset(entry); std::map<history::UIStarID, history::StarID> folder_id_to_id_map; typedef std::map<history::StarID, BookmarkNode*> IDToNodeMap; IDToNodeMap id_to_node_map; + history::UIStarID synced_folder_folder_id = 0; + history::StarID synced_folder_id = 0; + history::UIStarID other_folder_folder_id = 0; history::StarID other_folder_id = 0; @@ -562,6 +597,10 @@ bool StarredURLDatabase::MigrateBookmarksToFileImpl(const FilePath& path) { other_folder_id = i->id; other_folder_folder_id = i->folder_id; } + if (i->type == history::StarredEntry::SYNCED) { + synced_folder_id = i->id; + synced_folder_folder_id = i->folder_id; + } } } @@ -573,12 +612,17 @@ bool StarredURLDatabase::MigrateBookmarksToFileImpl(const FilePath& path) { id_to_node_map[other_folder_id] = &other_node; folder_id_to_id_map[other_folder_folder_id] = other_folder_id; } + if (synced_folder_folder_id) { + id_to_node_map[synced_folder_id] = &synced_node; + folder_id_to_id_map[synced_folder_folder_id] = synced_folder_id; + } // Iterate through the entries again creating the nodes. for (std::vector<history::StarredEntry>::iterator i = entries.begin(); i != entries.end(); ++i) { if (!i->parent_folder_id) { DCHECK(i->type == history::StarredEntry::BOOKMARK_BAR || + i->type == history::StarredEntry::SYNCED || i->type == history::StarredEntry::OTHER); // Only entries with no parent should be the bookmark bar and other // bookmarks folders. @@ -616,7 +660,7 @@ bool StarredURLDatabase::MigrateBookmarksToFileImpl(const FilePath& path) { // Save to file. BookmarkCodec encoder; scoped_ptr<Value> encoded_bookmarks( - encoder.Encode(&bookmark_bar_node, &other_node)); + encoder.Encode(&bookmark_bar_node, &other_node, &synced_node)); std::string content; base::JSONWriter::Write(encoded_bookmarks.get(), true, &content); diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc index e05da4e..7677989 100644 --- a/chrome/browser/sync/engine/download_updates_command.cc +++ b/chrome/browser/sync/engine/download_updates_command.cc @@ -6,11 +6,13 @@ #include <string> +#include "base/command_line.h" #include "chrome/browser/sync/engine/syncer.h" #include "chrome/browser/sync/engine/syncer_proto_util.h" #include "chrome/browser/sync/engine/syncproto.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/model_type_payload_map.h" +#include "chrome/common/chrome_switches.h" using syncable::ScopedDirLookup; @@ -33,6 +35,10 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) { ClientToServerMessage::GET_UPDATES); GetUpdatesMessage* get_updates = client_to_server_message.mutable_get_updates(); + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder)) { + get_updates->set_include_syncable_bookmarks(true); + } ScopedDirLookup dir(session->context()->directory_manager(), session->context()->account_name()); diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc index a82291b..8d61778 100644 --- a/chrome/browser/sync/glue/bookmark_change_processor.cc +++ b/chrome/browser/sync/glue/bookmark_change_processor.cc @@ -191,7 +191,8 @@ void BookmarkChangeProcessor::BookmarkNodeChanged(BookmarkModel* model, const BookmarkNode* node) { DCHECK(running()); // We shouldn't see changes to the top-level nodes. - if (node == model->GetBookmarkBarNode() || node == model->other_node()) { + if (node == model->GetBookmarkBarNode() || node == model->other_node() || + node == model->synced_node()) { NOTREACHED() << "Saw update to permanent node!"; return; } @@ -225,7 +226,8 @@ void BookmarkChangeProcessor::BookmarkNodeMoved(BookmarkModel* model, DCHECK(running()); const BookmarkNode* child = new_parent->GetChild(new_index); // We shouldn't see changes to the top-level nodes. - if (child == model->GetBookmarkBarNode() || child == model->other_node()) { + if (child == model->GetBookmarkBarNode() || child == model->other_node() || + child == model->synced_node()) { NOTREACHED() << "Saw update to permanent node!"; return; } @@ -384,7 +386,8 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel( model_associator_->GetChromeNodeFromSyncId(changes[i].id); // Ignore changes to the permanent top-level nodes. We only care about // their children. - if ((dst == model->GetBookmarkBarNode()) || (dst == model->other_node())) + if ((dst == model->GetBookmarkBarNode()) || (dst==model->other_node()) || + (dst == model->synced_node())) continue; if (changes[i].action == sync_api::SyncManager::ChangeRecord::ACTION_DELETE) { diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc index c839501..c01829b 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.cc +++ b/chrome/browser/sync/glue/bookmark_model_associator.cc @@ -6,6 +6,7 @@ #include <stack> +#include "base/command_line.h" #include "base/hash_tables.h" #include "base/message_loop.h" #include "base/task.h" @@ -17,6 +18,7 @@ #include "chrome/browser/sync/syncable/autofill_migration.h" #include "chrome/browser/sync/syncable/nigori_util.h" #include "chrome/browser/sync/util/cryptographer.h" +#include "chrome/common/chrome_switches.h" #include "content/browser/browser_thread.h" namespace browser_sync { @@ -40,6 +42,7 @@ namespace browser_sync { // TODO(ncarter): Pull these tags from an external protocol specification // rather than hardcoding them here. static const char kBookmarkBarTag[] = "bookmark_bar"; +static const char kSyncedBookmarksTag[] = "synced_bookmarks"; static const char kOtherBookmarksTag[] = "other_bookmarks"; // Bookmark comparer for map of bookmark nodes. @@ -232,6 +235,7 @@ void BookmarkModelAssociator::Disassociate(int64 sync_id) { bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { DCHECK(has_nodes); *has_nodes = false; + bool has_synced_folder = true; int64 bookmark_bar_sync_id; if (!GetSyncIdForTaggedNode(kBookmarkBarTag, &bookmark_bar_sync_id)) { return false; @@ -240,6 +244,10 @@ bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { if (!GetSyncIdForTaggedNode(kOtherBookmarksTag, &other_bookmarks_sync_id)) { return false; } + int64 synced_bookmarks_sync_id; + if (!GetSyncIdForTaggedNode(kSyncedBookmarksTag, &synced_bookmarks_sync_id)) { + has_synced_folder = false; + } sync_api::ReadTransaction trans(user_share_); @@ -253,10 +261,18 @@ bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { return false; } + sync_api::ReadNode synced_bookmarks_node(&trans); + if (has_synced_folder && + !synced_bookmarks_node.InitByIdLookup(synced_bookmarks_sync_id)) { + return false; + } + // Sync model has user created nodes if either one of the permanent nodes // has children. *has_nodes = bookmark_bar_node.GetFirstChildId() != sync_api::kInvalidId || - other_bookmarks_node.GetFirstChildId() != sync_api::kInvalidId; + other_bookmarks_node.GetFirstChildId() != sync_api::kInvalidId || + (has_synced_folder && + synced_bookmarks_node.GetFirstChildId() != sync_api::kInvalidId); return true; } @@ -346,14 +362,32 @@ bool BookmarkModelAssociator::BuildAssociations() { << "are running against an out-of-date server?"; return false; } + // We only need to ensure that the "synced bookmarks" folder exists on the + // server if the command line flag is set. + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder) && + !AssociateTaggedPermanentNode(bookmark_model_->synced_node(), + kSyncedBookmarksTag)) { + LOG(ERROR) << "Server did not create top-level synced nodes. Possibly " + << "we are running against an out-of-date server?"; + return false; + } int64 bookmark_bar_sync_id = GetSyncIdFromChromeId( bookmark_model_->GetBookmarkBarNode()->id()); DCHECK(bookmark_bar_sync_id != sync_api::kInvalidId); int64 other_bookmarks_sync_id = GetSyncIdFromChromeId( bookmark_model_->other_node()->id()); DCHECK(other_bookmarks_sync_id != sync_api::kInvalidId); + int64 synced_bookmarks_sync_id = GetSyncIdFromChromeId( + bookmark_model_->synced_node()->id()); + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder)) { + DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId); + } std::stack<int64> dfs_stack; + if (synced_bookmarks_sync_id != sync_api::kInvalidId) + dfs_stack.push(synced_bookmarks_sync_id); dfs_stack.push(other_bookmarks_sync_id); dfs_stack.push(bookmark_bar_sync_id); @@ -486,14 +520,24 @@ bool BookmarkModelAssociator::LoadAssociations() { // We should always be able to find the permanent nodes. return false; } + int64 synced_bookmarks_id = -1; + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder) && + !GetSyncIdForTaggedNode(kSyncedBookmarksTag, &synced_bookmarks_id)) { + // We should always be able to find the permanent nodes. + return false; + } // Build a bookmark node ID index since we are going to repeatedly search for // bookmark nodes by their IDs. BookmarkNodeIdIndex id_index; id_index.AddAll(bookmark_model_->GetBookmarkBarNode()); id_index.AddAll(bookmark_model_->other_node()); + id_index.AddAll(bookmark_model_->synced_node()); std::stack<int64> dfs_stack; + if (synced_bookmarks_id != -1) + dfs_stack.push(synced_bookmarks_id); dfs_stack.push(other_bookmarks_id); dfs_stack.push(bookmark_bar_id); @@ -522,6 +566,7 @@ bool BookmarkModelAssociator::LoadAssociations() { // Don't try to call NodesMatch on permanent nodes like bookmark bar and // other bookmarks. They are not expected to match. if (node != bookmark_model_->GetBookmarkBarNode() && + node != bookmark_model_->synced_node() && node != bookmark_model_->other_node() && !NodesMatch(node, &sync_parent)) return false; diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc index ee82421..5f6230e 100644 --- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc @@ -9,6 +9,7 @@ #include <stack> #include <vector> +#include "base/command_line.h" #include "base/file_path.h" #include "base/file_util.h" #include "base/memory/scoped_ptr.h" @@ -23,6 +24,7 @@ #include "chrome/browser/sync/glue/bookmark_change_processor.h" #include "chrome/browser/sync/glue/bookmark_model_associator.h" #include "chrome/browser/sync/syncable/directory_manager.h" +#include "chrome/common/chrome_switches.h" #include "chrome/test/sync/engine/test_id_factory.h" #include "chrome/test/sync/engine/test_user_share.h" #include "chrome/test/testing_profile.h" @@ -291,6 +293,8 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { virtual void SetUp() { test_user_share_.SetUp(); + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kEnableSyncedBookmarksFolder); } virtual void TearDown() { @@ -360,6 +364,7 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { ASSERT_TRUE(InitSyncNodeFromChromeNode(bnode, &gnode)); // Non-root node titles and parents must match. if (bnode != model_->GetBookmarkBarNode() && + bnode != model_->synced_node() && bnode != model_->other_node()) { EXPECT_EQ(bnode->GetTitle(), WideToUTF16Hack(gnode.GetTitle())); EXPECT_EQ( @@ -459,6 +464,7 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { const BookmarkNode* root = model_->root_node(); EXPECT_EQ(root->GetIndexOf(model_->GetBookmarkBarNode()), 0); EXPECT_EQ(root->GetIndexOf(model_->other_node()), 1); + EXPECT_EQ(root->GetIndexOf(model_->synced_node()), 2); std::stack<int64> stack; stack.push(bookmark_bar_id()); @@ -481,6 +487,11 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { ExpectModelMatch(&trans); } + int64 synced_bookmarks_id() { + return + model_associator_->GetSyncIdFromChromeId(model_->synced_node()->id()); + } + int64 other_bookmarks_id() { return model_associator_->GetSyncIdFromChromeId(model_->other_node()->id()); @@ -514,6 +525,7 @@ TEST_F(ProfileSyncServiceBookmarkTest, InitialState) { EXPECT_TRUE(other_bookmarks_id()); EXPECT_TRUE(bookmark_bar_id()); + EXPECT_TRUE(synced_bookmarks_id()); ExpectModelMatch(); } @@ -540,6 +552,11 @@ TEST_F(ProfileSyncServiceBookmarkTest, BookmarkModelOperations) { folder, 1, ASCIIToUTF16("Airplanes"), GURL("http://www.easyjet.com/")); ExpectSyncerNodeMatching(url2); ExpectModelMatch(); + // Test addition. + const BookmarkNode* synced_folder = + model_->AddFolder(model_->synced_node(), 0, ASCIIToUTF16("pie")); + ExpectSyncerNodeMatching(synced_folder); + ExpectModelMatch(); // Test modification. model_->SetTitle(url2, ASCIIToUTF16("EasyJet")); @@ -556,6 +573,8 @@ TEST_F(ProfileSyncServiceBookmarkTest, BookmarkModelOperations) { ExpectModelMatch(); model_->Copy(url2, model_->GetBookmarkBarNode(), 0); ExpectModelMatch(); + model_->SetTitle(synced_folder, ASCIIToUTF16("strawberry")); + ExpectModelMatch(); // Test deletion. // Delete a single item. @@ -565,6 +584,8 @@ TEST_F(ProfileSyncServiceBookmarkTest, BookmarkModelOperations) { model_->Remove(folder2->parent(), folder2->parent()->GetIndexOf(folder2)); ExpectModelMatch(); + model_->Remove(model_->synced_node(), 0); + ExpectModelMatch(); } TEST_F(ProfileSyncServiceBookmarkTest, ServerChangeProcessing) { @@ -591,6 +612,8 @@ TEST_F(ProfileSyncServiceBookmarkTest, ServerChangeProcessing) { "scrollbars=0,status=0,toolbar=0,width=300," \ "height=300,resizable');});"); adds.AddURL(L"", javascript_url, other_bookmarks_id(), 0); + int64 u6 = adds.AddURL(L"Sync1", "http://www.syncable.edu/", + synced_bookmarks_id(), 0); std::vector<sync_api::SyncManager::ChangeRecord>::const_iterator it; // The bookmark model shouldn't yet have seen any of the nodes of |adds|. @@ -619,6 +642,8 @@ TEST_F(ProfileSyncServiceBookmarkTest, ServerChangeProcessing) { // Then add u3 after f1. int64 u3_old_parent = mods.ModifyPosition(u3, f2, f1); + std::wstring u6_old_title = mods.ModifyTitle(u6, L"Synced Folder A"); + // Test that the property changes have not yet taken effect. ExpectBrowserNodeTitle(u2, u2_old_title); /* ExpectBrowserNodeURL(u2, u2_old_url); */ @@ -629,6 +654,8 @@ TEST_F(ProfileSyncServiceBookmarkTest, ServerChangeProcessing) { ExpectBrowserNodeParent(u3, u3_old_parent); + ExpectBrowserNodeTitle(u6, u6_old_title); + // Apply the changes. mods.ApplyPendingChanges(change_processor_.get()); @@ -641,6 +668,7 @@ TEST_F(ProfileSyncServiceBookmarkTest, ServerChangeProcessing) { FakeServerChange dels(&trans); dels.Delete(u2); dels.Delete(u3); + dels.Delete(u6); ExpectBrowserNodeKnown(u2); ExpectBrowserNodeKnown(u3); @@ -649,6 +677,7 @@ TEST_F(ProfileSyncServiceBookmarkTest, ServerChangeProcessing) { ExpectBrowserNodeUnknown(u2); ExpectBrowserNodeUnknown(u3); + ExpectBrowserNodeUnknown(u6); ExpectModelMatch(&trans); } @@ -935,23 +964,31 @@ namespace { // | |-- f2u3, http://www.f2u3.com/ // | +-- f2u1, http://www.f2u1.com/ // +-- Other bookmarks -// |-- f3 -// | |-- f3u4, http://www.f3u4.com/ -// | |-- f3u2, http://www.f3u2.com/ -// | |-- f3u3, http://www.f3u3.com/ -// | +-- f3u1, http://www.f3u1.com/ -// |-- u4, http://www.u4.com/ -// |-- u3, http://www.u3.com/ -// --- f4 -// | |-- f4u1, http://www.f4u1.com/ -// | |-- f4u2, http://www.f4u2.com/ -// | |-- f4u3, http://www.f4u3.com/ -// | +-- f4u4, http://www.f4u4.com/ -// |-- dup -// | +-- dupu1, http://www.dupu1.com/ -// +-- dup -// +-- dupu2, http://www.dupu1.com/ -// +// | |-- f3 +// | | |-- f3u4, http://www.f3u4.com/ +// | | |-- f3u2, http://www.f3u2.com/ +// | | |-- f3u3, http://www.f3u3.com/ +// | | +-- f3u1, http://www.f3u1.com/ +// | |-- u4, http://www.u4.com/ +// | |-- u3, http://www.u3.com/ +// | --- f4 +// | | |-- f4u1, http://www.f4u1.com/ +// | | |-- f4u2, http://www.f4u2.com/ +// | | |-- f4u3, http://www.f4u3.com/ +// | | +-- f4u4, http://www.f4u4.com/ +// | |-- dup +// | | +-- dupu1, http://www.dupu1.com/ +// | +-- dup +// | +-- dupu2, http://www.dupu1.com/ +// | +// +-- Synced bookmarks +// |-- f5 +// | |-- f5u1, http://www.f5u1.com/ +// |-- f6 +// | |-- f6u1, http://www.f6u1.com/ +// | |-- f6u2, http://www.f6u2.com/ +// +-- u5, http://www.u5.com/ + static TestData kBookmarkBarChildren[] = { { L"u2", "http://www.u2.com/" }, { L"f1", NULL }, @@ -998,6 +1035,20 @@ static TestData kDup2Children[] = { { L"dupu2", "http://www.dupu2.com/" }, }; +static TestData kSyncedBookmarkChildren[] = { + { L"f5", NULL }, + { L"f6", NULL }, + { L"u5", "http://www.u5.com/" }, +}; +static TestData kF5Children[] = { + { L"f5u1", "http://www.f5u1.com/" }, + { L"f5u2", "http://www.f5u2.com/" }, +}; +static TestData kF6Children[] = { + { L"f6u1", "http://www.f6u1.com/" }, + { L"f6u2", "http://www.f6u2.com/" }, +}; + } // anonymous namespace. void ProfileSyncServiceBookmarkTestWithData::PopulateFromTestData( @@ -1065,6 +1116,17 @@ void ProfileSyncServiceBookmarkTestWithData::WriteTestDataToBookmarkModel() { dup_node = other_bookmarks_node->GetChild(5); PopulateFromTestData(dup_node, kDup2Children, arraysize(kDup2Children)); + const BookmarkNode* synced_bookmarks_node = model_->synced_node(); + PopulateFromTestData(synced_bookmarks_node, + kSyncedBookmarkChildren, + arraysize(kSyncedBookmarkChildren)); + + ASSERT_GE(synced_bookmarks_node->child_count(), 3); + const BookmarkNode* f5_node = synced_bookmarks_node->GetChild(0); + PopulateFromTestData(f5_node, kF5Children, arraysize(kF5Children)); + const BookmarkNode* f6_node = synced_bookmarks_node->GetChild(1); + PopulateFromTestData(f6_node, kF6Children, arraysize(kF6Children)); + ExpectBookmarkModelMatchesTestData(); } @@ -1095,6 +1157,18 @@ void ProfileSyncServiceBookmarkTestWithData:: CompareWithTestData(dup_node, kDup1Children, arraysize(kDup1Children)); dup_node = other_bookmarks_node->GetChild(5); CompareWithTestData(dup_node, kDup2Children, arraysize(kDup2Children)); + + const BookmarkNode* synced_bookmarks_node = model_->synced_node(); + CompareWithTestData(synced_bookmarks_node, + kSyncedBookmarkChildren, + arraysize(kSyncedBookmarkChildren)); + + ASSERT_GE(synced_bookmarks_node->child_count(), 3); + const BookmarkNode* f5_node = synced_bookmarks_node->GetChild(0); + CompareWithTestData(f5_node, kF5Children, arraysize(kF5Children)); + const BookmarkNode* f6_node = synced_bookmarks_node->GetChild(1); + CompareWithTestData(f6_node, kF6Children, arraysize(kF6Children)); + } // Tests persistence of the profile sync service by unloading the @@ -1161,6 +1235,7 @@ TEST_F(ProfileSyncServiceBookmarkTestWithData, MergeWithEmptyBookmarkModel) { LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE); EXPECT_EQ(model_->GetBookmarkBarNode()->child_count(), 0); EXPECT_EQ(model_->other_node()->child_count(), 0); + EXPECT_EQ(model_->synced_node()->child_count(), 0); // Now restart the sync service. Starting it should populate the bookmark // model -- test for consistency. diff --git a/chrome/browser/sync/protocol/sync.proto b/chrome/browser/sync/protocol/sync.proto index 32a8833..be947ad 100644 --- a/chrome/browser/sync/protocol/sync.proto +++ b/chrome/browser/sync/protocol/sync.proto @@ -387,6 +387,9 @@ message GetUpdatesMessage { // containing GetUpdatesStreamingResponse. These ClientToServerResponses are // delimited by a length prefix, which is encoded as a varint. optional bool streaming = 7 [default = false]; + + // Whether to request the syncable_bookmarks permanent item. + optional bool include_syncable_bookmarks = 1000 [default = false]; }; message AuthenticateMessage { diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm index d7d203e..9d7f7a8 100644 --- a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm +++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm @@ -539,6 +539,7 @@ void RecordAppLaunch(Profile* profile, GURL url) { // Don't allow edit/delete of the bar node, or of "Other Bookmarks" if ((node == nil) || (node == bookmarkModel_->other_node()) || + (node == bookmarkModel_->synced_node()) || (node == bookmarkModel_->GetBookmarkBarNode())) return NO; return YES; @@ -800,6 +801,7 @@ void RecordAppLaunch(Profile* profile, GURL url) { BookmarkNode::Type type = senderNode->type(); if (type == BookmarkNode::BOOKMARK_BAR || type == BookmarkNode::OTHER_NODE || + type == BookmarkNode::SYNCED || type == BookmarkNode::FOLDER) { parent = senderNode; newIndex = parent->child_count(); diff --git a/chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk_unittest.cc b/chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk_unittest.cc index 318f246..65c03b7 100644 --- a/chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk_unittest.cc +++ b/chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk_unittest.cc @@ -6,11 +6,13 @@ #include <string> +#include "base/command_line.h" #include "base/string_util.h" #include "base/utf_string_conversions.h" #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.h" #include "chrome/browser/ui/gtk/bookmarks/bookmark_tree_model.h" +#include "chrome/common/chrome_switches.h" #include "chrome/test/testing_profile.h" #include "content/browser/browser_thread.h" #include "testing/gtest/include/gtest/gtest.h" @@ -33,6 +35,8 @@ class BookmarkEditorGtkTest : public testing::Test { } virtual void SetUp() { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kEnableSyncedBookmarksFolder); profile_.reset(new TestingProfile()); profile_->CreateBookmarkModel(true); profile_->BlockUntilBookmarkModelLoaded(); @@ -71,6 +75,8 @@ class BookmarkEditorGtkTest : public testing::Test { // oa // OF1 // of1a + // synced node + // sa void AddTestData() { std::string test_base = base_path(); @@ -89,6 +95,10 @@ class BookmarkEditorGtkTest : public testing::Test { const BookmarkNode* of1 = model_->AddFolder(model_->other_node(), 1, ASCIIToUTF16("OF1")); model_->AddURL(of1, 0, ASCIIToUTF16("of1a"), GURL(test_base + "of1a")); + + // Children of the synced node. + model_->AddURL(model_->synced_node(), 0, ASCIIToUTF16("sa"), + GURL(test_base + "sa")); } }; @@ -106,6 +116,8 @@ TEST_F(BookmarkEditorGtkTest, ModelsMatch) { GtkTreeIter bookmark_bar_node = toplevel; ASSERT_TRUE(gtk_tree_model_iter_next(store, &toplevel)); GtkTreeIter other_node = toplevel; + ASSERT_TRUE(gtk_tree_model_iter_next(store, &toplevel)); + GtkTreeIter synced_node = toplevel; ASSERT_FALSE(gtk_tree_model_iter_next(store, &toplevel)); // The bookmark bar should have 2 nodes: folder F1 and F2. @@ -130,6 +142,9 @@ TEST_F(BookmarkEditorGtkTest, ModelsMatch) { ASSERT_TRUE(gtk_tree_model_iter_children(store, &child, &other_node)); ASSERT_EQ("OF1", UTF16ToUTF8(GetTitleFromTreeIter(store, &child))); ASSERT_FALSE(gtk_tree_model_iter_next(store, &child)); + + // Synced node should have one child (sa). + ASSERT_EQ(0, gtk_tree_model_iter_n_children(store, &synced_node)); } // Changes the title and makes sure parent/visual order doesn't change. diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc index b8e25ef..6b18180 100644 --- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc +++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc @@ -485,9 +485,10 @@ BookmarkEditorView::EditorNode* BookmarkEditorView::CreateRootNode() { EditorNode* root_node = new EditorNode(std::wstring(), 0); const BookmarkNode* bb_root_node = bb_model_->root_node(); CreateNodes(bb_root_node, root_node); - DCHECK(root_node->child_count() == 2); + DCHECK(root_node->child_count() == 3); DCHECK(bb_root_node->GetChild(0)->type() == BookmarkNode::BOOKMARK_BAR); DCHECK(bb_root_node->GetChild(1)->type() == BookmarkNode::OTHER_NODE); + DCHECK(bb_root_node->GetChild(2)->type() == BookmarkNode::SYNCED); return root_node; } diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc index 042dc12..8a63a2d 100644 --- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc +++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc @@ -132,9 +132,9 @@ TEST_F(BookmarkEditorViewTest, ModelsMatch) { CreateEditor(profile_.get(), NULL, BookmarkEditor::EditDetails(), BookmarkEditorView::SHOW_TREE); BookmarkEditorView::EditorNode* editor_root = editor_tree_model()->GetRoot(); - // The root should have two children, one for the bookmark bar node, - // the other for the 'other bookmarks' folder. - ASSERT_EQ(2, editor_root->child_count()); + // The root should have three children: bookmark bar, other bookmarks and + // synced bookmarks. + ASSERT_EQ(3, editor_root->child_count()); BookmarkEditorView::EditorNode* bb_node = editor_root->GetChild(0); // The root should have 2 nodes: folder F1 and F2. diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc index 4c67a94..6325a44 100644 --- a/chrome/common/chrome_switches.cc +++ b/chrome/common/chrome_switches.cc @@ -495,6 +495,9 @@ const char kEnableTabGroupsContextMenu[] = "enable-tab-groups-context-menu"; // Enable syncing browser typed urls. const char kEnableSyncTypedUrls[] = "enable-sync-typed-urls"; +// Enable syncing browser typed urls. +const char kEnableSyncedBookmarksFolder[] = "enable-synced-bookmarks-folder"; + // Enable use of experimental TCP sockets API for sending data in the // SYN packet. const char kEnableTcpFastOpen[] = "enable-tcp-fastopen"; diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h index 9b2bbc1..e720ac1 100644 --- a/chrome/common/chrome_switches.h +++ b/chrome/common/chrome_switches.h @@ -148,6 +148,7 @@ extern const char kEnableSyncAutofill[]; extern const char kEnableSyncPreferences[]; extern const char kEnableSyncSessions[]; extern const char kEnableSyncTypedUrls[]; +extern const char kEnableSyncedBookmarksFolder[]; extern const char kEnableTabGroupsContextMenu[]; extern const char kEnableTcpFastOpen[]; extern const char kEnableTopSites[]; |