From 3273dceb702908e7a2e7c13488d0bdfcdce2148b Mon Sep 17 00:00:00 2001 From: "nick@chromium.org" Date: Wed, 27 Jan 2010 16:08:08 +0000 Subject: In the sync database, use protobuf-based storage. Drop the old bookmark-only columns. Add getters and setters for BookmarkSpecifics to syncapi as well as syncable entries. Make the datatype be a required property when creating a syncapi node. Add a datatype for the 'google chrome' top level folder. Add database migrations from version 67 to the new schema. Add infrastructure to support migrations generically. Add unit tests for the migrations. Pull a new version of the protobuf library to pick up a fix for a bug that this change exposed (I upstreamed the fix). Fix some example code in the sql helpers so that it would actually compile. BUG=29899,30041 TEST=New unit tests for migrations: unit tests are based on actual database dumps. Additionally, I manually tested 2-client sync using combos of old-protocol servers, new-protocol servers, and initial database versions v67, v68, and v0 (new client). I manually verified that add/edit/delete works in these combination cases. Afterwards I verified (by inspecting the sync databases) that the ModelTypes are consistent across the various migration/protocol paths. Review URL: http://codereview.chromium.org/554066 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@37253 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/sync/engine/all_status.cc | 1 - .../sync/engine/apply_updates_command_unittest.cc | 4 + chrome/browser/sync/engine/build_commit_command.cc | 38 +- .../browser/sync/engine/process_updates_command.cc | 5 +- chrome/browser/sync/engine/syncapi.cc | 170 ++++--- chrome/browser/sync/engine/syncapi.h | 75 ++- chrome/browser/sync/engine/syncer.cc | 12 +- chrome/browser/sync/engine/syncer_unittest.cc | 202 +++++++- chrome/browser/sync/engine/syncer_util.cc | 97 ++-- chrome/browser/sync/engine/syncer_util.h | 2 +- chrome/browser/sync/engine/syncproto.h | 24 + .../browser/sync/engine/verify_updates_command.cc | 5 +- .../browser/sync/glue/bookmark_change_processor.cc | 18 +- .../sync/glue/preference_change_processor.cc | 4 +- .../browser/sync/profile_sync_service_unittest.cc | 7 +- .../sync/syncable/directory_backing_store.cc | 309 ++++++++++-- .../sync/syncable/directory_backing_store.h | 38 ++ .../syncable/directory_backing_store_unittest.cc | 523 +++++++++++++++++++++ chrome/browser/sync/syncable/model_type.h | 16 +- chrome/browser/sync/syncable/syncable.cc | 87 +++- chrome/browser/sync/syncable/syncable.h | 62 +-- chrome/browser/sync/syncable/syncable_columns.h | 10 +- chrome/browser/sync/syncable/syncable_unittest.cc | 20 +- chrome/chrome_tests.gypi | 1 + chrome/common/sqlite_utils.cc | 4 +- chrome/test/sync/engine/mock_server_connection.cc | 29 +- chrome/test/sync/engine/mock_server_connection.h | 14 +- 27 files changed, 1439 insertions(+), 338 deletions(-) create mode 100755 chrome/browser/sync/syncable/directory_backing_store_unittest.cc (limited to 'chrome') diff --git a/chrome/browser/sync/engine/all_status.cc b/chrome/browser/sync/engine/all_status.cc index a9bd051..49ec3ce 100644 --- a/chrome/browser/sync/engine/all_status.cc +++ b/chrome/browser/sync/engine/all_status.cc @@ -14,7 +14,6 @@ #include "chrome/browser/sync/engine/net/server_connection_manager.h" #include "chrome/browser/sync/engine/syncer.h" #include "chrome/browser/sync/engine/syncer_thread.h" -#include "chrome/browser/sync/engine/syncproto.h" #include "chrome/browser/sync/notifier/listener/talk_mediator.h" #include "chrome/browser/sync/protocol/service_constants.h" #include "chrome/browser/sync/sessions/session_state.h" diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc index ed6811f..546adef 100755 --- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc +++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "chrome/browser/sync/engine/apply_updates_command.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/syncable.h" @@ -78,6 +79,9 @@ class ApplyUpdatesCommandTest : public testing::Test, entry.Put(syncable::SERVER_NON_UNIQUE_NAME, item_id); entry.Put(syncable::SERVER_PARENT_ID, Id::CreateFromServerId(parent_id)); entry.Put(syncable::SERVER_IS_DIR, true); + sync_pb::EntitySpecifics default_bookmark_specifics; + default_bookmark_specifics.MutableExtension(sync_pb::bookmark); + entry.Put(syncable::SERVER_SPECIFICS, default_bookmark_specifics); } TestDirectorySetterUpper syncdb_; diff --git a/chrome/browser/sync/engine/build_commit_command.cc b/chrome/browser/sync/engine/build_commit_command.cc index 9544405..b945fd7 100755 --- a/chrome/browser/sync/engine/build_commit_command.cc +++ b/chrome/browser/sync/engine/build_commit_command.cc @@ -22,6 +22,7 @@ using std::vector; using syncable::ExtendedAttribute; using syncable::Id; using syncable::MutableEntry; +using syncable::SPECIFICS; namespace browser_sync { @@ -45,38 +46,26 @@ void BuildCommitCommand::AddExtensionsActivityToMessage( } namespace { -void SetNewStyleBookmarkData(MutableEntry* meta_entry, SyncEntity* sync_entry) { +void SetEntrySpecifics(MutableEntry* meta_entry, SyncEntity* sync_entry) { + // Add the new style extension and the folder bit. + sync_entry->mutable_specifics()->CopyFrom(meta_entry->Get(SPECIFICS)); + sync_entry->set_folder(meta_entry->Get(syncable::IS_DIR)); - // Add the new style extension. Needed for folders as well as bookmarks. - // It'll identify the folder type on the server. - sync_pb::BookmarkSpecifics* bookmark_specifics = sync_entry-> - mutable_specifics()->MutableExtension(sync_pb::bookmark); - - if (!meta_entry->Get(syncable::IS_DIR)) { - // Set new-style extended bookmark data. - string bookmark_url = meta_entry->Get(syncable::BOOKMARK_URL); - bookmark_specifics->set_url(bookmark_url); - SyncerProtoUtil::CopyBlobIntoProtoBytes( - meta_entry->Get(syncable::BOOKMARK_FAVICON), - bookmark_specifics->mutable_favicon()); - } else { - sync_entry->set_folder(true); - } + DCHECK(meta_entry->GetModelType() == sync_entry->GetModelType()); } void SetOldStyleBookmarkData(MutableEntry* meta_entry, SyncEntity* sync_entry) { + DCHECK(meta_entry->Get(SPECIFICS).HasExtension(sync_pb::bookmark)); // Old-style inlined bookmark data. sync_pb::SyncEntity_BookmarkData* bookmark = sync_entry->mutable_bookmarkdata(); if (!meta_entry->Get(syncable::IS_DIR)) { - string bookmark_url = meta_entry->Get(syncable::BOOKMARK_URL); - bookmark->set_bookmark_url(bookmark_url); - SyncerProtoUtil::CopyBlobIntoProtoBytes( - meta_entry->Get(syncable::BOOKMARK_FAVICON), - bookmark->mutable_bookmark_favicon()); - + const sync_pb::BookmarkSpecifics& bookmark_specifics = + meta_entry->Get(SPECIFICS).GetExtension(sync_pb::bookmark); + bookmark->set_bookmark_url(bookmark_specifics.url()); + bookmark->set_bookmark_favicon(bookmark_specifics.favicon()); bookmark->set_bookmark_folder(false); } else { bookmark->set_bookmark_folder(true); @@ -172,8 +161,7 @@ void BuildCommitCommand::ExecuteImpl(SyncSession* session) { // Deletion is final on the server, let's move things and then delete them. if (meta_entry.Get(syncable::IS_DEL)) { sync_entry->set_deleted(true); - } else if (meta_entry.Get(syncable::IS_BOOKMARK_OBJECT)) { - + } else if (meta_entry.Get(SPECIFICS).HasExtension(sync_pb::bookmark)) { // Common data in both new and old protocol. const Id& prev_id = meta_entry.Get(syncable::PREV_ID); string prev_string = prev_id.IsRoot() ? string() : prev_id.GetServerId(); @@ -183,7 +171,7 @@ void BuildCommitCommand::ExecuteImpl(SyncSession* session) { // over the wire; instead, when deployed servers are able to accept // the new-style scheme, we should abandon the old way. SetOldStyleBookmarkData(&meta_entry, sync_entry); - SetNewStyleBookmarkData(&meta_entry, sync_entry); + SetEntrySpecifics(&meta_entry, sync_entry); } } session->status_controller()->mutable_commit_message()->CopyFrom(message); diff --git a/chrome/browser/sync/engine/process_updates_command.cc b/chrome/browser/sync/engine/process_updates_command.cc index eee2565..a54481f 100755 --- a/chrome/browser/sync/engine/process_updates_command.cc +++ b/chrome/browser/sync/engine/process_updates_command.cc @@ -121,15 +121,14 @@ bool ReverifyEntry(syncable::WriteTransaction* trans, const SyncEntity& entry, const bool deleted = entry.has_deleted() && entry.deleted(); const bool is_directory = entry.IsFolder(); - const bool is_bookmark = - SyncerUtil::GetModelType(entry) == syncable::BOOKMARKS; + const syncable::ModelType model_type = entry.GetModelType(); return VERIFY_SUCCESS == SyncerUtil::VerifyUpdateConsistency(trans, entry, same_id, deleted, is_directory, - is_bookmark); + model_type); } } // namespace diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index 3f0ad2d..8facc52 100755 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -47,6 +47,7 @@ #include "chrome/browser/sync/engine/syncer_thread.h" #include "chrome/browser/sync/notifier/listener/talk_mediator.h" #include "chrome/browser/sync/notifier/listener/talk_mediator_impl.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/protocol/service_constants.h" #include "chrome/browser/sync/sessions/sync_session_context.h" #include "chrome/browser/sync/syncable/directory_manager.h" @@ -82,6 +83,7 @@ using std::string; using std::vector; using syncable::Directory; using syncable::DirectoryManager; +using syncable::SPECIFICS; typedef GoogleServiceAuthError AuthError; @@ -400,20 +402,9 @@ struct UserShare { //////////////////////////////////// // BaseNode member definitions. -// BaseNode::BaseNodeInternal provides storage for member Get() functions that -// need to return pointers (e.g. strings). -struct BaseNode::BaseNodeInternal { - GURL url; - std::wstring title; - Directory::ChildHandles child_handles; - syncable::Blob favicon; -}; +BaseNode::BaseNode() {} -BaseNode::BaseNode() : data_(new BaseNode::BaseNodeInternal) {} - -BaseNode::~BaseNode() { - delete data_; -} +BaseNode::~BaseNode() {} int64 BaseNode::GetParentId() const { return IdToMetahandle(GetTransaction()->GetWrappedTrans(), @@ -428,26 +419,14 @@ bool BaseNode::GetIsFolder() const { return GetEntry()->Get(syncable::IS_DIR); } -const std::wstring& BaseNode::GetTitle() const { - ServerNameToSyncAPIName(GetEntry()->Get(syncable::NON_UNIQUE_NAME), - &data_->title); - return data_->title; +std::wstring BaseNode::GetTitle() const { + std::wstring result; + ServerNameToSyncAPIName(GetEntry()->Get(syncable::NON_UNIQUE_NAME), &result); + return result; } -const GURL& BaseNode::GetURL() const { - GURL url(GetEntry()->Get(syncable::BOOKMARK_URL)); - url.Swap(&data_->url); - return data_->url; -} - -const int64* BaseNode::GetChildIds(size_t* child_count) const { - DCHECK(child_count); - Directory* dir = GetTransaction()->GetLookup(); - dir->GetChildHandles(GetTransaction()->GetWrappedTrans(), - GetEntry()->Get(syncable::ID), &data_->child_handles); - - *child_count = data_->child_handles.size(); - return (data_->child_handles.empty()) ? NULL : &data_->child_handles[0]; +GURL BaseNode::GetURL() const { + return GURL(GetBookmarkSpecifics().url()); } int64 BaseNode::GetPredecessorId() const { @@ -474,19 +453,28 @@ int64 BaseNode::GetFirstChildId() const { return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string); } -const unsigned char* BaseNode::GetFaviconBytes(size_t* size_in_bytes) { - data_->favicon = GetEntry()->Get(syncable::BOOKMARK_FAVICON); - *size_in_bytes = data_->favicon.size(); - if (*size_in_bytes) - return &(data_->favicon[0]); - else - return NULL; +void BaseNode::GetFaviconBytes(std::vector* output) const { + if (!output) + return; + const std::string& favicon = GetBookmarkSpecifics().favicon(); + output->assign(reinterpret_cast(favicon.data()), + reinterpret_cast(favicon.data() + + favicon.length())); } int64 BaseNode::GetExternalId() const { return GetEntry()->Get(syncable::LOCAL_EXTERNAL_ID); } +const sync_pb::BookmarkSpecifics& BaseNode::GetBookmarkSpecifics() const { + DCHECK(GetModelType() == syncable::BOOKMARKS); + return GetEntry()->Get(SPECIFICS).GetExtension(sync_pb::bookmark); +} + +syncable::ModelType BaseNode::GetModelType() const { + return GetEntry()->GetModelType(); +} + //////////////////////////////////// // WriteNode member definitions void WriteNode::SetIsFolder(bool folder) { @@ -511,11 +499,32 @@ void WriteNode::SetTitle(const std::wstring& title) { } void WriteNode::SetURL(const GURL& url) { - const std::string& url_string = url.spec(); - if (url_string == entry_->Get(syncable::BOOKMARK_URL)) - return; // Skip redundant changes. + sync_pb::BookmarkSpecifics new_value = GetBookmarkSpecifics(); + new_value.set_url(url.spec()); + SetBookmarkSpecifics(new_value); +} + +void WriteNode::SetBookmarkSpecifics( + const sync_pb::BookmarkSpecifics& new_value) { + DCHECK(GetModelType() == syncable::BOOKMARKS); + PutBookmarkSpecificsAndMarkForSyncing(new_value); +} + +void WriteNode::PutBookmarkSpecificsAndMarkForSyncing( + const sync_pb::BookmarkSpecifics& new_value) { + sync_pb::EntitySpecifics entity_specifics; + entity_specifics.MutableExtension(sync_pb::bookmark)->CopyFrom(new_value); + PutSpecificsAndMarkForSyncing(entity_specifics); +} - entry_->Put(syncable::BOOKMARK_URL, url_string); +void WriteNode::PutSpecificsAndMarkForSyncing( + const sync_pb::EntitySpecifics& specifics) { + // Skip redundant changes. + if (specifics.SerializeAsString() == + entry_->Get(SPECIFICS).SerializeAsString()) { + return; + } + entry_->Put(SPECIFICS, specifics); MarkForSyncing(); } @@ -545,7 +554,8 @@ bool WriteNode::InitByIdLookup(int64 id) { // Create a new node with default properties, and bind this WriteNode to it. // Return true on success. -bool WriteNode::InitByCreation(const BaseNode& parent, +bool WriteNode::InitByCreation(syncable::ModelType model_type, + const BaseNode& parent, const BaseNode* predecessor) { DCHECK(!entry_) << "Init called twice"; // |predecessor| must be a child of |parent| or NULL. @@ -568,9 +578,18 @@ bool WriteNode::InitByCreation(const BaseNode& parent, // Entries are untitled folders by default. entry_->Put(syncable::IS_DIR, true); - // TODO(ncarter): Naming this bit IS_BOOKMARK_OBJECT is a bit unfortunate, - // since the rest of SyncAPI is essentially bookmark-agnostic. - entry_->Put(syncable::IS_BOOKMARK_OBJECT, true); + + // Set an empty specifics of the appropriate datatype. The presence + // of the specific extension will identify the model type. + switch (model_type) { + case syncable::BOOKMARKS: + PutBookmarkSpecificsAndMarkForSyncing( + sync_pb::BookmarkSpecifics::default_instance()); + break; + default: + NOTREACHED(); + } + DCHECK(GetModelType() == model_type); // Now set the predecessor, which sets IS_UNSYNCED as necessary. PutPredecessor(predecessor); @@ -629,14 +648,10 @@ void WriteNode::PutPredecessor(const BaseNode* predecessor) { MarkForSyncing(); } -void WriteNode::SetFaviconBytes(const unsigned char* bytes, - size_t size_in_bytes) { - syncable::Blob new_favicon(bytes, bytes + size_in_bytes); - if (new_favicon == entry_->Get(syncable::BOOKMARK_FAVICON)) - return; // Skip redundant changes. - - entry_->Put(syncable::BOOKMARK_FAVICON, new_favicon); - MarkForSyncing(); +void WriteNode::SetFaviconBytes(const vector& bytes) { + sync_pb::BookmarkSpecifics new_value = GetBookmarkSpecifics(); + new_value.set_favicon(bytes.empty() ? NULL : &bytes[0], bytes.size()); + SetBookmarkSpecifics(new_value); } void WriteNode::MarkForSyncing() { @@ -671,8 +686,10 @@ bool ReadNode::InitByIdLookup(int64 id) { return false; if (entry_->Get(syncable::IS_DEL)) return false; - LOG_IF(WARNING, !entry_->Get(syncable::IS_BOOKMARK_OBJECT)) - << "SyncAPI InitByIdLookup referencing non-bookmark object."; + syncable::ModelType model_type = GetModelType(); + LOG_IF(WARNING, model_type == syncable::UNSPECIFIED || + model_type == syncable::TOP_LEVEL_FOLDER) + << "SyncAPI InitByIdLookup referencing unusual object."; return true; } @@ -694,12 +711,13 @@ bool ReadNode::InitByTagLookup(const std::string& tag) { return false; if (entry_->Get(syncable::IS_DEL)) return false; - LOG_IF(WARNING, !entry_->Get(syncable::IS_BOOKMARK_OBJECT)) - << "SyncAPI InitByTagLookup referencing non-bookmark object."; + syncable::ModelType model_type = GetModelType(); + LOG_IF(WARNING, model_type == syncable::UNSPECIFIED || + model_type == syncable::TOP_LEVEL_FOLDER) + << "SyncAPI InitByTagLookup referencing unusually typed object."; return true; } - ////////////////////////////////////////////////////////////////////////// // ReadTransaction member definitions ReadTransaction::ReadTransaction(UserShare* share) @@ -917,8 +935,12 @@ class SyncManager::SyncInternal { // the relative order is unchanged). To handle such cases, we rely on the // caller to treat a position update on any sibling as updating the positions // of all siblings. - static bool BookmarkPositionsDiffer(const syncable::EntryKernel& a, - const syncable::Entry& b) { + static bool VisiblePositionsDiffer(const syncable::EntryKernel& a, + const syncable::Entry& b) { + // If the datatype isn't one where the browser model cares about position, + // don't bother notifying that data model of position-only changes. + if (!b.ShouldMaintainPosition()) + return false; if (a.ref(syncable::NEXT_ID) != b.Get(syncable::NEXT_ID)) return true; if (a.ref(syncable::PARENT_ID) != b.Get(syncable::PARENT_ID)) @@ -929,17 +951,23 @@ class SyncManager::SyncInternal { // Determine if any of the fields made visible to clients of the Sync API // differ between the versions of an entry stored in |a| and |b|. A return // value of false means that it should be OK to ignore this change. - static bool BookmarkPropertiesDiffer(const syncable::EntryKernel& a, - const syncable::Entry& b) { + static bool VisiblePropertiesDiffer(const syncable::EntryKernel& a, + const syncable::Entry& b) { + syncable::ModelType model_type = b.GetModelType(); + // Suppress updates to items that aren't tracked by any browser model. + if (model_type == syncable::UNSPECIFIED || + model_type == syncable::TOP_LEVEL_FOLDER) { + return false; + } if (a.ref(syncable::NON_UNIQUE_NAME) != b.Get(syncable::NON_UNIQUE_NAME)) return true; if (a.ref(syncable::IS_DIR) != b.Get(syncable::IS_DIR)) return true; - if (a.ref(syncable::BOOKMARK_URL) != b.Get(syncable::BOOKMARK_URL)) + if (a.ref(SPECIFICS).SerializeAsString() != + b.Get(SPECIFICS).SerializeAsString()) { return true; - if (a.ref(syncable::BOOKMARK_FAVICON) != b.Get(syncable::BOOKMARK_FAVICON)) - return true; - if (BookmarkPositionsDiffer(a, b)) + } + if (VisiblePositionsDiffer(a, b)) return true; return false; } @@ -1398,7 +1426,7 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncApi( if (e.IsRoot()) { // Ignore root object, should it ever change. continue; - } else if (!e.Get(syncable::IS_BOOKMARK_OBJECT)) { + } else if (e.GetModelType() != syncable::BOOKMARKS) { // Ignore non-bookmark objects. continue; } else if (e.Get(syncable::IS_UNSYNCED)) { @@ -1432,15 +1460,15 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncer( if (e.IsRoot()) continue; // Ignore non-bookmark objects. - if (!e.Get(syncable::IS_BOOKMARK_OBJECT)) + if (e.GetModelType() != syncable::BOOKMARKS) continue; if (exists_now && !existed_before) change_buffer_.PushAddedItem(id); else if (!exists_now && existed_before) change_buffer_.PushDeletedItem(id); - else if (exists_now && existed_before && BookmarkPropertiesDiffer(*i, e)) - change_buffer_.PushUpdatedItem(id, BookmarkPositionsDiffer(*i, e)); + else if (exists_now && existed_before && VisiblePropertiesDiffer(*i, e)) + change_buffer_.PushUpdatedItem(id, VisiblePositionsDiffer(*i, e)); } } diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h index 63ce5f5..b49d97a 100644 --- a/chrome/browser/sync/engine/syncapi.h +++ b/chrome/browser/sync/engine/syncapi.h @@ -39,11 +39,13 @@ #define CHROME_BROWSER_SYNC_ENGINE_SYNCAPI_H_ #include +#include #include "base/basictypes.h" #include "base/file_path.h" #include "build/build_config.h" #include "chrome/browser/google_service_auth_error.h" +#include "chrome/browser/sync/syncable/model_type.h" #include "googleurl/src/gurl.h" namespace browser_sync { @@ -62,6 +64,11 @@ class ScopedDirLookup; class WriteTransaction; } +namespace sync_pb { +class BookmarkSpecifics; +class EntitySpecifics; +} + namespace sync_api { // Forward declarations of classes to be defined later in this file. @@ -104,18 +111,29 @@ class BaseNode { // Returns the title of the object. // Uniqueness of the title is not enforced on siblings -- it is not an error // for two children to share a title. - const std::wstring& GetTitle() const; + std::wstring GetTitle() const; + + // Returns the model type of this object. The model type is set at node + // creation time and is expected never to change. + syncable::ModelType GetModelType() const; + + // Getter specific to the BOOKMARK datatype. Returns protobuf + // data. Can only be called if GetModelType() == BOOKMARK. + const sync_pb::BookmarkSpecifics& GetBookmarkSpecifics() const; + // Legacy, bookmark-specific getter that wraps GetBookmarkSpecifics() above. // Returns the URL of a bookmark object. - const GURL& GetURL() const; + // TODO(ncarter): Remove this datatype-specific accessor. + GURL GetURL() const; - // Return a pointer to the byte data of the favicon image for this node. - // Will return NULL if there is no favicon data associated with this node. - // The length of the array is returned to the caller via |size_in_bytes|. + // Legacy, bookmark-specific getter that wraps GetBookmarkSpecifics() above. + // Fill in a vector with the byte data of this node's favicon. Assumes + // that the node is a bookmark. // Favicons are expected to be PNG images, and though no verification is // done on the syncapi client of this, the server may reject favicon updates // that are invalid for whatever reason. - const unsigned char* GetFaviconBytes(size_t* size_in_bytes); + // TODO(ncarter): Remove this datatype-specific accessor. + void GetFaviconBytes(std::vector* output) const; // Returns the local external ID associated with the node. int64 GetExternalId() const; @@ -132,12 +150,6 @@ class BaseNode { // children, return 0. int64 GetFirstChildId() const; - // Get an array containing the IDs of this node's children. The memory is - // owned by BaseNode and becomes invalid if GetChildIds() is called a second - // time on this node, or when the node is destroyed. Return the array size - // in the child_count parameter. - const int64* GetChildIds(size_t* child_count) const; - // These virtual accessors provide access to data members of derived classes. virtual const syncable::Entry* GetEntry() const = 0; virtual const BaseTransaction* GetTransaction() const = 0; @@ -147,15 +159,9 @@ class BaseNode { virtual ~BaseNode(); private: - struct BaseNodeInternal; - // Node is meant for stack use only. void* operator new(size_t size); - // Provides storage for member functions that return pointers to class - // memory, e.g. C strings returned by GetTitle(). - BaseNodeInternal* data_; - DISALLOW_COPY_AND_ASSIGN(BaseNode); }; @@ -173,17 +179,20 @@ class WriteNode : public BaseNode { // BaseNode implementation. virtual bool InitByIdLookup(int64 id); - // Create a new node with the specified parent and predecessor. Use a NULL - // |predecessor| to indicate that this is to be the first child. + // Create a new node with the specified parent and predecessor. |model_type| + // dictates the type of the item, and controls which EntitySpecifics proto + // extension can be used with this item. Use a NULL |predecessor| + // to indicate that this is to be the first child. // |predecessor| must be a child of |new_parent| or NULL. Returns false on // failure. - bool InitByCreation(const BaseNode& parent, const BaseNode* predecessor); + bool InitByCreation(syncable::ModelType model_type, + const BaseNode& parent, + const BaseNode* predecessor); // These Set() functions correspond to the Get() functions of BaseNode. void SetIsFolder(bool folder); void SetTitle(const std::wstring& title); - void SetURL(const GURL& url); - void SetFaviconBytes(const unsigned char* bytes, size_t size_in_bytes); + // External ID is a client-only field, so setting it doesn't cause the item to // be synced again. void SetExternalId(int64 external_id); @@ -196,6 +205,16 @@ class WriteNode : public BaseNode { // be a child of |new_parent| or NULL. Returns false on failure.. bool SetPosition(const BaseNode& new_parent, const BaseNode* predecessor); + // Set the bookmark specifics (url and favicon). + // Should only be called if GetModelType() == BOOKMARK. + void SetBookmarkSpecifics(const sync_pb::BookmarkSpecifics& specifics); + + // Legacy, bookmark-specific setters that wrap SetBookmarkSpecifics() above. + // Should only be called if GetModelType() == BOOKMARK. + // TODO(ncarter): Remove these two datatype-specific accessors. + void SetURL(const GURL& url); + void SetFaviconBytes(const std::vector& bytes); + // Implementation of BaseNode's abstract virtual accessors. virtual const syncable::Entry* GetEntry() const; @@ -207,6 +226,16 @@ class WriteNode : public BaseNode { // Helper to set the previous node. void PutPredecessor(const BaseNode* predecessor); + // Private helpers to set type-specific protobuf data. These don't + // do any checking on the previous modeltype, so they can be used + // for internal initialization (you can use them to set the modeltype). + // Additionally, they will mark for syncing if the underlying value + // changes. + void PutBookmarkSpecificsAndMarkForSyncing( + const sync_pb::BookmarkSpecifics& new_value); + void PutSpecificsAndMarkForSyncing( + const sync_pb::EntitySpecifics& specifics); + // Sets IS_UNSYNCED and SYNCING to ensure this entry is considered in an // upcoming commit pass. void MarkForSyncing(); diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc index 01c2fdd..8316390 100755 --- a/chrome/browser/sync/engine/syncer.cc +++ b/chrome/browser/sync/engine/syncer.cc @@ -33,16 +33,14 @@ using base::TimeDelta; using sync_pb::ClientCommand; using syncable::Blob; using syncable::IS_UNAPPLIED_UPDATE; -using syncable::SERVER_BOOKMARK_FAVICON; -using syncable::SERVER_BOOKMARK_URL; using syncable::SERVER_CTIME; -using syncable::SERVER_IS_BOOKMARK_OBJECT; using syncable::SERVER_IS_DEL; using syncable::SERVER_IS_DIR; using syncable::SERVER_MTIME; using syncable::SERVER_NON_UNIQUE_NAME; using syncable::SERVER_PARENT_ID; using syncable::SERVER_POSITION_IN_PARENT; +using syncable::SERVER_SPECIFICS; using syncable::SERVER_VERSION; using syncable::SYNCER; using syncable::ScopedDirLookup; @@ -294,10 +292,8 @@ void CopyServerFields(syncable::Entry* src, syncable::MutableEntry* dest) { dest->Put(SERVER_VERSION, src->Get(SERVER_VERSION)); dest->Put(SERVER_IS_DIR, src->Get(SERVER_IS_DIR)); dest->Put(SERVER_IS_DEL, src->Get(SERVER_IS_DEL)); - dest->Put(SERVER_IS_BOOKMARK_OBJECT, src->Get(SERVER_IS_BOOKMARK_OBJECT)); dest->Put(IS_UNAPPLIED_UPDATE, src->Get(IS_UNAPPLIED_UPDATE)); - dest->Put(SERVER_BOOKMARK_URL, src->Get(SERVER_BOOKMARK_URL)); - dest->Put(SERVER_BOOKMARK_FAVICON, src->Get(SERVER_BOOKMARK_FAVICON)); + dest->Put(SERVER_SPECIFICS, src->Get(SERVER_SPECIFICS)); dest->Put(SERVER_POSITION_IN_PARENT, src->Get(SERVER_POSITION_IN_PARENT)); } @@ -309,10 +305,8 @@ void ClearServerData(syncable::MutableEntry* entry) { entry->Put(SERVER_VERSION, 0); entry->Put(SERVER_IS_DIR, false); entry->Put(SERVER_IS_DEL, false); - entry->Put(SERVER_IS_BOOKMARK_OBJECT, false); entry->Put(IS_UNAPPLIED_UPDATE, false); - entry->Put(SERVER_BOOKMARK_URL, ""); - entry->Put(SERVER_BOOKMARK_FAVICON, Blob()); + entry->Put(SERVER_SPECIFICS, sync_pb::EntitySpecifics::default_instance()); entry->Put(SERVER_POSITION_IN_PARENT, 0); } diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc index 17ed151..b9729eb 100755 --- a/chrome/browser/sync/engine/syncer_unittest.cc +++ b/chrome/browser/sync/engine/syncer_unittest.cc @@ -20,8 +20,9 @@ #include "chrome/browser/sync/engine/net/server_connection_manager.h" #include "chrome/browser/sync/engine/process_updates_command.h" #include "chrome/browser/sync/engine/syncer.h" -#include "chrome/browser/sync/engine/syncer_util.h" #include "chrome/browser/sync/engine/syncer_proto_util.h" +#include "chrome/browser/sync/engine/syncer_util.h" +#include "chrome/browser/sync/engine/syncproto.h" #include "chrome/browser/sync/protocol/sync.pb.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/syncable.h" @@ -65,7 +66,6 @@ using syncable::GET_BY_HANDLE; using syncable::GET_BY_ID; using syncable::GET_BY_TAG; using syncable::ID; -using syncable::IS_BOOKMARK_OBJECT; using syncable::IS_DEL; using syncable::IS_DIR; using syncable::IS_UNAPPLIED_UPDATE; @@ -80,8 +80,10 @@ using syncable::SERVER_IS_DEL; using syncable::SERVER_NON_UNIQUE_NAME; using syncable::SERVER_PARENT_ID; using syncable::SERVER_POSITION_IN_PARENT; +using syncable::SERVER_SPECIFICS; using syncable::SERVER_VERSION; using syncable::SINGLETON_TAG; +using syncable::SPECIFICS; using syncable::UNITTEST; using sessions::ConflictProgress; @@ -205,6 +207,10 @@ class SyncerTest : public testing::Test, ExtendedAttributeKey key(entry->Get(META_HANDLE), "DATA"); MutableExtendedAttribute attr(trans, CREATE, key); attr.mutable_value()->swap(test_value); + sync_pb::EntitySpecifics specifics; + specifics.MutableExtension(sync_pb::bookmark)->set_url("http://demo/"); + specifics.MutableExtension(sync_pb::bookmark)->set_favicon("PNG"); + entry->Put(syncable::SPECIFICS, specifics); entry->Put(syncable::IS_UNSYNCED, true); } void VerifyTestDataInEntry(BaseTransaction* trans, Entry* entry) { @@ -215,7 +221,15 @@ class SyncerTest : public testing::Test, ExtendedAttribute attr(trans, GET_BY_HANDLE, key); EXPECT_FALSE(attr.is_deleted()); EXPECT_TRUE(test_value == attr.value()); + VerifyTestBookmarkDataInEntry(entry); } + void VerifyTestBookmarkDataInEntry(Entry* entry) { + const sync_pb::EntitySpecifics& specifics = entry->Get(syncable::SPECIFICS); + EXPECT_TRUE(specifics.HasExtension(sync_pb::bookmark)); + EXPECT_EQ("PNG", specifics.GetExtension(sync_pb::bookmark).favicon()); + EXPECT_EQ("http://demo/", specifics.GetExtension(sync_pb::bookmark).url()); + } + void SyncRepeatedlyToTriggerConflictResolution(SyncSession* session) { // We should trigger after less than 6 syncs, but extra does no harm. for (int i = 0 ; i < 6 ; ++i) @@ -227,7 +241,11 @@ class SyncerTest : public testing::Test, for (int i = 0 ; i < 12 ; ++i) syncer_->SyncShare(session); } - + sync_pb::EntitySpecifics DefaultBookmarkSpecifics() { + sync_pb::EntitySpecifics result; + result.MutableExtension(sync_pb::bookmark); + return result; + } // Enumeration of alterations to entries for commit ordering tests. enum EntryFeature { LIST_END = 0, // Denotes the end of the list of features from below. @@ -251,6 +269,7 @@ class SyncerTest : public testing::Test, void RunCommitOrderingTest(CommitOrderingTest* test) { ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); ASSERT_TRUE(dir.good()); + map expected_positions; { // Transaction scope. WriteTransaction trans(dir, UNITTEST, __FILE__, __LINE__); @@ -264,6 +283,7 @@ class SyncerTest : public testing::Test, string utf8_name = test->id.GetServerId(); string name(utf8_name.begin(), utf8_name.end()); MutableEntry entry(&trans, CREATE, test->parent_id, name); + entry.Put(syncable::ID, test->id); if (test->id.ServerKnows()) { entry.Put(BASE_VERSION, 5); @@ -272,6 +292,7 @@ class SyncerTest : public testing::Test, } entry.Put(syncable::IS_DIR, true); entry.Put(syncable::IS_UNSYNCED, true); + entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); // Set the time to 30 seconds in the future to reduce the chance of // flaky tests. int64 now_server_time = ClientTimeToServerTime(syncable::Now()); @@ -351,6 +372,7 @@ class SyncerTest : public testing::Test, EXPECT_TRUE(entry.good()); entry.Put(syncable::IS_UNSYNCED, true); entry.Put(syncable::IS_DIR, true); + entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); entry.Put(syncable::BASE_VERSION, id.ServerKnows() ? 1 : 0); entry.Put(syncable::ID, id); return entry.Get(META_HANDLE); @@ -413,11 +435,11 @@ TEST_F(SyncerTest, GetCommitIdsCommandTruncates) { MutableEntry entry_c(&wtrans, GET_BY_HANDLE, handle_c); MutableEntry entry_d(&wtrans, GET_BY_HANDLE, handle_d); MutableEntry entry_e(&wtrans, GET_BY_HANDLE, handle_e); - entry_x.Put(IS_BOOKMARK_OBJECT, true); - entry_b.Put(IS_BOOKMARK_OBJECT, true); - entry_c.Put(IS_BOOKMARK_OBJECT, true); - entry_d.Put(IS_BOOKMARK_OBJECT, true); - entry_e.Put(IS_BOOKMARK_OBJECT, true); + entry_x.Put(SPECIFICS, DefaultBookmarkSpecifics()); + entry_b.Put(SPECIFICS, DefaultBookmarkSpecifics()); + entry_c.Put(SPECIFICS, DefaultBookmarkSpecifics()); + entry_d.Put(SPECIFICS, DefaultBookmarkSpecifics()); + entry_e.Put(SPECIFICS, DefaultBookmarkSpecifics()); entry_b.Put(PARENT_ID, entry_x.Get(ID)); entry_c.Put(PARENT_ID, entry_x.Get(ID)); entry_c.PutPredecessor(entry_b.Get(ID)); @@ -495,6 +517,7 @@ TEST_F(SyncerTest, TestGetUnsyncedAndSimpleCommit) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::BASE_VERSION, 1); parent.Put(syncable::ID, parent_id_); MutableEntry child(&wtrans, syncable::CREATE, parent_id_, "Pete"); @@ -661,6 +684,7 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNesting) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, ids_.FromNumber(100)); parent.Put(syncable::BASE_VERSION, 1); MutableEntry child(&wtrans, syncable::CREATE, ids_.FromNumber(100), @@ -668,6 +692,7 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNesting) { ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); child.Put(syncable::ID, ids_.FromNumber(101)); child.Put(syncable::BASE_VERSION, 1); MutableEntry grandchild(&wtrans, syncable::CREATE, ids_.FromNumber(101), @@ -675,6 +700,7 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNesting) { ASSERT_TRUE(grandchild.good()); grandchild.Put(syncable::ID, ids_.FromNumber(102)); grandchild.Put(syncable::IS_UNSYNCED, true); + grandchild.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); grandchild.Put(syncable::BASE_VERSION, 1); } { @@ -736,11 +762,13 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNewItems) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, parent_id_); MutableEntry child(&wtrans, syncable::CREATE, wtrans.root_id(), "2"); ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); child.Put(syncable::ID, child_id_); parent.Put(syncable::BASE_VERSION, 1); child.Put(syncable::BASE_VERSION, 1); @@ -751,11 +779,13 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNewItems) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, ids_.FromNumber(102)); MutableEntry child(&wtrans, syncable::CREATE, parent_id_, "B"); ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); child.Put(syncable::ID, ids_.FromNumber(-103)); parent.Put(syncable::BASE_VERSION, 1); } @@ -765,11 +795,13 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNewItems) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, ids_.FromNumber(-104)); MutableEntry child(&wtrans, syncable::CREATE, child_id_, "B"); ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); child.Put(syncable::ID, ids_.FromNumber(105)); child.Put(syncable::BASE_VERSION, 1); } @@ -798,6 +830,7 @@ TEST_F(SyncerTest, TestCommitListOrderingCounterexample) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, parent_id_); MutableEntry child1(&wtrans, syncable::CREATE, parent_id_, "1"); ASSERT_TRUE(child1.good()); @@ -836,6 +869,7 @@ TEST_F(SyncerTest, TestCommitListOrderingAndNewParent) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, parent_id_); parent.Put(syncable::BASE_VERSION, 1); } @@ -848,12 +882,14 @@ TEST_F(SyncerTest, TestCommitListOrderingAndNewParent) { ASSERT_TRUE(parent2.good()); parent2.Put(syncable::IS_UNSYNCED, true); parent2.Put(syncable::IS_DIR, true); + parent2.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent2.Put(syncable::ID, parent2_id); MutableEntry child(&wtrans, syncable::CREATE, parent2_id, child_name); ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); child.Put(syncable::ID, child_id); child.Put(syncable::BASE_VERSION, 1); } @@ -906,6 +942,7 @@ TEST_F(SyncerTest, TestCommitListOrderingAndNewParentAndChild) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(syncable::ID, parent_id_); parent.Put(syncable::BASE_VERSION, 1); } @@ -919,6 +956,7 @@ TEST_F(SyncerTest, TestCommitListOrderingAndNewParentAndChild) { ASSERT_TRUE(parent2.good()); parent2.Put(syncable::IS_UNSYNCED, true); parent2.Put(syncable::IS_DIR, true); + parent2.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent2.Put(syncable::ID, parent2_local_id); meta_handle_a = parent2.Get(syncable::META_HANDLE); @@ -926,6 +964,7 @@ TEST_F(SyncerTest, TestCommitListOrderingAndNewParentAndChild) { ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); child.Put(syncable::ID, child_local_id); meta_handle_b = child.Get(syncable::META_HANDLE); } @@ -1197,6 +1236,7 @@ TEST_F(SyncerTest, CommitTimeRename) { MutableEntry parent(&trans, CREATE, root_id_, "Folder"); ASSERT_TRUE(parent.good()); parent.Put(IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(IS_UNSYNCED, true); metahandle_folder = parent.Get(META_HANDLE); @@ -1246,6 +1286,7 @@ TEST_F(SyncerTest, CommitTimeRenameI18N) { MutableEntry parent(&trans, CREATE, root_id_, "Folder"); ASSERT_TRUE(parent.good()); parent.Put(IS_DIR, true); + parent.Put(SPECIFICS, DefaultBookmarkSpecifics()); parent.Put(IS_UNSYNCED, true); metahandle = parent.Get(META_HANDLE); } @@ -1279,6 +1320,7 @@ TEST_F(SyncerTest, CommitReuniteUpdateAdjustsChildren) { MutableEntry entry(&trans, CREATE, trans.root_id(), "new_folder"); ASSERT_TRUE(entry.good()); entry.Put(IS_DIR, true); + entry.Put(SPECIFICS, DefaultBookmarkSpecifics()); entry.Put(IS_UNSYNCED, true); metahandle_folder = entry.Get(META_HANDLE); } @@ -1604,6 +1646,7 @@ class EntryCreatedInNewFolderTest : public SyncerTest { CHECK(entry2.good()); entry2.Put(syncable::IS_DIR, true); entry2.Put(syncable::IS_UNSYNCED, true); + entry2.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); } }; @@ -1617,6 +1660,7 @@ TEST_F(EntryCreatedInNewFolderTest, EntryCreatedInNewFolderMidSync) { ASSERT_TRUE(entry.good()); entry.Put(syncable::IS_DIR, true); entry.Put(syncable::IS_UNSYNCED, true); + entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); } mock_server_->SetMidCommitCallback( @@ -1697,24 +1741,28 @@ TEST_F(SyncerTest, DoublyChangedWithResolver) { parent.Put(syncable::IS_DIR, true); parent.Put(syncable::ID, parent_id_); parent.Put(syncable::BASE_VERSION, 5); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); MutableEntry child(&wtrans, syncable::CREATE, parent_id_, "Pete.htm"); ASSERT_TRUE(child.good()); child.Put(syncable::ID, child_id_); child.Put(syncable::BASE_VERSION, 10); WriteTestDataToEntry(&wtrans, &child); } - mock_server_->AddUpdateBookmark(child_id_, parent_id_, "Pete.htm", 11, 10); + mock_server_->AddUpdateBookmark(child_id_, parent_id_, "Pete2.htm", 11, 10); mock_server_->set_conflict_all_commits(true); LoopSyncShare(syncer_); syncable::Directory::ChildHandles children; { ReadTransaction trans(dir, __FILE__, __LINE__); dir->GetChildHandles(&trans, parent_id_, &children); - // We expect the conflict resolver to just clobber the entry. + // We expect the conflict resolver to preserve the local entry. Entry child(&trans, syncable::GET_BY_ID, child_id_); ASSERT_TRUE(child.good()); EXPECT_TRUE(child.Get(syncable::IS_UNSYNCED)); EXPECT_FALSE(child.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(child.Get(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_EQ("Pete.htm", child.Get(NON_UNIQUE_NAME)); + VerifyTestBookmarkDataInEntry(&child); } // Only one entry, since we just overwrite one. @@ -1735,6 +1783,7 @@ TEST_F(SyncerTest, CommitsUpdateDoesntAlterEntry) { ASSERT_TRUE(entry.good()); EXPECT_FALSE(entry.Get(ID).ServerKnows()); entry.Put(syncable::IS_DIR, true); + entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); entry.Put(syncable::IS_UNSYNCED, true); entry.Put(syncable::MTIME, test_time); entry_metahandle = entry.Get(META_HANDLE); @@ -1777,13 +1826,13 @@ TEST_F(SyncerTest, ParentAndChildBothMatch) { parent.Put(IS_UNSYNCED, true); parent.Put(ID, parent_id); parent.Put(BASE_VERSION, 1); - parent.Put(IS_BOOKMARK_OBJECT, true); + parent.Put(SPECIFICS, DefaultBookmarkSpecifics()); MutableEntry child(&wtrans, CREATE, parent.Get(ID), "test.htm"); ASSERT_TRUE(child.good()); child.Put(ID, child_id); child.Put(BASE_VERSION, 1); - child.Put(IS_BOOKMARK_OBJECT, true); + child.Put(SPECIFICS, DefaultBookmarkSpecifics()); WriteTestDataToEntry(&wtrans, &child); } mock_server_->AddUpdateDirectory(parent_id, root_id_, "Folder", 10, 10); @@ -1844,6 +1893,8 @@ TEST_F(SyncerTest, UnappliedUpdateDuringCommit) { entry.Put(SERVER_PARENT_ID, ids_.FromNumber(9999)); // Bad parent. entry.Put(IS_UNSYNCED, true); entry.Put(IS_UNAPPLIED_UPDATE, true); + entry.Put(SPECIFICS, DefaultBookmarkSpecifics()); + entry.Put(SERVER_SPECIFICS, DefaultBookmarkSpecifics()); entry.Put(IS_DEL, false); } syncer_->SyncShare(session_.get()); @@ -1875,6 +1926,7 @@ TEST_F(SyncerTest, DeletingEntryInFolder) { MutableEntry entry(&trans, CREATE, trans.root_id(), "existing"); ASSERT_TRUE(entry.good()); entry.Put(IS_DIR, true); + entry.Put(SPECIFICS, DefaultBookmarkSpecifics()); entry.Put(IS_UNSYNCED, true); existing_metahandle = entry.Get(META_HANDLE); } @@ -1884,6 +1936,7 @@ TEST_F(SyncerTest, DeletingEntryInFolder) { MutableEntry newfolder(&trans, CREATE, trans.root_id(), "new"); ASSERT_TRUE(newfolder.good()); newfolder.Put(IS_DIR, true); + newfolder.Put(SPECIFICS, DefaultBookmarkSpecifics()); newfolder.Put(IS_UNSYNCED, true); MutableEntry existing(&trans, GET_BY_HANDLE, existing_metahandle); @@ -1912,6 +1965,8 @@ TEST_F(SyncerTest, DeletingEntryWithLocalEdits) { MutableEntry newfolder(&trans, CREATE, ids_.FromNumber(1), "local"); ASSERT_TRUE(newfolder.good()); newfolder.Put(IS_UNSYNCED, true); + newfolder.Put(IS_DIR, true); + newfolder.Put(SPECIFICS, DefaultBookmarkSpecifics()); newfolder_metahandle = newfolder.Get(META_HANDLE); } mock_server_->AddUpdateDirectory(1, 0, "bob", 2, 20); @@ -2004,6 +2059,7 @@ TEST_F(SyncerTest, CommitManyItemsInOneGo) { MutableEntry e(&trans, CREATE, trans.root_id(), name); e.Put(IS_UNSYNCED, true); e.Put(IS_DIR, true); + e.Put(SPECIFICS, DefaultBookmarkSpecifics()); } } uint32 num_loops = 0; @@ -2107,6 +2163,7 @@ TEST_F(SyncerTest, NewEntryAndAlteredServerEntrySharePath) { local_folder_id = new_entry.Get(ID); local_folder_handle = new_entry.Get(META_HANDLE); new_entry.Put(IS_UNSYNCED, true); + new_entry.Put(SPECIFICS, DefaultBookmarkSpecifics()); MutableEntry old(&wtrans, GET_BY_ID, ids_.FromNumber(1)); ASSERT_TRUE(old.good()); WriteTestDataToEntry(&wtrans, &old); @@ -2115,8 +2172,118 @@ TEST_F(SyncerTest, NewEntryAndAlteredServerEntrySharePath) { mock_server_->set_conflict_all_commits(true); syncer_->SyncShare(this); syncer_events_.clear(); + { + // Update #20 should have been dropped in favor of the local version. + WriteTransaction wtrans(dir, UNITTEST, __FILE__, __LINE__); + MutableEntry server(&wtrans, GET_BY_ID, ids_.FromNumber(1)); + MutableEntry local(&wtrans, GET_BY_HANDLE, local_folder_handle); + ASSERT_TRUE(server.good()); + ASSERT_TRUE(local.good()); + EXPECT_TRUE(local.Get(META_HANDLE) != server.Get(META_HANDLE)); + EXPECT_FALSE(server.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(local.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(server.Get(IS_UNSYNCED)); + EXPECT_TRUE(local.Get(IS_UNSYNCED)); + EXPECT_EQ("Foo.htm", server.Get(NON_UNIQUE_NAME)); + EXPECT_EQ("Bar.htm", local.Get(NON_UNIQUE_NAME)); + } + // Allow local changes to commit. + mock_server_->set_conflict_all_commits(false); + syncer_->SyncShare(this); + syncer_events_.clear(); + + // Now add a server change to make the two names equal. There should + // be no conflict with that, since names are not unique. + mock_server_->AddUpdateBookmark(1, 0, "Bar.htm", 30, 30); + syncer_->SyncShare(this); + syncer_events_.clear(); + { + WriteTransaction wtrans(dir, UNITTEST, __FILE__, __LINE__); + MutableEntry server(&wtrans, GET_BY_ID, ids_.FromNumber(1)); + MutableEntry local(&wtrans, GET_BY_HANDLE, local_folder_handle); + ASSERT_TRUE(server.good()); + ASSERT_TRUE(local.good()); + EXPECT_TRUE(local.Get(META_HANDLE) != server.Get(META_HANDLE)); + EXPECT_FALSE(server.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(local.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(server.Get(IS_UNSYNCED)); + EXPECT_FALSE(local.Get(IS_UNSYNCED)); + EXPECT_EQ("Bar.htm", server.Get(NON_UNIQUE_NAME)); + EXPECT_EQ("Bar.htm", local.Get(NON_UNIQUE_NAME)); + EXPECT_EQ("http://google.com", // Default from AddUpdateBookmark. + server.Get(SPECIFICS).GetExtension(sync_pb::bookmark).url()); + } } +// Same as NewEntryAnddServerEntrySharePath, but using the old-style protocol. +TEST_F(SyncerTest, NewEntryAndAlteredServerEntrySharePath_OldBookmarksProto) { + mock_server_->set_use_legacy_bookmarks_protocol(true); + ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); + CHECK(dir.good()); + mock_server_->AddUpdateBookmark(1, 0, "Foo.htm", 10, 10); + syncer_->SyncShare(this); + int64 local_folder_handle; + syncable::Id local_folder_id; + { + WriteTransaction wtrans(dir, UNITTEST, __FILE__, __LINE__); + MutableEntry new_entry(&wtrans, CREATE, wtrans.root_id(), "Bar.htm"); + ASSERT_TRUE(new_entry.good()); + local_folder_id = new_entry.Get(ID); + local_folder_handle = new_entry.Get(META_HANDLE); + new_entry.Put(IS_UNSYNCED, true); + new_entry.Put(SPECIFICS, DefaultBookmarkSpecifics()); + MutableEntry old(&wtrans, GET_BY_ID, ids_.FromNumber(1)); + ASSERT_TRUE(old.good()); + WriteTestDataToEntry(&wtrans, &old); + } + mock_server_->AddUpdateBookmark(1, 0, "Bar.htm", 20, 20); + mock_server_->set_conflict_all_commits(true); + syncer_->SyncShare(this); + syncer_events_.clear(); + { + // Update #20 should have been dropped in favor of the local version. + WriteTransaction wtrans(dir, UNITTEST, __FILE__, __LINE__); + MutableEntry server(&wtrans, GET_BY_ID, ids_.FromNumber(1)); + MutableEntry local(&wtrans, GET_BY_HANDLE, local_folder_handle); + ASSERT_TRUE(server.good()); + ASSERT_TRUE(local.good()); + EXPECT_TRUE(local.Get(META_HANDLE) != server.Get(META_HANDLE)); + EXPECT_FALSE(server.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(local.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(server.Get(IS_UNSYNCED)); + EXPECT_TRUE(local.Get(IS_UNSYNCED)); + EXPECT_EQ("Foo.htm", server.Get(NON_UNIQUE_NAME)); + EXPECT_EQ("Bar.htm", local.Get(NON_UNIQUE_NAME)); + } + // Allow local changes to commit. + mock_server_->set_conflict_all_commits(false); + syncer_->SyncShare(this); + syncer_events_.clear(); + + // Now add a server change to make the two names equal. There should + // be no conflict with that, since names are not unique. + mock_server_->AddUpdateBookmark(1, 0, "Bar.htm", 30, 30); + syncer_->SyncShare(this); + syncer_events_.clear(); + { + WriteTransaction wtrans(dir, UNITTEST, __FILE__, __LINE__); + MutableEntry server(&wtrans, GET_BY_ID, ids_.FromNumber(1)); + MutableEntry local(&wtrans, GET_BY_HANDLE, local_folder_handle); + ASSERT_TRUE(server.good()); + ASSERT_TRUE(local.good()); + EXPECT_TRUE(local.Get(META_HANDLE) != server.Get(META_HANDLE)); + EXPECT_FALSE(server.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(local.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(server.Get(IS_UNSYNCED)); + EXPECT_FALSE(local.Get(IS_UNSYNCED)); + EXPECT_EQ("Bar.htm", server.Get(NON_UNIQUE_NAME)); + EXPECT_EQ("Bar.htm", local.Get(NON_UNIQUE_NAME)); + EXPECT_EQ("http://google.com", // Default from AddUpdateBookmark. + server.Get(SPECIFICS).GetExtension(sync_pb::bookmark).url()); + } +} + + // Circular links should be resolved by the server. TEST_F(SyncerTest, SiblingDirectoriesBecomeCircular) { // we don't currently resolve this. This test ensures we don't. @@ -3075,10 +3242,12 @@ TEST_F(SyncerTest, DuplicateIDReturn) { ASSERT_TRUE(folder.good()); folder.Put(IS_UNSYNCED, true); folder.Put(IS_DIR, true); + folder.Put(SPECIFICS, DefaultBookmarkSpecifics()); MutableEntry folder2(&trans, CREATE, trans.root_id(), "fred"); ASSERT_TRUE(folder2.good()); folder2.Put(IS_UNSYNCED, false); folder2.Put(IS_DIR, true); + folder2.Put(SPECIFICS, DefaultBookmarkSpecifics()); folder2.Put(BASE_VERSION, 3); folder2.Put(ID, syncable::Id::CreateFromServerId("mock_server:10000")); } @@ -3158,7 +3327,7 @@ TEST_F(SyncerTest, ConflictResolverMergesLocalDeleteAndServerUpdate) { local_deleted.Put(IS_DEL, true); local_deleted.Put(IS_DIR, false); local_deleted.Put(IS_UNSYNCED, true); - local_deleted.Put(IS_BOOKMARK_OBJECT, true); + local_deleted.Put(SPECIFICS, DefaultBookmarkSpecifics()); } mock_server_->AddUpdateBookmark(ids_.FromNumber(1), root_id_, "name", 10, 10); @@ -3476,6 +3645,7 @@ TEST_F(SyncerTest, DirectoryCommitTest) { ASSERT_TRUE(parent.good()); parent.Put(syncable::IS_UNSYNCED, true); parent.Put(syncable::IS_DIR, true); + parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); in_root_id = parent.Get(syncable::ID); foo_metahandle = parent.Get(META_HANDLE); @@ -3483,6 +3653,7 @@ TEST_F(SyncerTest, DirectoryCommitTest) { ASSERT_TRUE(child.good()); child.Put(syncable::IS_UNSYNCED, true); child.Put(syncable::IS_DIR, true); + child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics()); bar_metahandle = child.Get(META_HANDLE); in_dir_id = parent.Get(syncable::ID); } @@ -3571,15 +3742,16 @@ TEST_F(SyncerTest, EnsureWeSendUpOldParent) { "folder_two", 1, 1); syncer_->SyncShare(this); { - // A moved entry should send an old parent. + // A moved entry should send an "old parent." WriteTransaction trans(dir, UNITTEST, __FILE__, __LINE__); MutableEntry entry(&trans, GET_BY_ID, folder_one_id); ASSERT_TRUE(entry.good()); entry.Put(PARENT_ID, folder_two_id); entry.Put(IS_UNSYNCED, true); - // A new entry should send no parent. + // A new entry should send no "old parent." MutableEntry create(&trans, CREATE, trans.root_id(), "new_folder"); create.Put(IS_UNSYNCED, true); + create.Put(SPECIFICS, DefaultBookmarkSpecifics()); } syncer_->SyncShare(this); const sync_pb::CommitMessage& commit = mock_server_->last_sent_commit(); diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc index da451a5..c784b93 100755 --- a/chrome/browser/sync/engine/syncer_util.cc +++ b/chrome/browser/sync/engine/syncer_util.cc @@ -14,13 +14,12 @@ #include "chrome/browser/sync/engine/syncproto.h" #include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/syncable/directory_manager.h" +#include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/syncable/syncable.h" #include "chrome/browser/sync/syncable/syncable_changes_version.h" #include "chrome/browser/sync/util/sync_types.h" using syncable::BASE_VERSION; -using syncable::BOOKMARK_FAVICON; -using syncable::BOOKMARK_URL; using syncable::Blob; using syncable::CHANGES_VERSION; using syncable::CREATE; @@ -33,7 +32,6 @@ using syncable::ExtendedAttributeKey; using syncable::GET_BY_HANDLE; using syncable::GET_BY_ID; using syncable::ID; -using syncable::IS_BOOKMARK_OBJECT; using syncable::IS_DEL; using syncable::IS_DIR; using syncable::IS_UNAPPLIED_UPDATE; @@ -48,18 +46,17 @@ using syncable::NON_UNIQUE_NAME; using syncable::PARENT_ID; using syncable::PREV_ID; using syncable::ReadTransaction; -using syncable::SERVER_BOOKMARK_FAVICON; -using syncable::SERVER_BOOKMARK_URL; using syncable::SERVER_CTIME; -using syncable::SERVER_IS_BOOKMARK_OBJECT; using syncable::SERVER_IS_DEL; using syncable::SERVER_IS_DIR; using syncable::SERVER_MTIME; using syncable::SERVER_NON_UNIQUE_NAME; using syncable::SERVER_PARENT_ID; using syncable::SERVER_POSITION_IN_PARENT; +using syncable::SERVER_SPECIFICS; using syncable::SERVER_VERSION; using syncable::SINGLETON_TAG; +using syncable::SPECIFICS; using syncable::SYNCER; using syncable::WriteTransaction; @@ -237,14 +234,24 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry( } namespace { -void UpdateLocalBookmarkSpecifics(const string& url, - const string& favicon_bytes, - MutableEntry* local_entry) { - local_entry->Put(SERVER_BOOKMARK_URL, url); - Blob favicon_blob; - SyncerProtoUtil::CopyProtoBytesIntoBlob(favicon_bytes, - &favicon_blob); - local_entry->Put(SERVER_BOOKMARK_FAVICON, favicon_blob); +// Helper to synthesize a new-style sync_pb::EntitySpecifics for use locally, +// when the server speaks only the old sync_pb::SyncEntity_BookmarkData-based +// protocol. +void UpdateBookmarkSpecifics(const string& singleton_tag, + const string& url, + const string& favicon_bytes, + MutableEntry* local_entry) { + // In the new-style protocol, the server no longer sends bookmark info for + // the "google_chrome" folder. Mimic that here. + if (singleton_tag == "google_chrome") + return; + sync_pb::EntitySpecifics pb; + sync_pb::BookmarkSpecifics* bookmark = pb.MutableExtension(sync_pb::bookmark); + if (!url.empty()) + bookmark->set_url(url); + if (!favicon_bytes.empty()) + bookmark->set_favicon(favicon_bytes); + local_entry->Put(SERVER_SPECIFICS, pb); } } // namespace @@ -275,27 +282,23 @@ void SyncerUtil::UpdateServerFieldsFromUpdate( ServerTimeToClientTime(server_entry.ctime())); local_entry->Put(SERVER_MTIME, ServerTimeToClientTime(server_entry.mtime())); - local_entry->Put(SERVER_IS_BOOKMARK_OBJECT, - GetModelType(server_entry) == syncable::BOOKMARKS); local_entry->Put(SERVER_IS_DIR, server_entry.IsFolder()); if (server_entry.has_singleton_tag()) { const string& tag = server_entry.singleton_tag(); local_entry->Put(SINGLETON_TAG, tag); } - if (!server_entry.deleted()) { - if (server_entry.specifics().HasExtension(sync_pb::bookmark)) { - // New style bookmark data. - const sync_pb::BookmarkSpecifics& bookmark = - server_entry.specifics().GetExtension(sync_pb::bookmark); - UpdateLocalBookmarkSpecifics(bookmark.url(), - bookmark.favicon(), local_entry); - } else if (server_entry.has_bookmarkdata()) { - // Old style bookmark data. - const SyncEntity::BookmarkData& bookmark = server_entry.bookmarkdata(); - UpdateLocalBookmarkSpecifics(bookmark.bookmark_url(), - bookmark.bookmark_favicon(), - local_entry); - } + // Store the datatype-specific part as a protobuf. + if (server_entry.has_specifics()) { + DCHECK(server_entry.GetModelType() != syncable::UNSPECIFIED) + << "Storing unrecognized datatype in sync database."; + local_entry->Put(SERVER_SPECIFICS, server_entry.specifics()); + } else if (server_entry.has_bookmarkdata()) { + // Legacy protocol response for bookmark data. + const SyncEntity::BookmarkData& bookmark = server_entry.bookmarkdata(); + UpdateBookmarkSpecifics(server_entry.singleton_tag(), + bookmark.bookmark_url(), + bookmark.bookmark_favicon(), + local_entry); } if (server_entry.has_position_in_parent()) { local_entry->Put(SERVER_POSITION_IN_PARENT, @@ -391,13 +394,11 @@ bool SyncerUtil::ServerAndLocalEntriesMatch(syncable::Entry* entry) { return false; } - if (entry->Get(IS_BOOKMARK_OBJECT)) { - if (!entry->Get(IS_DIR)) { - if (entry->Get(BOOKMARK_URL) != entry->Get(SERVER_BOOKMARK_URL)) { - LOG(WARNING) << "Bookmark URL mismatch"; - return false; - } - } + // TODO(ncarter): This is unfortunately heavyweight. Can we do better? + if (entry->Get(SPECIFICS).SerializeAsString() != + entry->Get(SERVER_SPECIFICS).SerializeAsString()) { + LOG(WARNING) << "Specifics mismatch"; + return false; } if (entry->Get(IS_DIR)) return true; @@ -435,8 +436,11 @@ void SyncerUtil::UpdateLocalDataFromServerData( syncable::MutableEntry* entry) { CHECK(!entry->Get(IS_UNSYNCED)); CHECK(entry->Get(IS_UNAPPLIED_UPDATE)); + LOG(INFO) << "Updating entry : " << *entry; - entry->Put(IS_BOOKMARK_OBJECT, entry->Get(SERVER_IS_BOOKMARK_OBJECT)); + // Start by setting the properties that determine the model_type. + entry->Put(SPECIFICS, entry->Get(SERVER_SPECIFICS)); + entry->Put(IS_DIR, entry->Get(SERVER_IS_DIR)); // This strange dance around the IS_DEL flag avoids problems when setting // the name. // TODO(chron): Is this still an issue? Unit test this codepath. @@ -455,10 +459,7 @@ void SyncerUtil::UpdateLocalDataFromServerData( entry->Put(CTIME, entry->Get(SERVER_CTIME)); entry->Put(MTIME, entry->Get(SERVER_MTIME)); entry->Put(BASE_VERSION, entry->Get(SERVER_VERSION)); - entry->Put(IS_DIR, entry->Get(SERVER_IS_DIR)); entry->Put(IS_DEL, entry->Get(SERVER_IS_DEL)); - entry->Put(BOOKMARK_URL, entry->Get(SERVER_BOOKMARK_URL)); - entry->Put(BOOKMARK_FAVICON, entry->Get(SERVER_BOOKMARK_FAVICON)); entry->Put(IS_UNAPPLIED_UPDATE, false); } @@ -616,7 +617,7 @@ VerifyResult SyncerUtil::VerifyUpdateConsistency( syncable::MutableEntry* same_id, const bool deleted, const bool is_directory, - const bool has_bookmark_data) { + syncable::ModelType model_type) { CHECK(same_id->good()); @@ -624,11 +625,16 @@ VerifyResult SyncerUtil::VerifyUpdateConsistency( if (deleted) return VERIFY_SUCCESS; + if (model_type == syncable::UNSPECIFIED) { + // This update is to an item of a datatype we don't recognize. The server + // shouldn't have sent it to us. Throw it on the ground. + return VERIFY_SKIP; + } + if (same_id->Get(SERVER_VERSION) > 0) { // Then we've had an update for this entry before. if (is_directory != same_id->Get(SERVER_IS_DIR) || - (!is_directory && - has_bookmark_data != same_id->Get(SERVER_IS_BOOKMARK_OBJECT))) { + model_type != same_id->GetServerModelType()) { if (same_id->Get(IS_DEL)) { // If we've deleted the item, we don't care. return VERIFY_SKIP; } else { @@ -657,8 +663,7 @@ VerifyResult SyncerUtil::VerifyUpdateConsistency( if (same_id->Get(BASE_VERSION) > 0) { // We've committed this entry in the past. if (is_directory != same_id->Get(IS_DIR) || - (!is_directory && - has_bookmark_data != same_id->Get(IS_BOOKMARK_OBJECT))) { + model_type != same_id->GetModelType()) { LOG(ERROR) << "Server update doesn't agree with committed item. "; LOG(ERROR) << " Entry: " << *same_id; LOG(ERROR) << " Update: " << SyncEntityDebugString(entry); diff --git a/chrome/browser/sync/engine/syncer_util.h b/chrome/browser/sync/engine/syncer_util.h index f42bbe3..3f8d371 100755 --- a/chrome/browser/sync/engine/syncer_util.h +++ b/chrome/browser/sync/engine/syncer_util.h @@ -88,7 +88,7 @@ class SyncerUtil { syncable::MutableEntry* same_id, const bool deleted, const bool is_directory, - const bool is_bookmark); + syncable::ModelType model_type); // Assumes we have an existing entry; verify an update that seems to be // expressing an 'undelete' diff --git a/chrome/browser/sync/engine/syncproto.h b/chrome/browser/sync/engine/syncproto.h index cbe56a1..1d5f428 100644 --- a/chrome/browser/sync/engine/syncproto.h +++ b/chrome/browser/sync/engine/syncproto.h @@ -7,7 +7,9 @@ #ifndef CHROME_BROWSER_SYNC_ENGINE_SYNCPROTO_H_ #define CHROME_BROWSER_SYNC_ENGINE_SYNCPROTO_H_ +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/protocol/sync.pb.h" +#include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/syncable/syncable_id.h" namespace browser_sync { @@ -49,6 +51,28 @@ class SyncEntity : public IdWrapper { return ((has_folder() && folder()) || (has_bookmarkdata() && bookmarkdata().bookmark_folder())); } + + // Note: keep this consistent with GetModelType in syncable.cc! + syncable::ModelType GetModelType() const { + DCHECK(!id().IsRoot()); // Root shouldn't ever go over the wire. + + if (deleted()) + return syncable::UNSPECIFIED; + + if (specifics().HasExtension(sync_pb::bookmark) || has_bookmarkdata()) + return syncable::BOOKMARKS; + + // Loose check for server-created top-level folders that aren't + // bound to a particular model type. + if (!singleton_tag().empty() && IsFolder()) + return syncable::TOP_LEVEL_FOLDER; + + // This is an item of a datatype we can't understand. Maybe it's + // from the future? Either we mis-encoded the object, or the + // server sent us entries it shouldn't have. + NOTREACHED() << "Unknown datatype in sync proto."; + return syncable::UNSPECIFIED; + } }; class CommitResponse_EntryResponse diff --git a/chrome/browser/sync/engine/verify_updates_command.cc b/chrome/browser/sync/engine/verify_updates_command.cc index d357c7c..76b3851 100755 --- a/chrome/browser/sync/engine/verify_updates_command.cc +++ b/chrome/browser/sync/engine/verify_updates_command.cc @@ -59,8 +59,7 @@ VerifyResult VerifyUpdatesCommand::VerifyUpdate( const bool deleted = entry.has_deleted() && entry.deleted(); const bool is_directory = entry.IsFolder(); - const bool is_bookmark = - SyncerUtil::GetModelType(entry) == syncable::BOOKMARKS; + const syncable::ModelType model_type = entry.GetModelType(); if (!id.ServerKnows()) { LOG(ERROR) << "Illegal negative id in received updates"; @@ -91,7 +90,7 @@ VerifyResult VerifyUpdatesCommand::VerifyUpdate( // consistency rules. if (VERIFY_UNDECIDED == result) { result = SyncerUtil::VerifyUpdateConsistency(trans, entry, &same_id, - deleted, is_directory, is_bookmark); + deleted, is_directory, model_type); } if (VERIFY_UNDECIDED == result) diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc index a4e2b47..4b45a74 100644 --- a/chrome/browser/sync/glue/bookmark_change_processor.cc +++ b/chrome/browser/sync/glue/bookmark_change_processor.cc @@ -281,8 +281,9 @@ bool BookmarkChangeProcessor::PlaceSyncNode(MoveOrCreate operation, bool success = false; if (index == 0) { // Insert into first position. - success = (operation == CREATE) ? dst->InitByCreation(sync_parent, NULL) : - dst->SetPosition(sync_parent, NULL); + success = (operation == CREATE) ? + dst->InitByCreation(syncable::BOOKMARKS, sync_parent, NULL) : + dst->SetPosition(sync_parent, NULL); if (success) { DCHECK_EQ(dst->GetParentId(), sync_parent.GetId()); DCHECK_EQ(dst->GetId(), sync_parent.GetFirstChildId()); @@ -297,7 +298,7 @@ bool BookmarkChangeProcessor::PlaceSyncNode(MoveOrCreate operation, return false; } success = (operation == CREATE) ? - dst->InitByCreation(sync_parent, &sync_prev) : + dst->InitByCreation(syncable::BOOKMARKS, sync_parent, &sync_prev) : dst->SetPosition(sync_parent, &sync_prev); if (success) { DCHECK_EQ(dst->GetParentId(), sync_parent.GetId()); @@ -498,9 +499,9 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon( sync_api::BaseNode* sync_node, const BookmarkNode* bookmark_node, Profile* profile) { - size_t icon_size = 0; - const unsigned char* icon_bytes = sync_node->GetFaviconBytes(&icon_size); - if (!icon_size || !icon_bytes) + std::vector icon_bytes_vector; + sync_node->GetFaviconBytes(&icon_bytes_vector); + if (icon_bytes_vector.empty()) return false; // Registering a favicon requires that we provide a source URL, but we @@ -510,9 +511,6 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon( // which does not collide with others. GURL fake_icon_url = bookmark_node->GetURL(); - std::vector icon_bytes_vector(icon_bytes, - icon_bytes + icon_size); - HistoryService* history = profile->GetHistoryService(Profile::EXPLICIT_ACCESS); FaviconService* favicon_service = @@ -534,7 +532,7 @@ void BookmarkChangeProcessor::SetSyncNodeFavicon( std::vector favicon_bytes; EncodeFavicon(bookmark_node, model, &favicon_bytes); if (!favicon_bytes.empty()) - sync_node->SetFaviconBytes(&favicon_bytes[0], favicon_bytes.size()); + sync_node->SetFaviconBytes(favicon_bytes); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/preference_change_processor.cc b/chrome/browser/sync/glue/preference_change_processor.cc index f85ec5c..a05823b 100644 --- a/chrome/browser/sync/glue/preference_change_processor.cc +++ b/chrome/browser/sync/glue/preference_change_processor.cc @@ -42,7 +42,9 @@ void PreferenceChangeProcessor::Observe(NotificationType type, error_handler()->OnUnrecoverableError(); return; } - if (!sync_node.InitByCreation(root_node, NULL)) { + // TODO(ncarter): Define and use a preferences model_type. + syncable::ModelType model_type = syncable::BOOKMARKS; + if (!sync_node.InitByCreation(model_type, root_node, NULL)) { LOG(ERROR) << "Preference node creation failed."; error_handler()->OnUnrecoverableError(); return; diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index a122053..4b53582 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -72,7 +72,7 @@ class TestModelAssociator : public BookmarkModelAssociator { } sync_api::WriteNode node(&trans); // Create new fake tagged nodes at the end of the ordering. - node.InitByCreation(root, predecessor); + node.InitByCreation(syncable::BOOKMARKS, root, predecessor); node.SetIsFolder(true); node.SetTitle(tag_wide); node.SetExternalId(0); @@ -146,12 +146,13 @@ class FakeServerChange { EXPECT_TRUE(parent.InitByIdLookup(parent_id)); sync_api::WriteNode node(trans_); if (predecessor_id == 0) { - EXPECT_TRUE(node.InitByCreation(parent, NULL)); + EXPECT_TRUE(node.InitByCreation(syncable::BOOKMARKS, parent, NULL)); } else { sync_api::ReadNode predecessor(trans_); EXPECT_TRUE(predecessor.InitByIdLookup(predecessor_id)); EXPECT_EQ(predecessor.GetParentId(), parent.GetId()); - EXPECT_TRUE(node.InitByCreation(parent, &predecessor)); + EXPECT_TRUE(node.InitByCreation(syncable::BOOKMARKS, parent, + &predecessor)); } EXPECT_EQ(node.GetPredecessorId(), predecessor_id); EXPECT_EQ(node.GetParentId(), parent_id); diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc index 6df61be..4cdf7c2 100755 --- a/chrome/browser/sync/syncable/directory_backing_store.cc +++ b/chrome/browser/sync/syncable/directory_backing_store.cc @@ -14,7 +14,9 @@ #include "base/hash_tables.h" #include "base/logging.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/protocol/service_constants.h" +#include "chrome/browser/sync/protocol/sync.pb.h" #include "chrome/browser/sync/syncable/syncable-inl.h" #include "chrome/browser/sync/syncable/syncable_columns.h" #include "chrome/browser/sync/util/crypto_helpers.h" @@ -36,13 +38,7 @@ namespace syncable { static const string::size_type kUpdateStatementBufferSize = 2048; // Increment this version whenever updating DB tables. -static const int32 kCurrentDBVersion = 68; - -static void RegisterPathNameCollate(sqlite3* dbhandle) { - const int collate = SQLITE_UTF8; - CHECK(SQLITE_OK == sqlite3_create_collation(dbhandle, "PATHNAME", collate, - NULL, &ComparePathNames16)); -} +extern const int32 kCurrentDBVersion = 69; // Extern only for our unittest. namespace { @@ -58,12 +54,13 @@ int ExecQuery(sqlite3* dbhandle, const char* query) { return result; } -} // namespace - -static string GenerateCacheGUID() { +string GenerateCacheGUID() { return Generate128BitRandomHexString(); } +} // namespace + + // Iterate over the fields of |entry| and bind each to |statement| for // updating. Returns the number of args bound. int BindFields(const EntryKernel& entry, SQLStatement* statement) { @@ -81,11 +78,10 @@ int BindFields(const EntryKernel& entry, SQLStatement* statement) { for ( ; i < STRING_FIELDS_END; ++i) { statement->bind_string(index++, entry.ref(static_cast(i))); } - for ( ; i < BLOB_FIELDS_END; ++i) { - uint8* blob = entry.ref(static_cast(i)).empty() ? - NULL : const_cast(&entry.ref(static_cast(i)).at(0)); - statement->bind_blob(index++, blob, - entry.ref(static_cast(i)).size()); + std::string temp; + for ( ; i < PROTO_FIELDS_END; ++i) { + entry.ref(static_cast(i)).SerializeToString(&temp); + statement->bind_blob(index++, temp.data(), temp.length()); } return index; } @@ -110,12 +106,12 @@ EntryKernel* UnpackEntry(SQLStatement* statement) { result->put(static_cast(i), (0 != statement->column_int(i))); } for ( ; i < STRING_FIELDS_END; ++i) { - result->put(static_cast(i), + result->put(static_cast(i), statement->column_string(i)); } - for ( ; i < BLOB_FIELDS_END; ++i) { - statement->column_blob_as_vector( - i, &result->mutable_ref(static_cast(i))); + for ( ; i < PROTO_FIELDS_END; ++i) { + result->mutable_ref(static_cast(i)).ParseFromArray( + statement->column_blob(i), statement->column_bytes(i)); } ZeroFields(result, i); } else { @@ -125,8 +121,11 @@ EntryKernel* UnpackEntry(SQLStatement* statement) { return result; } -static string ComposeCreateTableColumnSpecs(const ColumnSpec* begin, - const ColumnSpec* end) { +namespace { + +string ComposeCreateTableColumnSpecs() { + const ColumnSpec* begin = g_metas_columns; + const ColumnSpec* end = g_metas_columns + arraysize(g_metas_columns); string query; query.reserve(kUpdateStatementBufferSize); char separator = '('; @@ -141,6 +140,18 @@ static string ComposeCreateTableColumnSpecs(const ColumnSpec* begin, return query; } +void AppendColumnList(std::string* output) { + const char* joiner = " "; + // Be explicit in SELECT order to match up with UnpackEntry. + for (int i = BEGIN_FIELDS; i < BEGIN_FIELDS + FIELD_COUNT; ++i) { + output->append(joiner); + output->append(ColumnName(i)); + joiner = ", "; + } +} + +} // namespace + /////////////////////////////////////////////////////////////////////////////// // DirectoryBackingStore implementation. @@ -149,7 +160,8 @@ DirectoryBackingStore::DirectoryBackingStore(const string& dir_name, : load_dbhandle_(NULL), save_dbhandle_(NULL), dir_name_(dir_name), - backing_filepath_(backing_filepath) { + backing_filepath_(backing_filepath), + needs_column_refresh_(false) { } DirectoryBackingStore::~DirectoryBackingStore() { @@ -182,7 +194,6 @@ bool DirectoryBackingStore::OpenAndConfigureHandleHelper( } } sqlite3_busy_timeout(*handle, kDirectoryBackingStoreBusyTimeoutMs); - RegisterPathNameCollate(*handle); #if defined(OS_WIN) // Do not index this file. Scanning can occur every time we close the file, // which causes long delays in SQLite's file locking. @@ -200,8 +211,7 @@ bool DirectoryBackingStore::OpenAndConfigureHandleHelper( DirOpenResult DirectoryBackingStore::Load(MetahandlesIndex* entry_bucket, ExtendedAttributes* xattrs_bucket, Directory::KernelLoadInfo* kernel_load_info) { - DCHECK(load_dbhandle_ == NULL); - if (!OpenAndConfigureHandleHelper(&load_dbhandle_)) + if (!BeginLoad()) return FAILED_OPEN_DATABASE; DirOpenResult result = InitializeTables(); @@ -213,10 +223,18 @@ DirOpenResult DirectoryBackingStore::Load(MetahandlesIndex* entry_bucket, LoadExtendedAttributes(xattrs_bucket); LoadInfo(kernel_load_info); + EndLoad(); + return OPENED; +} + +bool DirectoryBackingStore::BeginLoad() { + DCHECK(load_dbhandle_ == NULL); + return OpenAndConfigureHandleHelper(&load_dbhandle_); +} + +void DirectoryBackingStore::EndLoad() { sqlite3_close(load_dbhandle_); load_dbhandle_ = NULL; // No longer used. - - return OPENED; } bool DirectoryBackingStore::SaveChanges( @@ -273,25 +291,38 @@ DirOpenResult DirectoryBackingStore::InitializeTables() { if (SQLITE_OK != transaction.BeginExclusive()) { return FAILED_DISK_FULL; } - int version_on_disk = 0; + int version_on_disk = GetVersion(); int last_result = SQLITE_OK; - if (DoesSqliteTableExist(load_dbhandle_, "share_version")) { - SQLStatement version_query; - version_query.prepare(load_dbhandle_, "SELECT data from share_version"); - last_result = version_query.step(); - if (SQLITE_ROW == last_result) { - version_on_disk = version_query.column_int(0); - } - last_result = version_query.reset(); + // Upgrade from version 67. Version 67 was widely distributed as the original + // Bookmark Sync release. Version 68 removed unique naming. + if (version_on_disk == 67) { + if (MigrateVersion67To68()) + version_on_disk = 68; + } + // Version 69 introduced additional datatypes. + if (version_on_disk == 68) { + if (MigrateVersion68To69()) + version_on_disk = 69; } + + // If one of the migrations requested it, drop columns that aren't current. + // It's only safe to do this after migrating all the way to the current + // version. + if (version_on_disk == kCurrentDBVersion && needs_column_refresh_) { + if (!RefreshColumns()) + version_on_disk = 0; + } + + // A final, alternative catch-all migration to simply re-sync everything. if (version_on_disk != kCurrentDBVersion) { if (version_on_disk > kCurrentDBVersion) { transaction.Rollback(); return FAILED_NEWER_VERSION; } + // Fallback (re-sync everything) migration path. LOG(INFO) << "Old/null sync database, version " << version_on_disk; - // Delete the existing database (if any), and create a freshone. + // Delete the existing database (if any), and create a fresh one. if (SQLITE_OK == last_result) { DropAllTables(); if (SQLITE_DONE == CreateTables()) { @@ -323,17 +354,40 @@ DirOpenResult DirectoryBackingStore::InitializeTables() { return FAILED_DISK_FULL; } +bool DirectoryBackingStore::RefreshColumns() { + DCHECK(needs_column_refresh_); + + // Create a new table named temp_metas. + SafeDropTable("temp_metas"); + if (CreateMetasTable(true) != SQLITE_DONE) + return false; + + // Populate temp_metas from metas. + std::string query = "INSERT INTO temp_metas ("; + AppendColumnList(&query); + query.append(") SELECT "); + AppendColumnList(&query); + query.append(" FROM metas"); + if (ExecQuery(load_dbhandle_, query.c_str()) != SQLITE_DONE) + return false; + + // Drop metas. + SafeDropTable("metas"); + + // Rename temp_metas -> metas. + int result = ExecQuery(load_dbhandle_, + "ALTER TABLE temp_metas RENAME TO metas"); + if (result != SQLITE_DONE) + return false; + needs_column_refresh_ = false; + return true; +} + void DirectoryBackingStore::LoadEntries(MetahandlesIndex* entry_bucket) { string select; select.reserve(kUpdateStatementBufferSize); - select.append("SELECT"); - const char* joiner = " "; - // Be explicit in SELECT order to match up with UnpackEntry. - for (int i = BEGIN_FIELDS; i < BEGIN_FIELDS + FIELD_COUNT; ++i) { - select.append(joiner); - select.append(ColumnName(i)); - joiner = ", "; - } + select.append("SELECT "); + AppendColumnList(&select); select.append(" FROM metas "); SQLStatement statement; statement.prepare(load_dbhandle_, select.c_str()); @@ -402,7 +456,7 @@ bool DirectoryBackingStore::SaveEntryToDB(const EntryKernel& entry) { values.append("VALUES "); const char* separator = "( "; int i = 0; - for (i = BEGIN_FIELDS; i < BLOB_FIELDS_END; ++i) { + for (i = BEGIN_FIELDS; i < PROTO_FIELDS_END; ++i) { query.append(separator); values.append(separator); separator = ", "; @@ -517,11 +571,162 @@ int DirectoryBackingStore::CreateExtendedAttributeTable() { void DirectoryBackingStore::DropAllTables() { SafeDropTable("metas"); + SafeDropTable("temp_metas"); SafeDropTable("share_info"); SafeDropTable("share_version"); SafeDropTable("extended_attributes"); + needs_column_refresh_ = false; +} + +bool DirectoryBackingStore::MigrateToSpecifics( + const char* old_columns, + const char* specifics_column, + void (*handler_function)(SQLStatement* old_value_query, + int old_value_column, + sync_pb::EntitySpecifics* mutable_new_value)) { + std::string query_sql = StringPrintf("SELECT metahandle, %s, %s FROM metas", + specifics_column, old_columns); + std::string update_sql = StringPrintf( + "UPDATE metas SET %s = ? WHERE metahandle = ?", specifics_column); + SQLStatement query; + query.prepare(load_dbhandle_, query_sql.c_str()); + while (query.step() == SQLITE_ROW) { + int64 metahandle = query.column_int64(0); + std::string new_value_bytes; + query.column_blob_as_string(1, &new_value_bytes); + sync_pb::EntitySpecifics new_value; + new_value.ParseFromString(new_value_bytes); + handler_function(&query, 2, &new_value); + new_value.SerializeToString(&new_value_bytes); + + SQLStatement update; + update.prepare(load_dbhandle_, update_sql.data(), update_sql.length()); + update.bind_blob(0, new_value_bytes.data(), new_value_bytes.length()); + update.bind_int64(1, metahandle); + if (update.step() != SQLITE_DONE) { + NOTREACHED(); + return false; + } + } + return true; +} + +bool DirectoryBackingStore::AddColumn(const ColumnSpec* column) { + SQLStatement add_column; + std::string sql = StringPrintf("ALTER TABLE metas ADD COLUMN %s %s", + column->name, column->spec); + add_column.prepare(load_dbhandle_, sql.c_str()); + return add_column.step() == SQLITE_DONE; +} + +bool DirectoryBackingStore::SetVersion(int version) { + SQLStatement statement; + statement.prepare(load_dbhandle_, "UPDATE share_version SET data = ?"); + statement.bind_int(0, version); + return statement.step() == SQLITE_DONE; +} + +int DirectoryBackingStore::GetVersion() { + if (!DoesSqliteTableExist(load_dbhandle_, "share_version")) + return 0; + SQLStatement version_query; + version_query.prepare(load_dbhandle_, "SELECT data from share_version"); + if (SQLITE_ROW != version_query.step()) + return 0; + int value = version_query.column_int(0); + if (version_query.reset() != SQLITE_OK) + return 0; + return value; +} + +bool DirectoryBackingStore::MigrateVersion67To68() { + // This change simply removed three columns: + // string NAME + // string UNSANITIZED_NAME + // string SERVER_NAME + // No data migration is necessary, but we should do a column refresh. + SetVersion(68); + needs_column_refresh_ = true; + return true; +} + +namespace { + +// Callback passed to MigrateToSpecifics for the v68->v69 migration. See +// MigrateVersion68To69(). +void EncodeBookmarkURLAndFavicon(SQLStatement* old_value_query, + int old_value_column, + sync_pb::EntitySpecifics* mutable_new_value) { + // Extract data from the column trio we expect. + bool old_is_bookmark_object = old_value_query->column_bool(old_value_column); + std::string old_url = old_value_query->column_string(old_value_column + 1); + std::string old_favicon; + old_value_query->column_blob_as_string(old_value_column + 2, &old_favicon); + bool old_is_dir = old_value_query->column_bool(old_value_column + 3); + + if (old_is_bookmark_object) { + sync_pb::BookmarkSpecifics* bookmark_data = + mutable_new_value->MutableExtension(sync_pb::bookmark); + if (!old_is_dir) { + bookmark_data->set_url(old_url); + bookmark_data->set_favicon(old_favicon); + } + } } +} // namespace + +bool DirectoryBackingStore::MigrateVersion68To69() { + // In Version 68, there were columns on table 'metas': + // string BOOKMARK_URL + // string SERVER_BOOKMARK_URL + // blob BOOKMARK_FAVICON + // blob SERVER_BOOKMARK_FAVICON + // In version 69, these columns went away in favor of storing + // a serialized EntrySpecifics protobuf in the columns: + // protobuf blob SPECIFICS + // protobuf blob SERVER_SPECIFICS + // For bookmarks, EntrySpecifics is extended as per + // bookmark_specifics.proto. This migration converts bookmarks from the + // former scheme to the latter scheme. + + // First, add the two new columns to the schema. + if (!AddColumn(&g_metas_columns[SPECIFICS])) + return false; + if (!AddColumn(&g_metas_columns[SERVER_SPECIFICS])) + return false; + + // Next, fold data from the old columns into the new protobuf columns. + if (!MigrateToSpecifics(("is_bookmark_object, bookmark_url, " + "bookmark_favicon, is_dir"), + "specifics", + &EncodeBookmarkURLAndFavicon)) { + return false; + } + if (!MigrateToSpecifics(("server_is_bookmark_object, " + "server_bookmark_url, " + "server_bookmark_favicon, " + "server_is_dir"), + "server_specifics", + &EncodeBookmarkURLAndFavicon)) { + return false; + } + + // Lastly, fix up the "Google Chrome" folder, which is of the TOP_LEVEL_FOLDER + // ModelType: it shouldn't have BookmarkSpecifics. + SQLStatement clear_permanent_items; + clear_permanent_items.prepare(load_dbhandle_, + "UPDATE metas SET specifics = NULL, server_specifics = NULL WHERE " + "singleton_tag IN ('google_chrome')"); + if (clear_permanent_items.step() != SQLITE_DONE) + return false; + + SetVersion(69); + needs_column_refresh_ = true; // Trigger deletion of old columns. + return true; +} + + int DirectoryBackingStore::CreateTables() { LOG(INFO) << "First run, creating tables"; // Create two little tables share_version and share_info @@ -579,9 +784,7 @@ int DirectoryBackingStore::CreateTables() { if (result != SQLITE_DONE) return result; // Create the big metas table. - string query = "CREATE TABLE metas " + ComposeCreateTableColumnSpecs - (g_metas_columns, g_metas_columns + arraysize(g_metas_columns)); - result = ExecQuery(load_dbhandle_, query.c_str()); + result = CreateMetasTable(false); if (result != SQLITE_DONE) return result; { @@ -610,4 +813,12 @@ sqlite3* DirectoryBackingStore::LazyGetSaveHandle() { return save_dbhandle_; } +int DirectoryBackingStore::CreateMetasTable(bool is_temporary) { + const char* name = is_temporary ? "temp_metas" : "metas"; + string query = "CREATE TABLE "; + query.append(name); + query.append(ComposeCreateTableColumnSpecs()); + return ExecQuery(load_dbhandle_, query.c_str()); +} + } // namespace syncable diff --git a/chrome/browser/sync/syncable/directory_backing_store.h b/chrome/browser/sync/syncable/directory_backing_store.h index a3fa5093..a4294ae 100644 --- a/chrome/browser/sync/syncable/directory_backing_store.h +++ b/chrome/browser/sync/syncable/directory_backing_store.h @@ -11,12 +11,19 @@ #include "base/file_path.h" #include "chrome/browser/sync/syncable/dir_open_result.h" #include "chrome/browser/sync/syncable/syncable.h" +#include "testing/gtest/include/gtest/gtest_prod.h" // For FRIEND_TEST extern "C" { struct sqlite3; struct sqlite3_stmt; } +class SQLStatement; + +namespace sync_pb { +class EntitySpecifics; +} + namespace syncable { struct ColumnSpec; @@ -68,10 +75,18 @@ class DirectoryBackingStore { virtual bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot); private: + FRIEND_TEST(DirectoryBackingStoreTest, MigrateVersion67To68); + FRIEND_TEST(DirectoryBackingStoreTest, MigrateVersion68To69); + FRIEND_TEST(MigrationTest, ToCurrentVersion); + // General Directory initialization and load helpers. DirOpenResult InitializeTables(); // Returns an sqlite return code, usually SQLITE_DONE. int CreateTables(); + // Create 'metas' or 'temp_metas' depending on value of is_temporary. + // Returns an sqlite return code, SQLITE_DONE on success. + int CreateMetasTable(bool is_temporary); + int CreateExtendedAttributeTable(); // We don't need to load any synced and applied deleted entries, we can // in fact just purge them forever on startup. @@ -98,6 +113,9 @@ class DirectoryBackingStore { // said handle. Returns true on success, false if the sqlite open operation // did not succeed. bool OpenAndConfigureHandleHelper(sqlite3** handle) const; + // Initialize and destroy load_dbhandle_. Broken out for testing. + bool BeginLoad(); + void EndLoad(); // Lazy creation of save_dbhandle_ for use by SaveChanges code path. sqlite3* LazyGetSaveHandle(); @@ -105,6 +123,22 @@ class DirectoryBackingStore { // Drop all tables in preparation for reinitialization. void DropAllTables(); + // Migration utilities. + bool AddColumn(const ColumnSpec* column); + bool RefreshColumns(); + bool SetVersion(int version); + int GetVersion(); + bool MigrateToSpecifics(const char* old_columns, + const char* specifics_column, + void (*handler_function)( + SQLStatement* old_value_query, + int old_value_column, + sync_pb::EntitySpecifics* mutable_new_value)); + + // Individual version migrations. + bool MigrateVersion67To68(); + bool MigrateVersion68To69(); + // The handle to our sqlite on-disk store for initialization and loading, and // for saving changes periodically via SaveChanges, respectively. // TODO(timsteele): We should only have one handle here. The reason we need @@ -117,6 +151,10 @@ class DirectoryBackingStore { std::string dir_name_; FilePath backing_filepath_; + // Set to true if migration left some old columns around that need to be + // discarded. + bool needs_column_refresh_; + DISALLOW_COPY_AND_ASSIGN(DirectoryBackingStore); }; diff --git a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc new file mode 100755 index 0000000..2bdded7 --- /dev/null +++ b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc @@ -0,0 +1,523 @@ +// Copyright (c) 2010 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. + +#include "testing/gtest/include/gtest/gtest.h" + +#include + +#include "app/sql/connection.h" +#include "app/sql/statement.h" +#include "app/sql/transaction.h" +#include "base/file_path.h" +#include "base/scoped_ptr.h" +#include "base/scoped_temp_dir.h" +#include "base/stl_util-inl.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" +#include "chrome/browser/sync/protocol/sync.pb.h" +#include "chrome/browser/sync/syncable/directory_backing_store.h" +#include "chrome/browser/sync/syncable/directory_manager.h" +#include "chrome/browser/sync/syncable/syncable-inl.h" +#include "chrome/browser/sync/syncable/syncable.h" +#include "chrome/common/sqlite_utils.h" +#include "testing/gtest/include/gtest/gtest-param-test.h" + +namespace syncable { + +extern const int32 kCurrentDBVersion; + +class MigrationTest : public testing::TestWithParam { + public: + virtual void SetUp() { + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + } + + protected: + std::string GetUsername() { + return "nick@chromium.org"; + } + + FilePath GetDatabasePath() { + return temp_dir_.path().Append( + DirectoryManager::GetSyncDataDatabaseFilename()); + } + void SetUpVersion67Database(); + void SetUpVersion68Database(); + + private: + ScopedTempDir temp_dir_; +}; + +class DirectoryBackingStoreTest : public MigrationTest {}; + +void MigrationTest::SetUpVersion67Database() { + // This is a version 67 database dump whose contents were backformed from + // the contents of the version 68 database dump (the v68 migration was + // actually written first). + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(connection.BeginTransaction()); + ASSERT_TRUE(connection.Execute( + "CREATE TABLE extended_attributes(metahandle bigint, key varchar(127), " + "value blob, PRIMARY KEY(metahandle, key) ON CONFLICT REPLACE);" + "CREATE TABLE metas (metahandle bigint primary key ON CONFLICT FAIL," + "base_version bigint default -1,server_version bigint default 0," + "mtime bigint default 0,server_mtime bigint default 0," + "ctime bigint default 0,server_ctime bigint default 0," + "server_position_in_parent bigint default 0," + "local_external_id bigint default 0,id varchar(255) default 'r'," + "parent_id varchar(255) default 'r'," + "server_parent_id varchar(255) default 'r'," + "prev_id varchar(255) default 'r',next_id varchar(255) default 'r'," + "is_unsynced bit default 0,is_unapplied_update bit default 0," + "is_del bit default 0,is_dir bit default 0," + "is_bookmark_object bit default 0,server_is_dir bit default 0," + "server_is_del bit default 0,server_is_bookmark_object bit default 0," + "name varchar(255), " /* COLLATE PATHNAME, */ + "unsanitized_name varchar(255)," /* COLLATE PATHNAME, */ + "non_unique_name varchar," + "server_name varchar(255)," /* COLLATE PATHNAME */ + "server_non_unique_name varchar," + "bookmark_url varchar,server_bookmark_url varchar," + "singleton_tag varchar,bookmark_favicon blob," + "server_bookmark_favicon blob);" + "INSERT INTO metas VALUES(1,-1,0,129079956640320000,0," + "129079956640320000,0,0,0,'r','r','r','r','r',0,0,0,1,0,0,0,0,NULL," + "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL);" + "INSERT INTO metas VALUES(2,669,669,128976886618480000," + "128976886618480000,128976886618480000,128976886618480000,-2097152," + "4,'s_ID_2','s_ID_9','s_ID_9','s_ID_2','s_ID_2',0,0,1,0,1,0,1,1," + "'Deleted Item',NULL,'Deleted Item','Deleted Item','Deleted Item'," + "'http://www.google.com/','http://www.google.com/2',NULL,'AASGASGA'," + "'ASADGADGADG');" + "INSERT INTO metas VALUES(4,681,681,129002163642690000," + "129002163642690000,129002163642690000,129002163642690000,-3145728," + "3,'s_ID_4','s_ID_9','s_ID_9','s_ID_4','s_ID_4',0,0,1,0,1,0,1,1," + "'Welcome to Chromium',NULL,'Welcome to Chromium'," + "'Welcome to Chromium','Welcome to Chromium'," + "'http://www.google.com/chrome/intl/en/welcome.html'," + "'http://www.google.com/chrome/intl/en/welcome.html',NULL,NULL," + "NULL);" + "INSERT INTO metas VALUES(5,677,677,129001555500000000," + "129001555500000000,129001555500000000,129001555500000000,1048576," + "7,'s_ID_5','s_ID_9','s_ID_9','s_ID_5','s_ID_5',0,0,1,0,1,0,1,1," + "'Google',NULL,'Google','Google','Google','http://www.google.com/'," + "'http://www.google.com/',NULL,'AGASGASG','AGFDGASG');" + "INSERT INTO metas VALUES(6,694,694,129053976170000000," + "129053976170000000,129053976170000000,129053976170000000,-4194304," + "6,'s_ID_6','s_ID_9','s_ID_9','r','r',0,0,0,1,1,1,0,1," + "'The Internet',NULL,'The Internet','The Internet'," + "'The Internet',NULL,NULL,NULL,NULL,NULL);" + "INSERT INTO metas VALUES(7,663,663,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000," + "1048576,0,'s_ID_7','r','r','r','r',0,0,0,1,1,1,0,1," + "'Google Chrome',NULL,'Google Chrome','Google Chrome'," + "'Google Chrome',NULL,NULL,'google_chrome',NULL,NULL);" + "INSERT INTO metas VALUES(8,664,664,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000,1048576," + "0,'s_ID_8','s_ID_7','s_ID_7','r','r',0,0,0,1,1,1,0,1,'Bookmarks'," + "NULL,'Bookmarks','Bookmarks','Bookmarks',NULL,NULL," + "'google_chrome_bookmarks',NULL,NULL);" + "INSERT INTO metas VALUES(9,665,665,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000," + "1048576,1,'s_ID_9','s_ID_8','s_ID_8','r','s_ID_10',0,0,0,1,1,1,0," + "1,'Bookmark Bar',NULL,'Bookmark Bar','Bookmark Bar','Bookmark Bar'," + "NULL,NULL,'bookmark_bar',NULL,NULL);" + "INSERT INTO metas VALUES(10,666,666,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000,2097152," + "2,'s_ID_10','s_ID_8','s_ID_8','s_ID_9','r',0,0,0,1,1,1,0,1," + "'Other Bookmarks',NULL,'Other Bookmarks','Other Bookmarks'," + "'Other Bookmarks',NULL,NULL,'other_bookmarks'," + "NULL,NULL);" + "INSERT INTO metas VALUES(11,683,683,129079956948440000," + "129079956948440000,129079956948440000,129079956948440000,-1048576," + "8,'s_ID_11','s_ID_6','s_ID_6','r','s_ID_13',0,0,0,0,1,0,0,1," + "'Home (The Chromium Projects)',NULL,'Home (The Chromium Projects)'," + "'Home (The Chromium Projects)','Home (The Chromium Projects)'," + "'http://dev.chromium.org/','http://dev.chromium.org/other',NULL," + "'AGATWA','AFAGVASF');" + "INSERT INTO metas VALUES(12,685,685,129079957513650000," + "129079957513650000,129079957513650000,129079957513650000,0,9," + "'s_ID_12','s_ID_6','s_ID_6','s_ID_13','s_ID_14',0,0,0,1,1,1,0,1," + "'Extra Bookmarks',NULL,'Extra Bookmarks','Extra Bookmarks'," + "'Extra Bookmarks',NULL,NULL,NULL,NULL,NULL);" + "INSERT INTO metas VALUES(13,687,687,129079957985300000," + "129079957985300000,129079957985300000,129079957985300000,-917504," + "10,'s_ID_13','s_ID_6','s_ID_6','s_ID_11','s_ID_12',0,0,0,0,1,0,0," + "1,'ICANN | Internet Corporation for Assigned Names and Numbers'," + "'ICANN Internet Corporation for Assigned Names and Numbers'," + "'ICANN | Internet Corporation for Assigned Names and Numbers'," + "'ICANN | Internet Corporation for Assigned Names and Numbers'," + "'ICANN | Internet Corporation for Assigned Names and Numbers'," + "'http://www.icann.com/','http://www.icann.com/',NULL," + "'PNGAXF0AAFF','DAAFASF');" + "INSERT INTO metas VALUES(14,692,692,129079958383000000," + "129079958383000000,129079958383000000,129079958383000000,1048576," + "11,'s_ID_14','s_ID_6','s_ID_6','s_ID_12','r',0,0,0,0,1,0,0,1," + "'The WebKit Open Source Project',NULL," + "'The WebKit Open Source Project','The WebKit Open Source Project'," + "'The WebKit Open Source Project','http://webkit.org/'," + "'http://webkit.org/x',NULL,'PNGX','PNG2Y');" + "CREATE TABLE share_info (id VARCHAR(128) primary key, " + "last_sync_timestamp INT, name VARCHAR(128), " + "initial_sync_ended BIT default 0, store_birthday VARCHAR(256), " + "db_create_version VARCHAR(128), db_create_time int, " + "next_id bigint default -2, cache_guid VARCHAR(32));" + "INSERT INTO share_info VALUES('nick@chromium.org',694," + "'nick@chromium.org',1,'c27e9f59-08ca-46f8-b0cc-f16a2ed778bb'," + "'Unknown',1263522064,-65542," + "'9010788312004066376x-6609234393368420856x');" + "CREATE TABLE share_version (id VARCHAR(128) primary key, data INT);" + "INSERT INTO share_version VALUES('nick@chromium.org',68);")); + ASSERT_TRUE(connection.CommitTransaction()); +} + +void MigrationTest::SetUpVersion68Database() { + // This sets up an actual version 68 database dump. The IDs were + // canonicalized to be less huge, and the favicons were overwritten + // with random junk so that they didn't contain any unprintable + // characters. A few server URLs were tweaked so that they'd be + // different from the local URLs. Lastly, the custom collation on + // the server_non_unique_name column was removed. + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(connection.BeginTransaction()); + ASSERT_TRUE(connection.Execute( + "CREATE TABLE extended_attributes(metahandle bigint, key varchar(127), " + "value blob, PRIMARY KEY(metahandle, key) ON CONFLICT REPLACE);" + "CREATE TABLE metas (metahandle bigint primary key ON CONFLICT FAIL," + "base_version bigint default -1,server_version bigint default 0," + "mtime bigint default 0,server_mtime bigint default 0," + "ctime bigint default 0,server_ctime bigint default 0," + "server_position_in_parent bigint default 0," + "local_external_id bigint default 0,id varchar(255) default 'r'," + "parent_id varchar(255) default 'r'," + "server_parent_id varchar(255) default 'r'," + "prev_id varchar(255) default 'r',next_id varchar(255) default 'r'," + "is_unsynced bit default 0,is_unapplied_update bit default 0," + "is_del bit default 0,is_dir bit default 0," + "is_bookmark_object bit default 0,server_is_dir bit default 0," + "server_is_del bit default 0," + "server_is_bookmark_object bit default 0," + "non_unique_name varchar,server_non_unique_name varchar(255)," + "bookmark_url varchar,server_bookmark_url varchar," + "singleton_tag varchar,bookmark_favicon blob," + "server_bookmark_favicon blob);" + "INSERT INTO metas VALUES(1,-1,0,129079956640320000,0," + "129079956640320000,0,0,0,'r','r','r','r','r',0,0,0,1,0,0,0,0,NULL," + "NULL,NULL,NULL,NULL,NULL,NULL);" + "INSERT INTO metas VALUES(2,669,669,128976886618480000," + "128976886618480000,128976886618480000,128976886618480000,-2097152," + "4,'s_ID_2','s_ID_9','s_ID_9','s_ID_2','s_ID_2',0,0,1,0,1,0,1,1," + "'Deleted Item','Deleted Item','http://www.google.com/'," + "'http://www.google.com/2',NULL,'AASGASGA','ASADGADGADG');" + "INSERT INTO metas VALUES(4,681,681,129002163642690000," + "129002163642690000,129002163642690000,129002163642690000,-3145728," + "3,'s_ID_4','s_ID_9','s_ID_9','s_ID_4','s_ID_4',0,0,1,0,1,0,1,1," + "'Welcome to Chromium','Welcome to Chromium'," + "'http://www.google.com/chrome/intl/en/welcome.html'," + "'http://www.google.com/chrome/intl/en/welcome.html',NULL,NULL," + "NULL);" + "INSERT INTO metas VALUES(5,677,677,129001555500000000," + "129001555500000000,129001555500000000,129001555500000000,1048576," + "7,'s_ID_5','s_ID_9','s_ID_9','s_ID_5','s_ID_5',0,0,1,0,1,0,1,1," + "'Google','Google','http://www.google.com/'," + "'http://www.google.com/',NULL,'AGASGASG','AGFDGASG');" + "INSERT INTO metas VALUES(6,694,694,129053976170000000," + "129053976170000000,129053976170000000,129053976170000000,-4194304," + "6,'s_ID_6','s_ID_9','s_ID_9','r','r',0,0,0,1,1,1,0,1," + "'The Internet','The Internet',NULL,NULL,NULL,NULL,NULL);" + "INSERT INTO metas VALUES(7,663,663,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000," + "1048576,0,'s_ID_7','r','r','r','r',0,0,0,1,1,1,0,1," + "'Google Chrome','Google Chrome',NULL,NULL,'google_chrome',NULL," + "NULL);" + "INSERT INTO metas VALUES(8,664,664,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000,1048576," + "0,'s_ID_8','s_ID_7','s_ID_7','r','r',0,0,0,1,1,1,0,1,'Bookmarks'," + "'Bookmarks',NULL,NULL,'google_chrome_bookmarks',NULL,NULL);" + "INSERT INTO metas VALUES(9,665,665,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000," + "1048576,1,'s_ID_9','s_ID_8','s_ID_8','r','s_ID_10',0,0,0,1,1,1,0," + "1,'Bookmark Bar','Bookmark Bar',NULL,NULL,'bookmark_bar',NULL," + "NULL);" + "INSERT INTO metas VALUES(10,666,666,128976864758480000," + "128976864758480000,128976864758480000,128976864758480000,2097152," + "2,'s_ID_10','s_ID_8','s_ID_8','s_ID_9','r',0,0,0,1,1,1,0,1," + "'Other Bookmarks','Other Bookmarks',NULL,NULL,'other_bookmarks'," + "NULL,NULL);" + "INSERT INTO metas VALUES(11,683,683,129079956948440000," + "129079956948440000,129079956948440000,129079956948440000,-1048576," + "8,'s_ID_11','s_ID_6','s_ID_6','r','s_ID_13',0,0,0,0,1,0,0,1," + "'Home (The Chromium Projects)','Home (The Chromium Projects)'," + "'http://dev.chromium.org/','http://dev.chromium.org/other',NULL," + "'AGATWA','AFAGVASF');" + "INSERT INTO metas VALUES(12,685,685,129079957513650000," + "129079957513650000,129079957513650000,129079957513650000,0,9," + "'s_ID_12','s_ID_6','s_ID_6','s_ID_13','s_ID_14',0,0,0,1,1,1,0,1," + "'Extra Bookmarks','Extra Bookmarks',NULL,NULL,NULL,NULL,NULL);" + "INSERT INTO metas VALUES(13,687,687,129079957985300000," + "129079957985300000,129079957985300000,129079957985300000,-917504," + "10,'s_ID_13','s_ID_6','s_ID_6','s_ID_11','s_ID_12',0,0,0,0,1,0,0," + "1,'ICANN | Internet Corporation for Assigned Names and Numbers'," + "'ICANN | Internet Corporation for Assigned Names and Numbers'," + "'http://www.icann.com/','http://www.icann.com/',NULL," + "'PNGAXF0AAFF','DAAFASF');" + "INSERT INTO metas VALUES(14,692,692,129079958383000000," + "129079958383000000,129079958383000000,129079958383000000,1048576," + "11,'s_ID_14','s_ID_6','s_ID_6','s_ID_12','r',0,0,0,0,1,0,0,1," + "'The WebKit Open Source Project','The WebKit Open Source Project'," + "'http://webkit.org/','http://webkit.org/x',NULL,'PNGX','PNG2Y');" + "CREATE TABLE share_info (id VARCHAR(128) primary key, " + "last_sync_timestamp INT, name VARCHAR(128), " + "initial_sync_ended BIT default 0, store_birthday VARCHAR(256), " + "db_create_version VARCHAR(128), db_create_time int, " + "next_id bigint default -2, cache_guid VARCHAR(32));" + "INSERT INTO share_info VALUES('nick@chromium.org',694," + "'nick@chromium.org',1,'c27e9f59-08ca-46f8-b0cc-f16a2ed778bb'," + "'Unknown',1263522064,-65542," + "'9010788312004066376x-6609234393368420856x');" + "CREATE TABLE share_version (id VARCHAR(128) primary key, data INT);" + "INSERT INTO share_version VALUES('nick@chromium.org',68);")); + ASSERT_TRUE(connection.CommitTransaction()); +} + +TEST_F(DirectoryBackingStoreTest, MigrateVersion67To68) { + SetUpVersion67Database(); + + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + + // Columns existing befor version 67. + ASSERT_TRUE(connection.DoesColumnExist("metas", "name")); + ASSERT_TRUE(connection.DoesColumnExist("metas", "unsanitized_name")); + ASSERT_TRUE(connection.DoesColumnExist("metas", "server_name")); + } + + scoped_ptr dbs( + new DirectoryBackingStore(GetUsername(), GetDatabasePath())); + + dbs->BeginLoad(); + ASSERT_FALSE(dbs->needs_column_refresh_); + ASSERT_TRUE(dbs->MigrateVersion67To68()); + ASSERT_EQ(68, dbs->GetVersion()); + dbs->EndLoad(); + ASSERT_TRUE(dbs->needs_column_refresh_); +} + +TEST_F(DirectoryBackingStoreTest, MigrateVersion68To69) { + SetUpVersion68Database(); + + scoped_ptr dbs( + new DirectoryBackingStore(GetUsername(), GetDatabasePath())); + + dbs->BeginLoad(); + ASSERT_FALSE(dbs->needs_column_refresh_); + ASSERT_TRUE(dbs->MigrateVersion68To69()); + ASSERT_EQ(69, dbs->GetVersion()); + dbs->EndLoad(); + ASSERT_TRUE(dbs->needs_column_refresh_); + + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(connection.DoesColumnExist("metas", "specifics")); + ASSERT_TRUE(connection.DoesColumnExist("metas", "server_specifics")); + sql::Statement s(connection.GetUniqueStatement("SELECT non_unique_name," + "is_del, is_dir, id, specifics, server_specifics FROM metas " + "WHERE metahandle = 2")); + ASSERT_TRUE(s.Step()); + ASSERT_EQ("Deleted Item", s.ColumnString(0)); + ASSERT_TRUE(s.ColumnBool(1)); + ASSERT_FALSE(s.ColumnBool(2)); + ASSERT_EQ("s_ID_2", s.ColumnString(3)); + sync_pb::EntitySpecifics specifics; + specifics.ParseFromArray(s.ColumnBlob(4), s.ColumnByteLength(4)); + ASSERT_TRUE(specifics.HasExtension(sync_pb::bookmark)); + ASSERT_EQ("http://www.google.com/", + specifics.GetExtension(sync_pb::bookmark).url()); + ASSERT_EQ("AASGASGA", specifics.GetExtension(sync_pb::bookmark).favicon()); + specifics.ParseFromArray(s.ColumnBlob(5), s.ColumnByteLength(5)); + ASSERT_TRUE(specifics.HasExtension(sync_pb::bookmark)); + ASSERT_EQ("http://www.google.com/2", + specifics.GetExtension(sync_pb::bookmark).url()); + ASSERT_EQ("ASADGADGADG", specifics.GetExtension(sync_pb::bookmark).favicon()); + ASSERT_FALSE(s.Step()); +} + +TEST_P(MigrationTest, ToCurrentVersion) { + switch (GetParam()) { + case 67: + SetUpVersion67Database(); + break; + case 68: + SetUpVersion68Database(); + break; + default: + FAIL() << "Need to supply database dump for version " << GetParam(); + } + + scoped_ptr dbs( + new DirectoryBackingStore(GetUsername(), GetDatabasePath())); + + dbs->BeginLoad(); + ASSERT_TRUE(OPENED == dbs->InitializeTables()); + ASSERT_FALSE(dbs->needs_column_refresh_); + ASSERT_EQ(kCurrentDBVersion, dbs->GetVersion()); + + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + + // Columns deleted in Version 67. + ASSERT_FALSE(connection.DoesColumnExist("metas", "name")); + ASSERT_FALSE(connection.DoesColumnExist("metas", "unsanitized_name")); + ASSERT_FALSE(connection.DoesColumnExist("metas", "server_name")); + + // Columns added in Version 68. + ASSERT_TRUE(connection.DoesColumnExist("metas", "specifics")); + ASSERT_TRUE(connection.DoesColumnExist("metas", "server_specifics")); + + // Columns deleted in Version 68. + ASSERT_FALSE(connection.DoesColumnExist("metas", "is_bookmark_object")); + ASSERT_FALSE(connection.DoesColumnExist("metas", + "server_is_bookmark_object")); + ASSERT_FALSE(connection.DoesColumnExist("metas", "bookmark_favicon")); + ASSERT_FALSE(connection.DoesColumnExist("metas", "bookmark_url")); + ASSERT_FALSE(connection.DoesColumnExist("metas", "server_bookmark_url")); + } + + MetahandlesIndex index; + dbs->LoadEntries(&index); + dbs->EndLoad(); + + MetahandlesIndex::iterator it = index.begin(); + ASSERT_TRUE(it != index.end()); + ASSERT_EQ(1, (*it)->ref(META_HANDLE)); + EXPECT_TRUE((*it)->ref(ID).IsRoot()); + + ASSERT_TRUE(++it != index.end()) << "Upgrade destroyed database contents."; + ASSERT_EQ(2, (*it)->ref(META_HANDLE)); + EXPECT_TRUE((*it)->ref(IS_DEL)); + EXPECT_TRUE((*it)->ref(SERVER_IS_DEL)); + EXPECT_TRUE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_TRUE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_EQ("http://www.google.com/", + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).url()); + EXPECT_EQ("AASGASGA", + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).favicon()); + EXPECT_EQ("http://www.google.com/2", + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).url()); + EXPECT_EQ("ASADGADGADG", + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).favicon()); + EXPECT_EQ("", (*it)->ref(SINGLETON_TAG)); + EXPECT_EQ("Deleted Item", (*it)->ref(NON_UNIQUE_NAME)); + EXPECT_EQ("Deleted Item", (*it)->ref(SERVER_NON_UNIQUE_NAME)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(4, (*it)->ref(META_HANDLE)); + EXPECT_TRUE((*it)->ref(IS_DEL)); + EXPECT_TRUE((*it)->ref(SERVER_IS_DEL)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(5, (*it)->ref(META_HANDLE)); + EXPECT_TRUE((*it)->ref(IS_DEL)); + EXPECT_TRUE((*it)->ref(SERVER_IS_DEL)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(6, (*it)->ref(META_HANDLE)); + EXPECT_TRUE((*it)->ref(IS_DIR)); + EXPECT_TRUE((*it)->ref(SERVER_IS_DIR)); + EXPECT_FALSE( + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).has_url()); + EXPECT_FALSE( + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).has_url()); + EXPECT_FALSE( + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).has_favicon()); + EXPECT_FALSE((*it)->ref(SERVER_SPECIFICS). + GetExtension(sync_pb::bookmark).has_favicon()); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(7, (*it)->ref(META_HANDLE)); + EXPECT_EQ("google_chrome", (*it)->ref(SINGLETON_TAG)); + EXPECT_FALSE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_FALSE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(8, (*it)->ref(META_HANDLE)); + EXPECT_EQ("google_chrome_bookmarks", (*it)->ref(SINGLETON_TAG)); + EXPECT_TRUE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_TRUE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(9, (*it)->ref(META_HANDLE)); + EXPECT_EQ("bookmark_bar", (*it)->ref(SINGLETON_TAG)); + EXPECT_TRUE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_TRUE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(10, (*it)->ref(META_HANDLE)); + EXPECT_FALSE((*it)->ref(IS_DEL)); + EXPECT_TRUE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_TRUE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_FALSE((*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).has_url()); + EXPECT_FALSE( + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).has_favicon()); + EXPECT_FALSE( + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).has_url()); + EXPECT_FALSE((*it)->ref(SERVER_SPECIFICS). + GetExtension(sync_pb::bookmark).has_favicon()); + EXPECT_EQ("other_bookmarks", (*it)->ref(SINGLETON_TAG)); + EXPECT_EQ("Other Bookmarks", (*it)->ref(NON_UNIQUE_NAME)); + EXPECT_EQ("Other Bookmarks", (*it)->ref(SERVER_NON_UNIQUE_NAME)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(11, (*it)->ref(META_HANDLE)); + EXPECT_FALSE((*it)->ref(IS_DEL)); + EXPECT_FALSE((*it)->ref(IS_DIR)); + EXPECT_TRUE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_TRUE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_EQ("http://dev.chromium.org/", + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).url()); + EXPECT_EQ("AGATWA", + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).favicon()); + EXPECT_EQ("http://dev.chromium.org/other", + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).url()); + EXPECT_EQ("AFAGVASF", + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).favicon()); + EXPECT_EQ("", (*it)->ref(SINGLETON_TAG)); + EXPECT_EQ("Home (The Chromium Projects)", (*it)->ref(NON_UNIQUE_NAME)); + EXPECT_EQ("Home (The Chromium Projects)", (*it)->ref(SERVER_NON_UNIQUE_NAME)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(12, (*it)->ref(META_HANDLE)); + EXPECT_FALSE((*it)->ref(IS_DEL)); + EXPECT_TRUE((*it)->ref(IS_DIR)); + EXPECT_EQ("Extra Bookmarks", (*it)->ref(NON_UNIQUE_NAME)); + EXPECT_EQ("Extra Bookmarks", (*it)->ref(SERVER_NON_UNIQUE_NAME)); + EXPECT_TRUE((*it)->ref(SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_TRUE((*it)->ref(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)); + EXPECT_FALSE( + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).has_url()); + EXPECT_FALSE( + (*it)->ref(SERVER_SPECIFICS).GetExtension(sync_pb::bookmark).has_url()); + EXPECT_FALSE( + (*it)->ref(SPECIFICS).GetExtension(sync_pb::bookmark).has_favicon()); + EXPECT_FALSE((*it)->ref(SERVER_SPECIFICS). + GetExtension(sync_pb::bookmark).has_favicon()); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(13, (*it)->ref(META_HANDLE)); + + ASSERT_TRUE(++it != index.end()); + ASSERT_EQ(14, (*it)->ref(META_HANDLE)); + + ASSERT_TRUE(++it == index.end()); + + STLDeleteElements(&index); +} + +INSTANTIATE_TEST_CASE_P(DirectoryBackingStore, MigrationTest, + testing::Range(67, kCurrentDBVersion)); + +} // namespace syncable diff --git a/chrome/browser/sync/syncable/model_type.h b/chrome/browser/sync/syncable/model_type.h index 7bd9826..cc00f4c4 100644 --- a/chrome/browser/sync/syncable/model_type.h +++ b/chrome/browser/sync/syncable/model_type.h @@ -2,6 +2,10 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +// Enumerate the various item subtypes that are supported by sync. +// Each sync object is expected to have an immutable object type. +// An object's type is inferred from the type of data it holds. + #ifndef CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ #define CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ @@ -10,9 +14,14 @@ namespace syncable { enum ModelType { - UNSPECIFIED = 0, - BOOKMARKS, - MODEL_TYPE_COUNT + UNSPECIFIED, // Object type unknown. Objects may transition through + // the unknown state during their initial creation, before + // their properties are set. After deletion, object types + // are generally preserved. + TOP_LEVEL_FOLDER, // A permanent folder whose children may be of mixed + // datatypes (e.g. the "Google Chrome" folder). + BOOKMARKS, // A bookmark folder or a bookmark URL object. + MODEL_TYPE_COUNT, }; inline ModelType ModelTypeFromInt(int i) { @@ -24,4 +33,3 @@ inline ModelType ModelTypeFromInt(int i) { } // namespace syncable #endif // CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ - diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc index 6816f9c..b8c66b7 100755 --- a/chrome/browser/sync/syncable/syncable.cc +++ b/chrome/browser/sync/syncable/syncable.cc @@ -35,6 +35,7 @@ #include "base/time.h" #include "chrome/browser/sync/engine/syncer.h" #include "chrome/browser/sync/engine/syncer_util.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/protocol/service_constants.h" #include "chrome/browser/sync/syncable/directory_backing_store.h" #include "chrome/browser/sync/syncable/directory_manager.h" @@ -396,8 +397,8 @@ void ZeroFields(EntryKernel* entry, int first_field) { entry->mutable_ref(static_cast(i)).Clear(); for ( ; i < BIT_FIELDS_END; ++i) entry->put(static_cast(i), false); - if (i < BLOB_FIELDS_END) - i = BLOB_FIELDS_END; + if (i < PROTO_FIELDS_END) + i = PROTO_FIELDS_END; } void Directory::InsertEntry(EntryKernel* entry) { @@ -998,6 +999,51 @@ void Entry::DeleteAllExtendedAttributes(WriteTransaction *trans) { dir()->DeleteAllExtendedAttributes(trans, kernel_->ref(META_HANDLE)); } +syncable::ModelType Entry::GetServerModelType() const { + if (Get(SERVER_SPECIFICS).HasExtension(sync_pb::bookmark)) + return BOOKMARKS; + if (IsRoot()) + return TOP_LEVEL_FOLDER; + // Loose check for server-created top-level folders that aren't + // bound to a particular model type. + if (!Get(SINGLETON_TAG).empty() && Get(SERVER_IS_DIR)) + return TOP_LEVEL_FOLDER; + + // Otherwise, we don't have a server type yet. That should only happen + // if the item is an uncommitted locally created item. + // It's possible we'll need to relax these checks in the future; they're + // just here for now as a safety measure. + DCHECK(Get(IS_UNSYNCED)); + DCHECK(Get(SERVER_VERSION) == 0); + DCHECK(Get(SERVER_IS_DEL)); + // Note: can't enforce !Get(ID).ServerKnows() here because that could + // actually happen if we hit AttemptReuniteLostCommitResponses. + return UNSPECIFIED; +} + +// Note: keep this consistent with GetModelType in syncproto.h! +syncable::ModelType Entry::GetModelType() const { + if (Get(SPECIFICS).HasExtension(sync_pb::bookmark)) + return BOOKMARKS; + if (IsRoot()) + return TOP_LEVEL_FOLDER; + // Loose check for server-created top-level folders that aren't + // bound to a particular model type. + if (!Get(SINGLETON_TAG).empty() && Get(IS_DIR)) + return TOP_LEVEL_FOLDER; + + // Otherwise, we don't have a local type yet. That should only happen + // if the item doesn't exist locally -- like if it's a new update item. + // It's possible we'll need to relax these checks in the future; they're + // just here for now as a safety measure. + DCHECK(Get(ID).ServerKnows()); + DCHECK(Get(IS_UNAPPLIED_UPDATE)); + DCHECK(!Get(IS_UNSYNCED)); + DCHECK(Get(BASE_VERSION) == CHANGES_VERSION); + DCHECK(Get(IS_DEL)); + return UNSPECIFIED; +} + /////////////////////////////////////////////////////////////////////////// // MutableEntry @@ -1176,9 +1222,6 @@ void MutableEntry::UnlinkFromOrder() { } bool MutableEntry::PutPredecessor(const Id& predecessor_id) { - // TODO(ncarter): Maybe there should be an independent HAS_POSITION bit? - if (!Get(IS_BOOKMARK_OBJECT)) - return true; UnlinkFromOrder(); if (Get(IS_DEL)) { @@ -1186,6 +1229,15 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) { return true; } + // TODO(ncarter): It should be possible to not maintain position for + // non-bookmark items. However, we'd need to robustly handle all possible + // permutations of setting IS_DEL and the SPECIFICS to identify the + // object type; or else, we'd need to add a ModelType to the + // MutableEntry's Create ctor. + // if (!ShouldMaintainPosition()) { + // return false; + // } + // This is classic insert-into-doubly-linked-list from CS 101 and your last // job interview. An "IsRoot" Id signifies the head or tail. Id successor_id; @@ -1356,22 +1408,22 @@ inline FastDump& operator<<(FastDump& dump, const DumpColon&) { std::ostream& operator<<(std::ostream& stream, const syncable::Entry& entry) { // Using ostreams directly here is dreadfully slow, because a mutex is // acquired for every <<. Users noticed it spiking CPU. - using syncable::BitField; - using syncable::BitTemp; - using syncable::BlobField; - using syncable::EntryKernel; - using syncable::g_metas_columns; - using syncable::IdField; - using syncable::Int64Field; - using syncable::StringField; using syncable::BEGIN_FIELDS; using syncable::BIT_FIELDS_END; using syncable::BIT_TEMPS_BEGIN; using syncable::BIT_TEMPS_END; - using syncable::BLOB_FIELDS_END; - using syncable::INT64_FIELDS_END; + using syncable::BitField; + using syncable::BitTemp; + using syncable::EntryKernel; using syncable::ID_FIELDS_END; + using syncable::INT64_FIELDS_END; + using syncable::IdField; + using syncable::Int64Field; + using syncable::PROTO_FIELDS_END; + using syncable::ProtoField; using syncable::STRING_FIELDS_END; + using syncable::StringField; + using syncable::g_metas_columns; int i; FastDump s(&stream); @@ -1393,9 +1445,10 @@ std::ostream& operator<<(std::ostream& stream, const syncable::Entry& entry) { const string& field = kernel->ref(static_cast(i)); s << g_metas_columns[i].name << colon << field << separator; } - for ( ; i < BLOB_FIELDS_END; ++i) { + for ( ; i < PROTO_FIELDS_END; ++i) { s << g_metas_columns[i].name << colon - << kernel->ref(static_cast(i)) << separator; + << kernel->ref(static_cast(i)).SerializeAsString() + << separator; } s << "TempFlags: "; for ( ; i < BIT_TEMPS_END; ++i) { diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h index 05b3052..0fe199e 100755 --- a/chrome/browser/sync/syncable/syncable.h +++ b/chrome/browser/sync/syncable/syncable.h @@ -24,6 +24,7 @@ #include "chrome/browser/sync/syncable/directory_event.h" #include "chrome/browser/sync/syncable/path_name_cmp.h" #include "chrome/browser/sync/syncable/syncable_id.h" +#include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/util/dbgq.h" #include "chrome/browser/sync/util/event_sys.h" #include "chrome/browser/sync/util/row_iterator.h" @@ -116,12 +117,8 @@ enum IsDelField { enum BitField { IS_DIR = IS_DEL + 1, - IS_BOOKMARK_OBJECT, - SERVER_IS_DIR, SERVER_IS_DEL, - SERVER_IS_BOOKMARK_OBJECT, - BIT_FIELDS_END }; @@ -135,9 +132,6 @@ enum StringField { NON_UNIQUE_NAME = STRING_FIELDS_BEGIN, // The server version of |NON_UNIQUE_NAME|. SERVER_NON_UNIQUE_NAME, - // For bookmark entries, the URL of the bookmark. - BOOKMARK_URL, - SERVER_BOOKMARK_URL, // A tag string which identifies this node as a particular top-level // permanent object. The tag can be thought of as a unique key that @@ -148,27 +142,25 @@ enum StringField { enum { STRING_FIELDS_COUNT = STRING_FIELDS_END - STRING_FIELDS_BEGIN, - BLOB_FIELDS_BEGIN = STRING_FIELDS_END + PROTO_FIELDS_BEGIN = STRING_FIELDS_END }; // From looking at the sqlite3 docs, it's not directly stated, but it // seems the overhead for storing a NULL blob is very small. -enum BlobField { - // For bookmark entries, the favicon data. These will be NULL for - // non-bookmark items. - BOOKMARK_FAVICON = BLOB_FIELDS_BEGIN, - SERVER_BOOKMARK_FAVICON, - BLOB_FIELDS_END, +enum ProtoField { + SPECIFICS = PROTO_FIELDS_BEGIN, + SERVER_SPECIFICS, + PROTO_FIELDS_END, }; enum { - BLOB_FIELDS_COUNT = BLOB_FIELDS_END - BLOB_FIELDS_BEGIN + PROTO_FIELDS_COUNT = PROTO_FIELDS_END - PROTO_FIELDS_BEGIN }; enum { - FIELD_COUNT = BLOB_FIELDS_END, + FIELD_COUNT = PROTO_FIELDS_END, // Past this point we have temporaries, stored in memory only. - BEGIN_TEMPS = BLOB_FIELDS_END, + BEGIN_TEMPS = PROTO_FIELDS_END, BIT_TEMPS_BEGIN = BEGIN_TEMPS, }; @@ -225,7 +217,7 @@ typedef std::set AttributeKeySet; struct EntryKernel { private: std::string string_fields[STRING_FIELDS_COUNT]; - Blob blob_fields[BLOB_FIELDS_COUNT]; + sync_pb::EntitySpecifics specifics_fields[PROTO_FIELDS_COUNT]; int64 int64_fields[INT64_FIELDS_COUNT]; Id id_fields[ID_FIELDS_COUNT]; std::bitset bit_fields; @@ -269,8 +261,8 @@ struct EntryKernel { inline void put(StringField field, const std::string& value) { string_fields[field - STRING_FIELDS_BEGIN] = value; } - inline void put(BlobField field, const Blob& value) { - blob_fields[field - BLOB_FIELDS_BEGIN] = value; + inline void put(ProtoField field, const sync_pb::EntitySpecifics& value) { + specifics_fields[field - PROTO_FIELDS_BEGIN].CopyFrom(value); } inline void put(BitTemp field, bool value) { bit_temps[field - BIT_TEMPS_BEGIN] = value; @@ -301,8 +293,8 @@ struct EntryKernel { inline const std::string& ref(StringField field) const { return string_fields[field - STRING_FIELDS_BEGIN]; } - inline const Blob& ref(BlobField field) const { - return blob_fields[field - BLOB_FIELDS_BEGIN]; + inline const sync_pb::EntitySpecifics& ref(ProtoField field) const { + return specifics_fields[field - PROTO_FIELDS_BEGIN]; } inline bool ref(BitTemp field) const { return bit_temps[field - BIT_TEMPS_BEGIN]; @@ -312,8 +304,8 @@ struct EntryKernel { inline std::string& mutable_ref(StringField field) { return string_fields[field - STRING_FIELDS_BEGIN]; } - inline Blob& mutable_ref(BlobField field) { - return blob_fields[field - BLOB_FIELDS_BEGIN]; + inline sync_pb::EntitySpecifics& mutable_ref(ProtoField field) { + return specifics_fields[field - PROTO_FIELDS_BEGIN]; } inline Id& mutable_ref(IdField field) { return id_fields[field - ID_FIELDS_BEGIN]; @@ -369,7 +361,7 @@ class Entry { return kernel_->ref(field); } const std::string& Get(StringField field) const; - inline Blob Get(BlobField field) const { + inline const sync_pb::EntitySpecifics& Get(ProtoField field) const { DCHECK(kernel_); return kernel_->ref(field); } @@ -378,6 +370,15 @@ class Entry { return kernel_->ref(field); } + ModelType GetServerModelType() const; + ModelType GetModelType() const; + + // If this returns false, we shouldn't bother maintaining + // a position value (sibling ordering) for this item. + bool ShouldMaintainPosition() const { + return GetModelType() == BOOKMARKS; + } + inline bool ExistsOnClientBecauseNameIsNonEmpty() const { DCHECK(kernel_); return !kernel_->ref(NON_UNIQUE_NAME).empty(); @@ -444,8 +445,15 @@ class MutableEntry : public Entry { bool Put(StringField field, const std::string& value); bool Put(BaseVersion field, int64 value); - inline bool Put(BlobField field, const Blob& value) { - return PutField(field, value); + inline bool Put(ProtoField field, const sync_pb::EntitySpecifics& value) { + DCHECK(kernel_); + // TODO(ncarter): This is unfortunately heavyweight. Can we do + // better? + if (kernel_->ref(field).SerializeAsString() != value.SerializeAsString()) { + kernel_->put(field, value); + kernel_->mark_dirty(); + } + return true; } inline bool Put(BitField field, bool value) { return PutField(field, value); diff --git a/chrome/browser/sync/syncable/syncable_columns.h b/chrome/browser/sync/syncable/syncable_columns.h index 92caf64..56f0212 100755 --- a/chrome/browser/sync/syncable/syncable_columns.h +++ b/chrome/browser/sync/syncable/syncable_columns.h @@ -44,21 +44,17 @@ static const ColumnSpec g_metas_columns[] = { {"is_unapplied_update", "bit default 0"}, {"is_del", "bit default 0"}, {"is_dir", "bit default 0"}, - {"is_bookmark_object", "bit default 0"}, {"server_is_dir", "bit default 0"}, {"server_is_del", "bit default 0"}, - {"server_is_bookmark_object", "bit default 0"}, ////////////////////////////////////// // Strings {"non_unique_name", "varchar"}, - {"server_non_unique_name", "varchar(255) COLLATE PATHNAME"}, - {"bookmark_url", "varchar"}, - {"server_bookmark_url", "varchar"}, + {"server_non_unique_name", "varchar(255)"}, {"singleton_tag", "varchar"}, ////////////////////////////////////// // Blobs. - {"bookmark_favicon", "blob"}, - {"server_bookmark_favicon", "blob"}, + {"specifics", "blob"}, + {"server_specifics", "blob"}, }; // At least enforce that there are equal number of column names and fields. diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc index a03028d..407508a 100755 --- a/chrome/browser/sync/syncable/syncable_unittest.cc +++ b/chrome/browser/sync/syncable/syncable_unittest.cc @@ -34,6 +34,7 @@ #include "base/logging.h" #include "base/platform_thread.h" #include "base/scoped_ptr.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/syncable/directory_backing_store.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/util/closure.h" @@ -626,7 +627,6 @@ TEST_F(SyncableDirectoryTest, TestSimpleFieldsPreservedDuringSaveChanges) { EntryKernel create_pre_save, update_pre_save; EntryKernel create_post_save, update_post_save; string create_name = "Create"; - string favicon_bytes = "PNG"; { WriteTransaction trans(dir_.get(), UNITTEST, __FILE__, __LINE__); @@ -634,10 +634,10 @@ TEST_F(SyncableDirectoryTest, TestSimpleFieldsPreservedDuringSaveChanges) { MutableEntry update(&trans, CREATE_NEW_UPDATE_ITEM, update_id); create.Put(IS_UNSYNCED, true); update.Put(IS_UNAPPLIED_UPDATE, true); - syncable::Blob fav(favicon_bytes.data(), - favicon_bytes.data() + favicon_bytes.size()); - create.Put(BOOKMARK_FAVICON, fav); - update.Put(BOOKMARK_FAVICON, fav); + sync_pb::EntitySpecifics specifics; + specifics.MutableExtension(sync_pb::bookmark)->set_favicon("PNG"); + specifics.MutableExtension(sync_pb::bookmark)->set_url("http://nowhere"); + create.Put(SPECIFICS, specifics); create_pre_save = create.GetKernelCopy(); update_pre_save = update.GetKernelCopy(); create_id = create.Get(ID); @@ -690,12 +690,12 @@ TEST_F(SyncableDirectoryTest, TestSimpleFieldsPreservedDuringSaveChanges) { update_post_save.ref((StringField)i)) << "String field #" << i << " changed during save/load"; } - for ( ; i < BLOB_FIELDS_END; ++i) { - EXPECT_EQ(create_pre_save.ref((BlobField)i), - create_post_save.ref((BlobField)i)) + for ( ; i < PROTO_FIELDS_END; ++i) { + EXPECT_EQ(create_pre_save.ref((ProtoField)i).SerializeAsString(), + create_post_save.ref((ProtoField)i).SerializeAsString()) << "Blob field #" << i << " changed during save/load"; - EXPECT_EQ(update_pre_save.ref((BlobField)i), - update_post_save.ref((BlobField)i)) + EXPECT_EQ(update_pre_save.ref((ProtoField)i).SerializeAsString(), + update_post_save.ref((ProtoField)i).SerializeAsString()) << "Blob field #" << i << " changed during save/load"; } } diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 779f4dc..0986369 100755 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1439,6 +1439,7 @@ 'browser/sync/sessions/status_controller_unittest.cc', 'browser/sync/sessions/sync_session_unittest.cc', 'browser/sync/syncable/syncable_id_unittest.cc', + 'browser/sync/syncable/directory_backing_store_unittest.cc', 'browser/sync/syncable/syncable_unittest.cc', 'browser/sync/util/crypto_helpers_unittest.cc', 'browser/sync/util/event_sys_unittest.cc', diff --git a/chrome/common/sqlite_utils.cc b/chrome/common/sqlite_utils.cc index 0fd8397..534a567 100644 --- a/chrome/common/sqlite_utils.cc +++ b/chrome/common/sqlite_utils.cc @@ -276,8 +276,8 @@ int SQLStatement::prepare(sqlite3* db, const char* sql, int sql_len) { DCHECK(!stmt_); int rv = sqlite3_prepare_v2(db, sql, sql_len, &stmt_, NULL); if (rv != SQLITE_OK) { - SQLErrorHandler* error_handler = GetErrorHandlerFactory()->Make(); - return error_handler->HandleError(rv, db_handle()); + SQLErrorHandler* error_handler = GetErrorHandlerFactory()->Make(); + return error_handler->HandleError(rv, db); } return rv; } diff --git a/chrome/test/sync/engine/mock_server_connection.cc b/chrome/test/sync/engine/mock_server_connection.cc index fdfb334..60d1eff 100755 --- a/chrome/test/sync/engine/mock_server_connection.cc +++ b/chrome/test/sync/engine/mock_server_connection.cc @@ -7,6 +7,7 @@ #include "chrome/test/sync/engine/mock_server_connection.h" #include "chrome/browser/sync/engine/syncer_proto_util.h" +#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" #include "chrome/browser/sync/util/character_set_converters.h" #include "chrome/test/sync/engine/test_id_factory.h" #include "testing/gtest/include/gtest/gtest.h" @@ -46,7 +47,8 @@ MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr, throttling_(false), fail_non_periodic_get_updates_(false), client_command_(NULL), - next_position_in_parent_(2) { + next_position_in_parent_(2), + use_legacy_bookmarks_protocol_(false) { server_reachable_ = true; }; @@ -152,19 +154,25 @@ void MockConnectionManager::ResetUpdates() { updates_.Clear(); } -namespace { +void MockConnectionManager::AddDefaultBookmarkData(sync_pb::SyncEntity* entity, + bool is_folder) { + if (use_legacy_bookmarks_protocol_) { + sync_pb::SyncEntity_BookmarkData* data = entity->mutable_bookmarkdata(); + data->set_bookmark_folder(is_folder); -void AddDefaultBookmarkData(SyncEntity* entity, bool is_folder) { - sync_pb::SyncEntity_BookmarkData* data = entity->mutable_bookmarkdata(); - data->set_bookmark_folder(is_folder); - - if (!is_folder) { - data->set_bookmark_url("http://google.com"); + if (!is_folder) { + data->set_bookmark_url("http://google.com"); + } + } else { + entity->set_folder(is_folder); + entity->mutable_specifics()->MutableExtension(sync_pb::bookmark); + if (!is_folder) { + entity->mutable_specifics()->MutableExtension(sync_pb::bookmark)-> + set_url("http://google.com"); + } } } -} // namespace - SyncEntity* MockConnectionManager::AddUpdateDirectory(int id, int parent_id, string name, @@ -199,6 +207,7 @@ SyncEntity* MockConnectionManager::AddUpdateFull(string id, string parent_id, SyncEntity* ent = updates_.add_entries(); ent->set_id_string(id); ent->set_parent_id_string(parent_id); + ent->set_non_unique_name(name); ent->set_name(name); ent->set_version(version); ent->set_sync_timestamp(sync_ts); diff --git a/chrome/test/sync/engine/mock_server_connection.h b/chrome/test/sync/engine/mock_server_connection.h index e8fa974..852ff07 100755 --- a/chrome/test/sync/engine/mock_server_connection.h +++ b/chrome/test/sync/engine/mock_server_connection.h @@ -98,7 +98,7 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { std::string* xattr_key, syncable::Blob* xattr_value, int xattr_count); - // Prepare to add checksums. + void SetLastUpdateDeleted(); void SetLastUpdateSingletonTag(const string& tag); void SetLastUpdateOriginatorFields(const string& client_id, @@ -149,6 +149,10 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { conflict_n_commits_ = value; } + void set_use_legacy_bookmarks_protocol(bool value) { + use_legacy_bookmarks_protocol_ = value; + } + private: sync_pb::SyncEntity* AddUpdateFull(syncable::Id id, syncable::Id parentid, string name, int64 version, @@ -165,6 +169,9 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { const std::string& auth_token); void ProcessCommit(sync_pb::ClientToServerMessage* csm, sync_pb::ClientToServerResponse* response_buffer); + + void AddDefaultBookmarkData(sync_pb::SyncEntity* entity, bool is_folder); + // Locate the most recent update message for purpose of alteration. sync_pb::SyncEntity* GetMutableLastUpdate(); @@ -227,6 +234,11 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { // The next value to use for the position_in_parent property. int64 next_position_in_parent_; + // The default is to use the newer sync_pb::BookmarkSpecifics-style protocol. + // If this option is set to true, then the MockConnectionManager will + // use the older sync_pb::SyncEntity_BookmarkData-style protocol. + bool use_legacy_bookmarks_protocol_; + DISALLOW_COPY_AND_ASSIGN(MockConnectionManager); }; -- cgit v1.1