diff options
author | yfriedman@chromium.org <yfriedman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-05-26 22:07:17 +0000 |
---|---|---|
committer | yfriedman@chromium.org <yfriedman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-05-26 22:07:17 +0000 |
commit | 6892e2e416250521ed0778206da0093388b99300 (patch) | |
tree | 84d80407cbfa15cec3214d6d2822e74143a79ce6 /chrome/browser/bookmarks | |
parent | 46fe10d6c3cc0433626007f93c2b7784a2889ada (diff) | |
download | chromium_src-6892e2e416250521ed0778206da0093388b99300.zip chromium_src-6892e2e416250521ed0778206da0093388b99300.tar.gz chromium_src-6892e2e416250521ed0778206da0093388b99300.tar.bz2 |
Revert "Revert 84829 - Initial implementation of "Synced Bookmarks" folder."
Second attempt to land this change. Fixes memory leak (82186),
deleted bookmarks (82273) and crash on add bookmarks in windows (82349).
Original desscription:
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=test bookmark addition/moving around in combination with sync
Review URL: http://codereview.chromium.org/7012005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@86902 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/bookmarks')
-rw-r--r-- | chrome/browser/bookmarks/bookmark_codec.cc | 49 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_codec.h | 12 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_codec_unittest.cc | 49 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_html_writer.cc | 23 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_html_writer_unittest.cc | 11 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model.cc | 38 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model.h | 25 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model_test_utils.cc | 4 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model_unittest.cc | 246 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_storage.cc | 5 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_storage.h | 6 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_utils.cc | 6 | ||||
-rw-r--r-- | chrome/browser/bookmarks/recently_used_folders_combo_model.cc | 7 |
13 files changed, 371 insertions, 110 deletions
diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc index dd145b3..56b0c25 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. @@ -147,21 +154,45 @@ bool BookmarkCodec::DecodeHelper(BookmarkNode* bb_node, 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) { + return false; // Invalid type for root folder 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); + + // Fail silently if we can't deserialize synced bookmarks. We can't require + // them to exist in order to be backwards-compatible with older versions of + // chrome. + Value* synced_folder_value; + if (roots_d_value->Get(kSyncedBookmarkFolderNameKey, &synced_folder_value) && + synced_folder_value->GetType() == Value::TYPE_DICTIONARY) { + DecodeNode(*static_cast<DictionaryValue*>(synced_folder_value), NULL, + synced_folder_node); + } else { + // If we didn't find the synced folder, we're almost guaranteed to have a + // duplicate id when we add the synced folder. Consequently, if we don't + // intend to reassign ids in the future (ids_valid_ is still true), then at + // least reassign the synced bookmarks to avoid it colliding with anything + // else. + if (ids_valid_) { + ReassignIDsHelper(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 +320,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..7358188 100644 --- a/chrome/browser/bookmarks/bookmark_codec_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_codec_unittest.cc @@ -2,7 +2,10 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "base/file_path.h" +#include "base/file_util.h" #include "base/memory/scoped_ptr.h" +#include "base/path_service.h" #include "base/string_util.h" #include "base/utf_string_conversions.h" #include "base/values.h" @@ -10,6 +13,8 @@ #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/bookmarks/bookmark_model_test_utils.h" #include "chrome/browser/bookmarks/bookmark_utils.h" +#include "chrome/common/chrome_paths.h" +#include "content/common/json_value_serializer.h" #include "testing/gtest/include/gtest/gtest.h" namespace { @@ -111,6 +116,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 +167,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); } }; @@ -287,3 +294,45 @@ TEST_F(BookmarkCodecTest, PersistIDsTest) { &decoded_model2, true); } + +TEST_F(BookmarkCodecTest, CanDecodeModelWithoutSyncedBookmarks) { + FilePath test_data_directory; + ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_directory)); + FilePath test_file = test_data_directory.AppendASCII( + "bookmarks/model_without_sync.json"); + ASSERT_TRUE(file_util::PathExists(test_file)); + + JSONFileValueSerializer serializer(test_file); + scoped_ptr<Value> root(serializer.Deserialize(NULL, NULL)); + + BookmarkModel decoded_model(NULL); + BookmarkCodec decoder; + ASSERT_TRUE(Decode(&decoder, &decoded_model, *root.get())); + ExpectIDsUnique(&decoded_model); + + const BookmarkNode* bbn = decoded_model.GetBookmarkBarNode(); + ASSERT_EQ(1, bbn->child_count()); + + const BookmarkNode* child = bbn->GetChild(0); + EXPECT_EQ(BookmarkNode::FOLDER, child->type()); + EXPECT_EQ(ASCIIToUTF16("Folder A"), child->GetTitle()); + ASSERT_EQ(1, child->child_count()); + + child = child->GetChild(0); + EXPECT_EQ(BookmarkNode::URL, child->type()); + EXPECT_EQ(ASCIIToUTF16("Bookmark Manager"), child->GetTitle()); + + const BookmarkNode* other = decoded_model.other_node(); + ASSERT_EQ(1, other->child_count()); + + child = other->GetChild(0); + EXPECT_EQ(BookmarkNode::FOLDER, child->type()); + EXPECT_EQ(ASCIIToUTF16("Folder B"), child->GetTitle()); + ASSERT_EQ(1, child->child_count()); + + child = child->GetChild(0); + EXPECT_EQ(BookmarkNode::URL, child->type()); + EXPECT_EQ(ASCIIToUTF16("Get started with Google Chrome"), child->GetTitle()); + + ASSERT_TRUE(decoded_model.synced_node() != NULL); +} 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 2f80465..3b24b3c 100644 --- a/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc @@ -167,6 +167,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"); @@ -204,6 +206,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); @@ -226,8 +229,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()); @@ -239,7 +242,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); @@ -257,4 +260,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 a48ecdf..7f74bc9 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" @@ -61,6 +63,18 @@ void BookmarkNode::InvalidateFavicon() { favicon_ = SkBitmap(); } +bool BookmarkNode::IsVisible() const { + // The synced bookmark folder is invisible if the flag isn't set and there are + // no bookmarks under it. + if (type_ != BookmarkNode::SYNCED || + CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncedBookmarksFolder) || + child_count() > 0) { + return true; + } + return false; +} + void BookmarkNode::Reset(const history::StarredEntry& entry) { DCHECK(entry.type != history::StarredEntry::URL || entry.url == url_); @@ -75,6 +89,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 +141,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 +277,7 @@ void BookmarkModel::SetTitle(const BookmarkNode* node, const string16& title) { if (node->GetTitle() == title) return; - if (node == bookmark_bar_node_ || node == other_node_) { + if (is_permanent_node(node)) { NOTREACHED(); return; } @@ -568,12 +586,14 @@ 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); + root_.Add(synced_node_, 2); { base::AutoLock url_lock(url_lock_); @@ -719,14 +739,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 +856,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..b3c8c8d 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); @@ -109,6 +110,15 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> { bool is_favicon_loaded() const { return loaded_favicon_; } void set_favicon_loaded(bool value) { loaded_favicon_ = value; } + // Accessor method for controlling the visibility of a bookmark node/sub-tree. + // Note that visibility is not propagated down the tree hierarchy so if a + // parent node is marked as invisible, a child node may return "Visible". This + // function is primarily useful when traversing the model to generate a UI + // representation but we may want to suppress some nodes. + // TODO(yfriedman): Remove this when enable-synced-bookmarks-folder is + // no longer a command line flag. + bool IsVisible() const; + HistoryService::Handle favicon_load_handle() const { return favicon_load_handle_; } @@ -193,6 +203,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 +324,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 +335,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 +397,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 +457,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_test_utils.cc b/chrome/browser/bookmarks/bookmark_model_test_utils.cc index 7286a9e..c2894fb 100644 --- a/chrome/browser/bookmarks/bookmark_model_test_utils.cc +++ b/chrome/browser/bookmarks/bookmark_model_test_utils.cc @@ -39,5 +39,7 @@ void BookmarkModelTestUtils::AssertModelsEqual(BookmarkModel* expected, AssertNodesEqual(expected->other_node(), actual->other_node(), check_ids); + AssertNodesEqual(expected->synced_node(), + actual->synced_node(), + check_ids); } - diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc index aec548e..f87445e 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,11 +85,16 @@ class BookmarkModelTest : public TestingBrowserProcessTest, int index2; }; - BookmarkModelTest() : model(NULL) { - model.AddObserver(this); - ClearCounts(); + BookmarkModelTest() : + model_(NULL), + original_command_line_(*CommandLine::ForCurrentProcess()) { + model_.AddObserver(this); + ClearCounts(); } + virtual void TearDown() { + *CommandLine::ForCurrentProcess() = original_command_line_; + } void Loaded(BookmarkModel* model) OVERRIDE { // We never load from the db, so that this should never get invoked. @@ -153,7 +160,9 @@ class BookmarkModelTest : public TestingBrowserProcessTest, ASSERT_EQ(reordered_count, reordered_count_); } - BookmarkModel model; + BookmarkModel model_; + + CommandLine original_command_line_; int moved_count; @@ -169,25 +178,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); @@ -195,17 +211,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); @@ -214,93 +251,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); @@ -312,17 +350,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); @@ -331,42 +369,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 ", @@ -375,34 +413,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_, 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_, 1); ASSERT_EQ(1U, most_recent_folders.size()); ASSERT_TRUE(most_recent_folders[0] != folder); } @@ -412,19 +461,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"))); @@ -435,7 +484,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_, 2, &recently_added); ASSERT_EQ(2U, recently_added.size()); ASSERT_TRUE(n1 == recently_added[0]); ASSERT_TRUE(n2 == recently_added[1]); @@ -443,7 +492,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_, 4, &recently_added); ASSERT_EQ(4U, recently_added.size()); ASSERT_TRUE(n2 == recently_added[0]); ASSERT_TRUE(n1 == recently_added[1]); @@ -457,38 +506,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 { @@ -528,8 +577,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_); @@ -541,23 +590,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_); @@ -762,6 +811,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. { "", "" }, @@ -789,11 +840,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_); } } @@ -863,6 +919,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"))); } @@ -972,8 +1031,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_, parent); BookmarkNode* child1 = AsMutable(parent->GetChild(1)); child1->set_title(ASCIIToUTF16("a")); @@ -985,7 +1044,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); @@ -997,3 +1056,32 @@ TEST_F(BookmarkModelTest, Sort) { EXPECT_EQ(parent->GetChild(2)->GetTitle(), ASCIIToUTF16("B")); EXPECT_EQ(parent->GetChild(3)->GetTitle(), ASCIIToUTF16("d")); } + +TEST_F(BookmarkModelTest, NodeVisibility) { + EXPECT_TRUE(model_.GetBookmarkBarNode()->IsVisible()); + EXPECT_TRUE(model_.other_node()->IsVisible()); + // Sync node invisible by default + EXPECT_FALSE(model_.synced_node()->IsVisible()); + + // Arbitrary node should be visible + TestNode bbn; + PopulateNodeFromString("B", &bbn); + const BookmarkNode* parent = model_.GetBookmarkBarNode(); + PopulateBookmarkNode(&bbn, &model_, parent); + EXPECT_TRUE(parent->GetChild(0)->IsVisible()); +} + +TEST_F(BookmarkModelTest, SyncNodeVisibileIfFlagSet) { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kEnableSyncedBookmarksFolder); + EXPECT_TRUE(model_.synced_node()->IsVisible()); +} + +TEST_F(BookmarkModelTest, SyncNodeVisibileWithChildren) { + const BookmarkNode* root = model_.synced_node(); + const string16 title(ASCIIToUTF16("foo")); + const GURL url("http://foo.com"); + + model_.AddURL(root, 0, title, url); + EXPECT_TRUE(model_.synced_node()->IsVisible()); +} 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 05e1392..19ec586 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 8ad306d..8206cfd 100644 --- a/chrome/browser/bookmarks/bookmark_utils.cc +++ b/chrome/browser/bookmarks/bookmark_utils.cc @@ -479,6 +479,12 @@ std::vector<const BookmarkNode*> GetMostRecentlyModifiedFolders( find(nodes.begin(), nodes.end(), model->other_node()) == nodes.end()) { nodes.push_back(model->other_node()); } + + if (nodes.size() < max_count && model->synced_node()->IsVisible() && + 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..0bc1b7d 100644 --- a/chrome/browser/bookmarks/recently_used_folders_combo_model.cc +++ b/chrome/browser/bookmarks/recently_used_folders_combo_model.cc @@ -26,12 +26,12 @@ 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->other_node()) { + if (!model->is_permanent_node(node)) { nodes_.insert(nodes_.begin(), node->parent()); } @@ -42,6 +42,9 @@ 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 (model->synced_node()->IsVisible()) { + nodes_.push_back(model->synced_node()); + } std::vector<const BookmarkNode*>::iterator it = std::find(nodes_.begin(), nodes_.end(), |