diff options
32 files changed, 1462 insertions, 350 deletions
@@ -33,7 +33,7 @@ deps = { "/trunk/deps/third_party/hunspell128@30191", "src/third_party/protobuf2/src": - "http://protobuf.googlecode.com/svn/trunk@219", + "http://protobuf.googlecode.com/svn/trunk@305", "src/tools/gyp": "http://gyp.googlecode.com/svn/trunk@776", diff --git a/app/sql/connection.h b/app/sql/connection.h index 52587ac..6927c89 100644 --- a/app/sql/connection.h +++ b/app/sql/connection.h @@ -224,8 +224,8 @@ class Connection { // you having to manage unique names. See StatementID above for more. // // Example: - // sql::Statement stmt = connection_.GetCachedStatement( - // SQL_FROM_HERE, "SELECT * FROM foo"); + // sql::Statement stmt(connection_.GetCachedStatement( + // SQL_FROM_HERE, "SELECT * FROM foo")); // if (!stmt) // return false; // Error creating statement. scoped_refptr<StatementRef> GetCachedStatement(const StatementID& id, diff --git a/app/sql/statement.h b/app/sql/statement.h index 92495c7..22682c8 100644 --- a/app/sql/statement.h +++ b/app/sql/statement.h @@ -25,7 +25,7 @@ enum ColType { }; // Normal usage: -// sql::Statement s = connection_.GetUniqueStatement(...); +// sql::Statement s(connection_.GetUniqueStatement(...)); // if (!s) // You should check for errors before using the statement. // return false; // 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<unsigned char>* output) const { + if (!output) + return; + const std::string& favicon = GetBookmarkSpecifics().favicon(); + output->assign(reinterpret_cast<const unsigned char*>(favicon.data()), + reinterpret_cast<const unsigned char*>(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<unsigned char>& 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 <string> +#include <vector> #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<unsigned char>* 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<unsigned char>& 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<int, syncable::Id> 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<sync_pb::SyncEntity> { 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<unsigned char> 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<unsigned char> 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<unsigned char> 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<StringField>(i))); } - for ( ; i < BLOB_FIELDS_END; ++i) { - uint8* blob = entry.ref(static_cast<BlobField>(i)).empty() ? - NULL : const_cast<uint8*>(&entry.ref(static_cast<BlobField>(i)).at(0)); - statement->bind_blob(index++, blob, - entry.ref(static_cast<BlobField>(i)).size()); + std::string temp; + for ( ; i < PROTO_FIELDS_END; ++i) { + entry.ref(static_cast<ProtoField>(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<BitField>(i), (0 != statement->column_int(i))); } for ( ; i < STRING_FIELDS_END; ++i) { - result->put(static_cast<StringField>(i), + result->put(static_cast<StringField>(i), statement->column_string(i)); } - for ( ; i < BLOB_FIELDS_END; ++i) { - statement->column_blob_as_vector( - i, &result->mutable_ref(static_cast<BlobField>(i))); + for ( ; i < PROTO_FIELDS_END; ++i) { + result->mutable_ref(static_cast<ProtoField>(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 <string> + +#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<int> { + 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<DirectoryBackingStore> 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<DirectoryBackingStore> 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<DirectoryBackingStore> 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<IdField>(i)).Clear(); for ( ; i < BIT_FIELDS_END; ++i) entry->put(static_cast<BitField>(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<StringField>(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<BlobField>(i)) << separator; + << kernel->ref(static_cast<ProtoField>(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<std::string> 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_COUNT> 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); }; diff --git a/third_party/protobuf2/config.h b/third_party/protobuf2/config.h index cb0bb00..3d38f04 100644 --- a/third_party/protobuf2/config.h +++ b/third_party/protobuf2/config.h @@ -1,12 +1,18 @@ /* config.h. Generated from config.h.in by configure. */ /* config.h.in. Generated from configure.ac by autoheader. */ +/* the name of <hash_set> */ +#define HASH_MAP_CLASS hash_map + /* the location of <hash_map> */ #define HASH_MAP_H <ext/hash_map> /* the namespace of hash_map/hash_set */ #define HASH_NAMESPACE __gnu_cxx +/* the name of <hash_set> */ +#define HASH_SET_CLASS hash_set + /* the location of <hash_set> */ #define HASH_SET_H <ext/hash_set> @@ -86,13 +92,13 @@ #define PACKAGE_NAME "Protocol Buffers" /* Define to the full name and version of this package. */ -#define PACKAGE_STRING "Protocol Buffers 2.1.1-pre" +#define PACKAGE_STRING "Protocol Buffers 2.3.0" /* Define to the one symbol short name of this package. */ #define PACKAGE_TARNAME "protobuf" /* Define to the version of this package. */ -#define PACKAGE_VERSION "2.1.1-pre" +#define PACKAGE_VERSION "2.3.0" /* Define to necessary symbol if this constant uses a non-standard name on your system. */ @@ -102,7 +108,7 @@ #define STDC_HEADERS 1 /* Version number of package */ -#define VERSION "2.1.1-pre" +#define VERSION "2.3.0" /* Define to 1 if on AIX 3. System headers sometimes define this. diff --git a/third_party/protobuf2/protobuf.gyp b/third_party/protobuf2/protobuf.gyp index d96d131..95272e9c 100644 --- a/third_party/protobuf2/protobuf.gyp +++ b/third_party/protobuf2/protobuf.gyp @@ -64,6 +64,7 @@ 'src/src/google/protobuf/repeated_field.cc', 'src/src/google/protobuf/wire_format_lite.cc', 'src/src/google/protobuf/io/coded_stream.cc', + 'src/src/google/protobuf/io/coded_stream_inl.h', 'src/src/google/protobuf/io/zero_copy_stream.cc', 'src/src/google/protobuf/io/zero_copy_stream_impl_lite.cc', '<(config_h_dir)/config.h', @@ -116,10 +117,10 @@ 'src/src/google/protobuf/compiler/importer.h', 'src/src/google/protobuf/compiler/parser.h', - 'src/src/google/protobuf/stubs/substitute.cc', - 'src/src/google/protobuf/stubs/substitute.h', 'src/src/google/protobuf/stubs/strutil.cc', 'src/src/google/protobuf/stubs/strutil.h', + 'src/src/google/protobuf/stubs/substitute.cc', + 'src/src/google/protobuf/stubs/substitute.h', 'src/src/google/protobuf/stubs/structurally_valid.cc', 'src/src/google/protobuf/descriptor.cc', 'src/src/google/protobuf/descriptor.pb.cc', @@ -156,6 +157,12 @@ 'sources': [ 'src/src/google/protobuf/compiler/code_generator.cc', 'src/src/google/protobuf/compiler/command_line_interface.cc', + 'src/src/google/protobuf/compiler/plugin.cc', + 'src/src/google/protobuf/compiler/plugin.pb.cc', + 'src/src/google/protobuf/compiler/subprocess.cc', + 'src/src/google/protobuf/compiler/subprocess.h', + 'src/src/google/protobuf/compiler/zip_writer.cc', + 'src/src/google/protobuf/compiler/zip_writer.h', 'src/src/google/protobuf/compiler/cpp/cpp_enum.cc', 'src/src/google/protobuf/compiler/cpp/cpp_enum.h', 'src/src/google/protobuf/compiler/cpp/cpp_enum_field.cc', @@ -203,18 +210,16 @@ 'src/src/google/protobuf/compiler/python/python_generator.cc', 'src/src/google/protobuf/compiler/main.cc', ], - 'dependencies': [ 'protobuf', ], - 'include_dirs': [ '<(config_h_dir)', 'src/src', ], }, ], - } +} # Local Variables: # tab-width:2 |