diff options
Diffstat (limited to 'chromium/components/sync_bookmarks')
20 files changed, 710 insertions, 330 deletions
diff --git a/chromium/components/sync_bookmarks/BUILD.gn b/chromium/components/sync_bookmarks/BUILD.gn index c359c1f51ea..b04da3954d8 100644 --- a/chromium/components/sync_bookmarks/BUILD.gn +++ b/chromium/components/sync_bookmarks/BUILD.gn @@ -33,7 +33,6 @@ static_library("sync_bookmarks") { "//base", "//components/bookmarks/browser", "//components/favicon/core", - "//components/history/core/browser", "//components/keyed_service/core:core", "//components/sync", "//components/undo", @@ -62,7 +61,6 @@ source_set("unit_tests") { "//components/bookmarks/browser", "//components/bookmarks/test", "//components/favicon/core/test:test_support", - "//components/history/core/browser", "//components/prefs:test_support", "//components/sync:test_support", "//components/undo", diff --git a/chromium/components/sync_bookmarks/DEPS b/chromium/components/sync_bookmarks/DEPS index 4160f765ff3..b9eb4432cbf 100644 --- a/chromium/components/sync_bookmarks/DEPS +++ b/chromium/components/sync_bookmarks/DEPS @@ -3,7 +3,6 @@ include_rules = [ "+components/bookmarks/test", "+components/favicon/core", "+components/favicon_base", - "+components/history/core/browser", "+components/keyed_service", "+components/prefs", "+components/sync", diff --git a/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc b/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc index c404b8dd99a..83cb88cf79b 100644 --- a/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc +++ b/chromium/components/sync_bookmarks/bookmark_local_changes_builder.cc @@ -46,24 +46,24 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests( DCHECK(entity); DCHECK(entity->IsUnsynced()); - const sync_pb::EntityMetadata* metadata = entity->metadata(); + const sync_pb::EntityMetadata& metadata = entity->metadata(); auto data = std::make_unique<syncer::EntityData>(); - data->id = metadata->server_id(); - data->creation_time = syncer::ProtoTimeToTime(metadata->creation_time()); + data->id = metadata.server_id(); + data->creation_time = syncer::ProtoTimeToTime(metadata.creation_time()); data->modification_time = - syncer::ProtoTimeToTime(metadata->modification_time()); + syncer::ProtoTimeToTime(metadata.modification_time()); - DCHECK(!metadata->client_tag_hash().empty()); + DCHECK(!metadata.client_tag_hash().empty()); data->client_tag_hash = - syncer::ClientTagHash::FromHashed(metadata->client_tag_hash()); - DCHECK(metadata->is_deleted() || + syncer::ClientTagHash::FromHashed(metadata.client_tag_hash()); + DCHECK(metadata.is_deleted() || data->client_tag_hash == syncer::ClientTagHash::FromUnhashed( syncer::BOOKMARKS, entity->bookmark_node()->guid().AsLowercaseString())); - if (!metadata->is_deleted()) { + if (!metadata.is_deleted()) { const bookmarks::BookmarkNode* node = entity->bookmark_node(); DCHECK(!node->is_permanent_node()); @@ -81,17 +81,17 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests( DCHECK(node); DCHECK_EQ(syncer::ClientTagHash::FromUnhashed( syncer::BOOKMARKS, node->guid().AsLowercaseString()), - syncer::ClientTagHash::FromHashed(metadata->client_tag_hash())); + syncer::ClientTagHash::FromHashed(metadata.client_tag_hash())); const bookmarks::BookmarkNode* parent = node->parent(); const SyncedBookmarkTrackerEntity* parent_entity = bookmark_tracker_->GetEntityForBookmarkNode(parent); DCHECK(parent_entity); - data->legacy_parent_id = parent_entity->metadata()->server_id(); + data->legacy_parent_id = parent_entity->metadata().server_id(); // Assign specifics only for the non-deletion case. In case of deletion, // EntityData should contain empty specifics to indicate deletion. data->specifics = CreateSpecificsFromBookmarkNode( - node, bookmark_model_, metadata->unique_position(), + node, bookmark_model_, metadata.unique_position(), /*force_favicon_load=*/true); // TODO(crbug.com/1058376): check after finishing if we need to use full // title instead of legacy canonicalized one. @@ -100,11 +100,11 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests( auto request = std::make_unique<syncer::CommitRequestData>(); request->entity = std::move(data); - request->sequence_number = metadata->sequence_number(); - request->base_version = metadata->server_version(); + request->sequence_number = metadata.sequence_number(); + request->base_version = metadata.server_version(); // Specifics hash has been computed in the tracker when this entity has been // added/updated. - request->specifics_hash = metadata->specifics_hash(); + request->specifics_hash = metadata.specifics_hash(); bookmark_tracker_->MarkCommitMayHaveStarted(entity); diff --git a/chromium/components/sync_bookmarks/bookmark_model_merger.cc b/chromium/components/sync_bookmarks/bookmark_model_merger.cc index 996571963df..263a9869115 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_merger.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_merger.cc @@ -1044,7 +1044,7 @@ BookmarkModelMerger::GenerateUniquePositionForLocalCreation( if (predecessor_entity != nullptr) { return syncer::UniquePosition::After( syncer::UniquePosition::FromProto( - predecessor_entity->metadata()->unique_position()), + predecessor_entity->metadata().unique_position()), suffix); } DCHECK(FindMatchingRemoteNodeByGUID(parent->children()[i - 1].get())); diff --git a/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc b/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc index bf31be698ab..ff00f1fce57 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_merger_unittest.cc @@ -189,7 +189,7 @@ syncer::UniquePosition PositionOf(const bookmarks::BookmarkNode* node, const SyncedBookmarkTrackerEntity* entity = tracker.GetEntityForBookmarkNode(node); return syncer::UniquePosition::FromProto( - entity->metadata()->unique_position()); + entity->metadata().unique_position()); } bool PositionsInTrackerMatchModel(const bookmarks::BookmarkNode* node, diff --git a/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc b/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc index 9ef9d34f9b5..1035d50ae38 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_observer_impl.cc @@ -7,7 +7,7 @@ #include <utility> #include "base/guid.h" -#include "base/strings/utf_string_conversions.h" +#include "base/no_destructor.h" #include "components/bookmarks/browser/bookmark_model.h" #include "components/bookmarks/browser/bookmark_node.h" #include "components/sync/base/hash_util.h" @@ -17,9 +17,76 @@ #include "components/sync/protocol/entity_specifics.pb.h" #include "components/sync_bookmarks/bookmark_specifics_conversions.h" #include "components/sync_bookmarks/synced_bookmark_tracker_entity.h" +#include "third_party/abseil-cpp/absl/types/variant.h" namespace sync_bookmarks { +namespace { + +// A helper wrapper used to compare UniquePosition with positions before the +// first and after the last elements. +class UniquePositionWrapper { + public: + static UniquePositionWrapper Min() { + return UniquePositionWrapper(MinUniquePosition{}); + } + + static UniquePositionWrapper Max() { + return UniquePositionWrapper(MaxUniquePosition{}); + } + + // |unique_position| must be valid. + static UniquePositionWrapper ForValidUniquePosition( + syncer::UniquePosition unique_position) { + DCHECK(unique_position.IsValid()); + return UniquePositionWrapper(std::move(unique_position)); + } + + UniquePositionWrapper(UniquePositionWrapper&&) = default; + UniquePositionWrapper& operator=(UniquePositionWrapper&&) = default; + + // Returns valid UniquePosition or invalid one for Min() and Max(). + const syncer::UniquePosition& GetUniquePosition() const { + static const base::NoDestructor<syncer::UniquePosition> + kEmptyUniquePosition; + if (HoldsUniquePosition()) { + return absl::get<syncer::UniquePosition>(value_); + } + return *kEmptyUniquePosition; + } + + bool LessThan(const UniquePositionWrapper& other) const { + if (value_.index() != other.value_.index()) { + return value_.index() < other.value_.index(); + } + if (!HoldsUniquePosition()) { + // Both arguments are MinUniquePosition or MaxUniquePosition, in both + // cases they are equal. + return false; + } + return GetUniquePosition().LessThan(other.GetUniquePosition()); + } + + private: + struct MinUniquePosition {}; + struct MaxUniquePosition {}; + + explicit UniquePositionWrapper(absl::variant<MinUniquePosition, + syncer::UniquePosition, + MaxUniquePosition> value) + : value_(std::move(value)) {} + + bool HoldsUniquePosition() const { + return absl::holds_alternative<syncer::UniquePosition>(value_); + } + + // The order is used to compare positions. + absl::variant<MinUniquePosition, syncer::UniquePosition, MaxUniquePosition> + value_; +}; + +} // namespace + BookmarkModelObserverImpl::BookmarkModelObserverImpl( const base::RepeatingClosure& nudge_for_commit_closure, base::OnceClosure on_bookmark_model_being_deleted_closure, @@ -61,7 +128,7 @@ void BookmarkModelObserverImpl::BookmarkNodeMoved( bookmark_tracker_->GetEntityForBookmarkNode(node); DCHECK(entity); - const std::string& sync_id = entity->metadata()->server_id(); + const std::string& sync_id = entity->metadata().server_id(); const base::Time modification_time = base::Time::Now(); const syncer::UniquePosition unique_position = ComputePosition(*new_parent, new_index, sync_id); @@ -70,7 +137,7 @@ void BookmarkModelObserverImpl::BookmarkNodeMoved( CreateSpecificsFromBookmarkNode(node, model, unique_position.ToProto(), /*force_favicon_load=*/true); - bookmark_tracker_->Update(entity, entity->metadata()->server_version(), + bookmark_tracker_->Update(entity, entity->metadata().server_version(), modification_time, specifics); // Mark the entity that it needs to be committed. bookmark_tracker_->IncrementSequenceNumber(entity); @@ -109,7 +176,7 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded( // the bookmark model contains to bookmarks with the same GUID. DCHECK(!entity->bookmark_node()) << "Added bookmark with duplicate GUID"; bookmark_tracker_->UndeleteTombstoneForBookmarkNode(entity, node); - bookmark_tracker_->Update(entity, entity->metadata()->server_version(), + bookmark_tracker_->Update(entity, entity->metadata().server_version(), creation_time, specifics); } else { entity = bookmark_tracker_->Add(node, node->guid().AsLowercaseString(), @@ -200,7 +267,7 @@ void BookmarkModelObserverImpl::BookmarkNodeChanged( } sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode( - node, model, entity->metadata()->unique_position(), + node, model, entity->metadata().unique_position(), /*force_favicon_load=*/true); ProcessUpdate(entity, specifics); } @@ -240,7 +307,7 @@ void BookmarkModelObserverImpl::BookmarkNodeFaviconChanged( } const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode( - node, model, entity->metadata()->unique_position(), + node, model, entity->metadata().unique_position(), /*force_favicon_load=*/false); // TODO(crbug.com/1094825): implement |base_specifics_hash| similar to @@ -267,39 +334,100 @@ void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered( return; } - // The given node's children got reordered. We need to reorder all the - // corresponding sync node. - // TODO(crbug.com/1321519): children reordering is used to move one bookmark - // on Android, it should be either taken into account here or another bridge - // interface should be provided. One approach would be something like: - // 1. Find a subsequence of elements in the beginning of the vector that is - // already sorted. - // 2. The same for the end. - // 3. If the two overlap, adjust so they don't. - // 4. Sort the middle, using Between (e.g. recursive implementation). - syncer::UniquePosition position; - for (const auto& child : node->children()) { - const SyncedBookmarkTrackerEntity* entity = - bookmark_tracker_->GetEntityForBookmarkNode(child.get()); - DCHECK(entity); - - const std::string& sync_id = entity->metadata()->server_id(); - const std::string suffix = syncer::GenerateSyncableBookmarkHash( - bookmark_tracker_->model_type_state().cache_guid(), sync_id); - const base::Time modification_time = base::Time::Now(); - - position = (child == node->children().front()) - ? syncer::UniquePosition::InitialPosition(suffix) - : syncer::UniquePosition::After(position, suffix); - - const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode( - child.get(), model, position.ToProto(), /*force_favicon_load=*/true); - - bookmark_tracker_->Update(entity, entity->metadata()->server_version(), - modification_time, specifics); - // Mark the entity that it needs to be committed. - bookmark_tracker_->IncrementSequenceNumber(entity); + if (node->children().size() <= 1) { + // There is no real change in the order of |node|'s children. + return; + } + + // The given node's children got reordered, all the corresponding sync nodes + // need to be reordered. The code is optimized to detect move of only one + // bookmark (which is the case on Android platform). There are 2 main cases: + // a bookmark moved to left or to right. Moving a bookmark to the first and + // last positions are two more special cases. The algorithm iterates over each + // bookmark and compares it to the left and right nodes to determine whether + // it's ordered or not. + // + // Each digit below represents bookmark's original position. + // + // Moving a bookmark to the left: 0123456 -> 0612345. + // When processing '6', its unique position is greater than both left and + // right nodes. + // + // Moving a bookmark to the right: 0123456 -> 0234516. + // When processing '1', its unique position is less than both left and right + // nodes. + // + // Note that in both cases left node is less than right node. This condition + // is checked when iterating over bookmarks and if it's violated, the + // algorithm falls back to generating positions for all the following nodes. + // + // For example, if two nodes are moved to one place: 0123456 -> 0156234 (nodes + // '5' and '6' are moved together). In this case, 0156 will remain and when + // processing '2', algorithm will fall back to generating unique positions for + // nodes '2', '3' and '4'. It will be detected by comparing the next node '3' + // with the previous '6'. + + // Store |cur| outside of the loop to prevent parsing UniquePosition twice. + UniquePositionWrapper cur = UniquePositionWrapper::ForValidUniquePosition( + GetUniquePositionForNode(node->children().front().get())); + UniquePositionWrapper prev = UniquePositionWrapper::Min(); + for (size_t current_index = 0; current_index < node->children().size(); + ++current_index) { + UniquePositionWrapper next = UniquePositionWrapper::Max(); + if (current_index + 1 < node->children().size()) { + next = UniquePositionWrapper::ForValidUniquePosition( + GetUniquePositionForNode(node->children()[current_index + 1].get())); + } + + // |prev| is the last ordered node. Compare |cur| and |next| with it to + // decide whether current node needs to be updated. Consider the following + // cases: 0. |prev| < |cur| < |next|: all elements are ordered. + // 1. |cur| < |prev| < |next|: update current node and put it between |prev| + // and |next|. + // 2. |cur| < |next| < |prev|: both |cur| and |next| are out of order, fall + // back to simple approach. + // 3. |next| < |cur| < |prev|: same as #2. + // 4. |prev| < |next| < |cur|: update current node and put it between |prev| + // and |next|. + // 5. |next| < |prev| < |cur|: consider current node ordered, |next| will be + // updated on the next step. + // + // In the following code only cases where current node needs to be updated + // are considered (#0 and #5 are omitted because there is nothing to do). + + bool update_current_position = false; + if (cur.LessThan(prev)) { + // |cur| < |prev|, cases #1, #2 and #3. + if (next.LessThan(prev)) { + // There are two consecutive nodes which both are out of order (#2, #3): + // |prev| > |cur| and |prev| > |next|. + // It means that more than one bookmark has been reordered, fall back to + // generating unique positions for all the remaining children. + // + // |current_index| is always not 0 because |prev| cannot be + // UniquePositionWrapper::Min() if |next| < |prev|. + DCHECK_GT(current_index, 0u); + UpdateAllUniquePositionsStartingAt(node, model, current_index); + break; + } + update_current_position = true; + } else if (next.LessThan(cur) && prev.LessThan(next)) { + // |prev| < |next| < |cur| (case #4). + update_current_position = true; + } + + if (update_current_position) { + cur = UniquePositionWrapper::ForValidUniquePosition( + UpdateUniquePositionForNode(node->children()[current_index].get(), + model, prev.GetUniquePosition(), + next.GetUniquePosition())); + } + + DCHECK(prev.LessThan(cur)); + prev = std::move(cur); + cur = std::move(next); } + nudge_for_commit_closure_.Run(); } @@ -344,7 +472,7 @@ syncer::UniquePosition BookmarkModelObserverImpl::ComputePosition( // No predecessor, insert before the successor. return syncer::UniquePosition::Before( syncer::UniquePosition::FromProto( - successor_entity->metadata()->unique_position()), + successor_entity->metadata().unique_position()), suffix); } @@ -352,16 +480,16 @@ syncer::UniquePosition BookmarkModelObserverImpl::ComputePosition( // No successor, insert after the predecessor return syncer::UniquePosition::After( syncer::UniquePosition::FromProto( - predecessor_entity->metadata()->unique_position()), + predecessor_entity->metadata().unique_position()), suffix); } // Both predecessor and successor, insert in the middle. return syncer::UniquePosition::Between( syncer::UniquePosition::FromProto( - predecessor_entity->metadata()->unique_position()), + predecessor_entity->metadata().unique_position()), syncer::UniquePosition::FromProto( - successor_entity->metadata()->unique_position()), + successor_entity->metadata().unique_position()), suffix); } @@ -374,18 +502,10 @@ void BookmarkModelObserverImpl::ProcessUpdate( // determined here by comparing hashes. if (entity->MatchesSpecificsHash(specifics)) { // Specifics haven't actually changed, so the local change can be ignored. - // - // This is an opportunity to populate the favicon hash in sync metadata if - // it hasn't been populated yet. This is needed because the proto field that - // stores favicon hashes was introduced late. The fact that hashed specifics - // match implies that the favicon (which is part of specifics) must also - // match, hence the proto field can be safely populated. - bookmark_tracker_->PopulateFaviconHashIfUnset( - entity, specifics.bookmark().favicon()); return; } - bookmark_tracker_->Update(entity, entity->metadata()->server_version(), + bookmark_tracker_->Update(entity, entity->metadata().server_version(), /*modification_time=*/base::Time::Now(), specifics); // Mark the entity that it needs to be committed. bookmark_tracker_->IncrementSequenceNumber(entity); @@ -405,7 +525,7 @@ void BookmarkModelObserverImpl::ProcessDelete( DCHECK(entity); // If the entity hasn't been committed and doesn't have an inflight commit // request, simply remove it from the tracker. - if (entity->metadata()->server_version() == syncer::kUncommittedVersion && + if (entity->metadata().server_version() == syncer::kUncommittedVersion && !entity->commit_may_have_started()) { bookmark_tracker_->Remove(entity); return; @@ -415,4 +535,70 @@ void BookmarkModelObserverImpl::ProcessDelete( bookmark_tracker_->IncrementSequenceNumber(entity); } +syncer::UniquePosition BookmarkModelObserverImpl::GetUniquePositionForNode( + const bookmarks::BookmarkNode* node) const { + DCHECK(bookmark_tracker_); + DCHECK(node); + const SyncedBookmarkTrackerEntity* entity = + bookmark_tracker_->GetEntityForBookmarkNode(node); + DCHECK(entity); + return syncer::UniquePosition::FromProto( + entity->metadata().unique_position()); +} + +syncer::UniquePosition BookmarkModelObserverImpl::UpdateUniquePositionForNode( + const bookmarks::BookmarkNode* node, + bookmarks::BookmarkModel* bookmark_model, + const syncer::UniquePosition& prev, + const syncer::UniquePosition& next) { + DCHECK(bookmark_tracker_); + DCHECK(node); + DCHECK(bookmark_model); + + const SyncedBookmarkTrackerEntity* entity = + bookmark_tracker_->GetEntityForBookmarkNode(node); + DCHECK(entity); + const std::string suffix = syncer::GenerateSyncableBookmarkHash( + bookmark_tracker_->model_type_state().cache_guid(), + entity->metadata().server_id()); + const base::Time modification_time = base::Time::Now(); + + syncer::UniquePosition new_unique_position; + if (prev.IsValid() && next.IsValid()) { + new_unique_position = syncer::UniquePosition::Between(prev, next, suffix); + } else if (prev.IsValid()) { + new_unique_position = syncer::UniquePosition::After(prev, suffix); + } else { + new_unique_position = syncer::UniquePosition::Before(next, suffix); + } + + sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode( + node, bookmark_model, new_unique_position.ToProto(), + /*force_favicon_load=*/true); + bookmark_tracker_->Update(entity, entity->metadata().server_version(), + modification_time, specifics); + + // Mark the entity that it needs to be committed. + bookmark_tracker_->IncrementSequenceNumber(entity); + return new_unique_position; +} + +void BookmarkModelObserverImpl::UpdateAllUniquePositionsStartingAt( + const bookmarks::BookmarkNode* parent, + bookmarks::BookmarkModel* bookmark_model, + size_t start_index) { + DCHECK_GT(start_index, 0u); + DCHECK_LT(start_index, parent->children().size()); + + syncer::UniquePosition prev = + GetUniquePositionForNode(parent->children()[start_index - 1].get()); + for (size_t current_index = start_index; + current_index < parent->children().size(); ++current_index) { + // Right position is unknown because it will also be updated. + prev = UpdateUniquePositionForNode(parent->children()[current_index].get(), + bookmark_model, prev, + /*next=*/syncer::UniquePosition()); + } +} + } // namespace sync_bookmarks diff --git a/chromium/components/sync_bookmarks/bookmark_model_observer_impl.h b/chromium/components/sync_bookmarks/bookmark_model_observer_impl.h index f1768b6f382..52d7bf0b61b 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_observer_impl.h +++ b/chromium/components/sync_bookmarks/bookmark_model_observer_impl.h @@ -93,6 +93,26 @@ class BookmarkModelObserverImpl : public bookmarks::BookmarkModelObserver { // over all children before processing the folder itself. void ProcessDelete(const bookmarks::BookmarkNode* node); + // Returns current unique_position from sync metadata for the tracked |node|. + syncer::UniquePosition GetUniquePositionForNode( + const bookmarks::BookmarkNode* node) const; + + // Updates the unique position in sync metadata for the tracked |node| and + // returns the new position. A new position is generated based on the left and + // right node's positions. At least one of |prev| and |next| must be valid. + syncer::UniquePosition UpdateUniquePositionForNode( + const bookmarks::BookmarkNode* node, + bookmarks::BookmarkModel* model, + const syncer::UniquePosition& prev, + const syncer::UniquePosition& next); + + // Updates unique positions for all children from |parent| starting from + // |start_index| (must not be 0). + void UpdateAllUniquePositionsStartingAt( + const bookmarks::BookmarkNode* parent, + bookmarks::BookmarkModel* bookmark_model, + size_t start_index); + // Points to the tracker owned by the processor. It keeps the mapping between // bookmark nodes and corresponding sync server entities. const raw_ptr<SyncedBookmarkTracker> bookmark_tracker_; diff --git a/chromium/components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc b/chromium/components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc index e97f9cc9ea7..190a5440bcb 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc @@ -43,6 +43,7 @@ using testing::IsNull; using testing::Ne; using testing::NiceMock; using testing::NotNull; +using testing::SizeIs; using testing::UnorderedElementsAre; const char kBookmarkBarId[] = "bookmark_bar_id"; @@ -103,12 +104,12 @@ class BookmarkModelObserverImplTest : public testing::Test { void SimulateCommitResponseForAllLocalChanges() { for (const SyncedBookmarkTrackerEntity* entity : bookmark_tracker()->GetEntitiesWithLocalChanges()) { - const std::string id = entity->metadata()->server_id(); + const std::string id = entity->metadata().server_id(); // Don't simulate change in id for simplicity. bookmark_tracker()->UpdateUponCommitResponse( entity, id, /*server_version=*/1, - /*acked_sequence_number=*/entity->metadata()->sequence_number()); + /*acked_sequence_number=*/entity->metadata().sequence_number()); } } @@ -117,7 +118,33 @@ class BookmarkModelObserverImplTest : public testing::Test { const SyncedBookmarkTrackerEntity* entity = bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node); return syncer::UniquePosition::FromProto( - entity->metadata()->unique_position()); + entity->metadata().unique_position()); + } + + std::vector<const bookmarks::BookmarkNode*> GenerateBookmarkNodes( + size_t num_bookmarks) { + const std::string kTitle = "title"; + const std::string kUrl = "http://www.url.com"; + + const bookmarks::BookmarkNode* bookmark_bar_node = + bookmark_model()->bookmark_bar_node(); + std::vector<const bookmarks::BookmarkNode*> nodes; + for (size_t i = 0; i < num_bookmarks; ++i) { + nodes.push_back(bookmark_model()->AddURL( + /*parent=*/bookmark_bar_node, /*index=*/i, base::UTF8ToUTF16(kTitle), + GURL(kUrl))); + } + + // Verify number of entities local changes. Should be the same as number of + // new nodes. + DCHECK_EQ(bookmark_tracker()->GetEntitiesWithLocalChanges().size(), + num_bookmarks); + + // All bookmarks should be tracked now (including permanent nodes). + DCHECK_EQ(bookmark_tracker()->TrackedEntitiesCountForTest(), + 3 + num_bookmarks); + + return nodes; } bookmarks::BookmarkModel* bookmark_model() { return bookmark_model_.get(); } @@ -157,7 +184,7 @@ TEST_F(BookmarkModelObserverImplTest, bookmark_tracker()->GetEntitiesWithLocalChanges(); ASSERT_THAT(local_changes.size(), 1U); EXPECT_THAT(local_changes[0]->bookmark_node(), Eq(bookmark_node)); - EXPECT_THAT(local_changes[0]->metadata()->server_id(), + EXPECT_THAT(local_changes[0]->metadata().server_id(), Eq(bookmark_node->guid().AsLowercaseString())); } @@ -249,50 +276,165 @@ TEST_F(BookmarkModelObserverImplTest, TEST_F(BookmarkModelObserverImplTest, ReorderChildrenShouldUpdateTheTrackerAndNudgeForCommit) { - const std::string kTitle = "title"; - const std::string kUrl = "http://www.url.com"; + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); - // Build this structure: + SimulateCommitResponseForAllLocalChanges(); + + // Reorder it to be (2 bookmarks have been moved): // bookmark_bar + // |- node1 + // |- node3 // |- node0 + // |- node2 + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[1], nodes[3], nodes[0], nodes[2]}); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[3]))); + EXPECT_TRUE(PositionOf(nodes[3]).LessThan(PositionOf(nodes[0]))); + EXPECT_TRUE(PositionOf(nodes[0]).LessThan(PositionOf(nodes[2]))); + + // Only 2 moved nodes should have local changes to commit. + EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), SizeIs(2)); +} + +TEST_F(BookmarkModelObserverImplTest, + ShouldReorderChildrenAndUpdateOnlyMovedToRightBookmark) { + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); + + SimulateCommitResponseForAllLocalChanges(); + + // Reorder it to be: + // bookmark_bar // |- node1 // |- node2 + // |- node0 (moved) // |- node3 - const bookmarks::BookmarkNode* bookmark_bar_node = - bookmark_model()->bookmark_bar_node(); - std::vector<const bookmarks::BookmarkNode*> nodes; - for (size_t i = 0; i < 4; ++i) { - nodes.push_back(bookmark_model()->AddURL( - /*parent=*/bookmark_bar_node, /*index=*/i, base::UTF8ToUTF16(kTitle), - GURL(kUrl))); - } + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[1], nodes[2], nodes[0], nodes[3]}); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[2]))); + EXPECT_TRUE(PositionOf(nodes[2]).LessThan(PositionOf(nodes[0]))); + EXPECT_TRUE(PositionOf(nodes[0]).LessThan(PositionOf(nodes[3]))); - // Verify number of entities local changes. Should be the same as number of - // new nodes. - ASSERT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges().size(), 4U); + // Only one moved node should have local changes to commit. + EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), + UnorderedElementsAre(HasBookmarkNode(nodes[0]))); +} - // All bookmarks should be tracked now. - ASSERT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 7U); +TEST_F(BookmarkModelObserverImplTest, + ShouldReorderChildrenAndUpdateOnlyMovedToLeftBookmark) { + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); + + SimulateCommitResponseForAllLocalChanges(); + + // Reorder it to be: + // bookmark_bar + // |- node0 + // |- node3 (moved) + // |- node1 + // |- node2 + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[0], nodes[3], nodes[1], nodes[2]}); + EXPECT_TRUE(PositionOf(nodes[0]).LessThan(PositionOf(nodes[3]))); + EXPECT_TRUE(PositionOf(nodes[3]).LessThan(PositionOf(nodes[1]))); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[2]))); + + // Only one moved node should have local changes to commit. + EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), + UnorderedElementsAre(HasBookmarkNode(nodes[3]))); +} + +TEST_F(BookmarkModelObserverImplTest, + ShouldReorderWhenBookmarkMovedToLastPosition) { + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); SimulateCommitResponseForAllLocalChanges(); // Reorder it to be: // bookmark_bar // |- node1 + // |- node2 // |- node3 + // |- node0 (moved) + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[1], nodes[2], nodes[3], nodes[0]}); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[2]))); + EXPECT_TRUE(PositionOf(nodes[2]).LessThan(PositionOf(nodes[3]))); + EXPECT_TRUE(PositionOf(nodes[3]).LessThan(PositionOf(nodes[0]))); + + // Only one moved node should have local changes to commit. + EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), + UnorderedElementsAre(HasBookmarkNode(nodes[0]))); +} + +TEST_F(BookmarkModelObserverImplTest, + ShouldReorderWhenBookmarkMovedToFirstPosition) { + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); + + SimulateCommitResponseForAllLocalChanges(); + + // Reorder it to be: + // bookmark_bar + // |- node3 (moved) // |- node0 + // |- node1 // |- node2 - bookmark_model()->ReorderChildren(bookmark_bar_node, - {nodes[1], nodes[3], nodes[0], nodes[2]}); - EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[3]))); + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[3], nodes[0], nodes[1], nodes[2]}); EXPECT_TRUE(PositionOf(nodes[3]).LessThan(PositionOf(nodes[0]))); - EXPECT_TRUE(PositionOf(nodes[0]).LessThan(PositionOf(nodes[2]))); + EXPECT_TRUE(PositionOf(nodes[0]).LessThan(PositionOf(nodes[1]))); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[2]))); - // All 4 nodes should have local changes to commit. + // Only one moved node should have local changes to commit. EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), - UnorderedElementsAre( - HasBookmarkNode(nodes[0]), HasBookmarkNode(nodes[1]), - HasBookmarkNode(nodes[2]), HasBookmarkNode(nodes[3]))); + UnorderedElementsAre(HasBookmarkNode(nodes[3]))); +} + +TEST_F(BookmarkModelObserverImplTest, ShouldReorderWhenAllBookmarksReversed) { + // In this case almost all the bookmarks should be updated apart from only one + // bookmark. + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); + + SimulateCommitResponseForAllLocalChanges(); + + // Reorder it to be (all nodes are moved): + // bookmark_bar + // |- node3 + // |- node2 + // |- node1 + // |- node0 + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[3], nodes[2], nodes[1], nodes[0]}); + EXPECT_TRUE(PositionOf(nodes[3]).LessThan(PositionOf(nodes[2]))); + EXPECT_TRUE(PositionOf(nodes[2]).LessThan(PositionOf(nodes[1]))); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[0]))); + + // Do not verify which nodes exactly have been updated, it depends on the + // implementation and any of the nodes may become a base node to calculate + // relative positions of all other nodes. + EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), SizeIs(3)); +} + +TEST_F(BookmarkModelObserverImplTest, + ShouldNotReorderIfAllBookmarksStillOrdered) { + std::vector<const bookmarks::BookmarkNode*> nodes = + GenerateBookmarkNodes(/*num_bookmarks=*/4); + + SimulateCommitResponseForAllLocalChanges(); + + // Keep the original order. + bookmark_model()->ReorderChildren(bookmark_model()->bookmark_bar_node(), + {nodes[0], nodes[1], nodes[2], nodes[3]}); + EXPECT_TRUE(PositionOf(nodes[0]).LessThan(PositionOf(nodes[1]))); + EXPECT_TRUE(PositionOf(nodes[1]).LessThan(PositionOf(nodes[2]))); + EXPECT_TRUE(PositionOf(nodes[2]).LessThan(PositionOf(nodes[3]))); + + // The bookmarks remain in the same order, nothing to commit. + EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), IsEmpty()); } TEST_F(BookmarkModelObserverImplTest, @@ -336,16 +478,15 @@ TEST_F(BookmarkModelObserverImplTest, const SyncedBookmarkTrackerEntity* bookmark3_entity = bookmark_tracker()->GetEntityForBookmarkNode(bookmark3_node); - ASSERT_FALSE(folder2_entity->metadata()->is_deleted()); - ASSERT_FALSE(bookmark2_entity->metadata()->is_deleted()); - ASSERT_FALSE(bookmark3_entity->metadata()->is_deleted()); + ASSERT_FALSE(folder2_entity->metadata().is_deleted()); + ASSERT_FALSE(bookmark2_entity->metadata().is_deleted()); + ASSERT_FALSE(bookmark3_entity->metadata().is_deleted()); - const std::string& folder2_entity_id = - folder2_entity->metadata()->server_id(); + const std::string& folder2_entity_id = folder2_entity->metadata().server_id(); const std::string& bookmark2_entity_id = - bookmark2_entity->metadata()->server_id(); + bookmark2_entity->metadata().server_id(); const std::string& bookmark3_entity_id = - bookmark3_entity->metadata()->server_id(); + bookmark3_entity->metadata().server_id(); // Delete folder2. EXPECT_CALL(*nudge_for_commit_closure(), Run()); bookmark_model()->Remove(folder2_node); @@ -354,15 +495,15 @@ TEST_F(BookmarkModelObserverImplTest, EXPECT_TRUE(bookmark_tracker() ->GetEntityForSyncId(folder2_entity_id) ->metadata() - ->is_deleted()); + .is_deleted()); EXPECT_TRUE(bookmark_tracker() ->GetEntityForSyncId(bookmark2_entity_id) ->metadata() - ->is_deleted()); + .is_deleted()); EXPECT_TRUE(bookmark_tracker() ->GetEntityForSyncId(bookmark3_entity_id) ->metadata() - ->is_deleted()); + .is_deleted()); // folder2, bookmark2, and bookmark3 should be in the local changes to be // committed and folder2 deletion should be the last one (after all children @@ -389,7 +530,7 @@ TEST_F(BookmarkModelObserverImplTest, ASSERT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 4U); const SyncedBookmarkTrackerEntity* entity = bookmark_tracker()->GetEntityForBookmarkNode(folder_node); - const std::string id = entity->metadata()->server_id(); + const std::string id = entity->metadata().server_id(); ASSERT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges().size(), 1U); bookmark_tracker()->MarkCommitMayHaveStarted(entity); @@ -429,7 +570,7 @@ TEST_F(BookmarkModelObserverImplTest, const std::string id = bookmark_tracker() ->GetEntityForBookmarkNode(folder_node) ->metadata() - ->server_id(); + .server_id(); ASSERT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges().size(), 1U); // Remove the folder. @@ -634,9 +775,9 @@ TEST_F(BookmarkModelObserverImplTest, ShouldNotIssueCommitUponFaviconLoad) { const SyncedBookmarkTrackerEntity* entity = bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node); ASSERT_THAT(entity, NotNull()); - ASSERT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); + ASSERT_TRUE(entity->metadata().has_bookmark_favicon_hash()); const uint32_t initial_favicon_hash = - entity->metadata()->bookmark_favicon_hash(); + entity->metadata().bookmark_favicon_hash(); // Clear the specifics hash (as if the proto definition would have changed). // This is needed because otherwise the commit is trivially optimized away @@ -653,8 +794,8 @@ TEST_F(BookmarkModelObserverImplTest, ShouldNotIssueCommitUponFaviconLoad) { ASSERT_TRUE(bookmark_client()->SimulateFaviconLoaded( kBookmarkUrl, kIconUrl, CreateTestImage(kColor))); - EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); - EXPECT_THAT(entity->metadata()->bookmark_favicon_hash(), + EXPECT_TRUE(entity->metadata().has_bookmark_favicon_hash()); + EXPECT_THAT(entity->metadata().bookmark_favicon_hash(), Eq(initial_favicon_hash)); EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), IsEmpty()); } @@ -678,9 +819,9 @@ TEST_F(BookmarkModelObserverImplTest, ShouldCommitLocalFaviconChange) { const SyncedBookmarkTrackerEntity* entity = bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node); ASSERT_THAT(entity, NotNull()); - ASSERT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); + ASSERT_TRUE(entity->metadata().has_bookmark_favicon_hash()); const uint32_t initial_favicon_hash = - entity->metadata()->bookmark_favicon_hash(); + entity->metadata().bookmark_favicon_hash(); // A favicon change should trigger a commit nudge once the favicon loads, but // not earlier. Note that OnFaviconsChanged() needs no icon URL to invalidate @@ -694,8 +835,8 @@ TEST_F(BookmarkModelObserverImplTest, ShouldCommitLocalFaviconChange) { ASSERT_TRUE(bookmark_client()->SimulateFaviconLoaded( kBookmarkUrl, kFinalIconUrl, CreateTestImage(SK_ColorBLUE))); - EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); - EXPECT_THAT(entity->metadata()->bookmark_favicon_hash(), + EXPECT_TRUE(entity->metadata().has_bookmark_favicon_hash()); + EXPECT_THAT(entity->metadata().bookmark_favicon_hash(), Ne(initial_favicon_hash)); EXPECT_THAT(bookmark_tracker()->GetEntitiesWithLocalChanges(), ElementsAre(HasBookmarkNode(bookmark_node))); @@ -780,7 +921,7 @@ TEST_F(BookmarkModelObserverImplTest, const std::vector<const SyncedBookmarkTrackerEntity*> local_changes = bookmark_tracker()->GetEntitiesWithLocalChanges(); ASSERT_THAT(local_changes, ElementsAre(folder_entity)); - ASSERT_TRUE(folder_entity->metadata()->is_deleted()); + ASSERT_TRUE(folder_entity->metadata().is_deleted()); ASSERT_EQ( bookmark_tracker()->GetEntityForClientTagHash(folder_client_tag_hash), folder_entity); @@ -795,7 +936,7 @@ TEST_F(BookmarkModelObserverImplTest, bookmark_tracker()->GetEntityForClientTagHash(folder_client_tag_hash), folder_entity); EXPECT_TRUE(folder_entity->IsUnsynced()); - EXPECT_FALSE(folder_entity->metadata()->is_deleted()); + EXPECT_FALSE(folder_entity->metadata().is_deleted()); EXPECT_EQ(folder_entity->bookmark_node(), folder); } diff --git a/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc b/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc index a24dfd2d0cc..925ff98fbe3 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_type_processor.cc @@ -472,7 +472,7 @@ void BookmarkModelTypeProcessor::StopTrackingMetadata() { void BookmarkModelTypeProcessor::GetAllNodesForDebugging( AllNodesCallback callback) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - auto all_nodes = std::make_unique<base::ListValue>(); + base::Value::List all_nodes; // Create a permanent folder since sync server no longer create root folders, // and USS won't migrate root folders from directory, we create root folders. base::Value::Dict root_node; @@ -486,12 +486,12 @@ void BookmarkModelTypeProcessor::GetAllNodesForDebugging( root_node.Set("IS_DIR", true); root_node.Set("modelType", "Bookmarks"); root_node.Set("NON_UNIQUE_NAME", "Bookmarks"); - all_nodes->Append(base::Value(std::move(root_node))); + all_nodes.Append(std::move(root_node)); const bookmarks::BookmarkNode* model_root_node = bookmark_model_->root_node(); int i = 0; for (const auto& child : model_root_node->children()) { - AppendNodeAndChildrenForDebugging(child.get(), i++, all_nodes.get()); + AppendNodeAndChildrenForDebugging(child.get(), i++, &all_nodes); } std::move(callback).Run(syncer::BOOKMARKS, std::move(all_nodes)); @@ -500,7 +500,7 @@ void BookmarkModelTypeProcessor::GetAllNodesForDebugging( void BookmarkModelTypeProcessor::AppendNodeAndChildrenForDebugging( const bookmarks::BookmarkNode* node, int index, - base::ListValue* all_nodes) const { + base::Value::List* all_nodes) const { const SyncedBookmarkTrackerEntity* entity = bookmark_tracker_->GetEntityForBookmarkNode(node); // Include only tracked nodes. Newly added nodes are tracked even before being @@ -509,17 +509,17 @@ void BookmarkModelTypeProcessor::AppendNodeAndChildrenForDebugging( if (!entity) { return; } - const sync_pb::EntityMetadata* metadata = entity->metadata(); + const sync_pb::EntityMetadata& metadata = entity->metadata(); // Copy data to an EntityData object to reuse its conversion // ToDictionaryValue() methods. syncer::EntityData data; - data.id = metadata->server_id(); + data.id = metadata.server_id(); data.creation_time = node->date_added(); data.modification_time = - syncer::ProtoTimeToTime(metadata->modification_time()); + syncer::ProtoTimeToTime(metadata.modification_time()); data.name = base::UTF16ToUTF8(node->GetTitle()); data.specifics = CreateSpecificsFromBookmarkNode( - node, bookmark_model_, metadata->unique_position(), + node, bookmark_model_, metadata.unique_position(), /*force_favicon_load=*/false); if (node->is_permanent_node()) { @@ -534,30 +534,26 @@ void BookmarkModelTypeProcessor::AppendNodeAndChildrenForDebugging( const SyncedBookmarkTrackerEntity* parent_entity = bookmark_tracker_->GetEntityForBookmarkNode(parent); DCHECK(parent_entity); - data.legacy_parent_id = parent_entity->metadata()->server_id(); + data.legacy_parent_id = parent_entity->metadata().server_id(); } - std::unique_ptr<base::DictionaryValue> data_dictionary = - data.ToDictionaryValue(); + base::Value::Dict data_dictionary = data.ToDictionaryValue(); // Set ID value as in legacy directory-based implementation, "s" means server. - data_dictionary->SetString("ID", "s" + metadata->server_id()); + data_dictionary.Set("ID", "s" + metadata.server_id()); if (node->is_permanent_node()) { // Hardcode the parent of permanent nodes. - data_dictionary->SetString("PARENT_ID", "BOOKMARKS_ROOT"); - data_dictionary->SetString("UNIQUE_SERVER_TAG", - data.server_defined_unique_tag); + data_dictionary.Set("PARENT_ID", "BOOKMARKS_ROOT"); + data_dictionary.Set("UNIQUE_SERVER_TAG", data.server_defined_unique_tag); } else { - data_dictionary->SetString("PARENT_ID", "s" + data.legacy_parent_id); + data_dictionary.Set("PARENT_ID", "s" + data.legacy_parent_id); } - data_dictionary->SetInteger("LOCAL_EXTERNAL_ID", node->id()); - data_dictionary->SetInteger("positionIndex", index); - data_dictionary->SetKey("metadata", - base::Value::FromUniquePtrValue( - syncer::EntityMetadataToValue(*metadata))); - data_dictionary->SetString("modelType", "Bookmarks"); - data_dictionary->SetBoolean("IS_DIR", node->is_folder()); - all_nodes->Append( - base::Value::FromUniquePtrValue(std::move(data_dictionary))); + data_dictionary.Set("LOCAL_EXTERNAL_ID", static_cast<int>(node->id())); + data_dictionary.Set("positionIndex", index); + data_dictionary.Set("metadata", base::Value::FromUniquePtrValue( + syncer::EntityMetadataToValue(metadata))); + data_dictionary.Set("modelType", "Bookmarks"); + data_dictionary.Set("IS_DIR", node->is_folder()); + all_nodes->Append(std::move(data_dictionary)); int i = 0; for (const auto& child : node->children()) { diff --git a/chromium/components/sync_bookmarks/bookmark_model_type_processor.h b/chromium/components/sync_bookmarks/bookmark_model_type_processor.h index c3ab029157a..0bb1081d1a0 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_type_processor.h +++ b/chromium/components/sync_bookmarks/bookmark_model_type_processor.h @@ -125,7 +125,7 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor, // since we iterate over child nodes already in the calling sites. void AppendNodeAndChildrenForDebugging(const bookmarks::BookmarkNode* node, int index, - base::ListValue* all_nodes) const; + base::Value::List* all_nodes) const; // Stores the start callback in between OnSyncStarting() and // ModelReadyToSync(). diff --git a/chromium/components/sync_bookmarks/bookmark_model_type_processor_unittest.cc b/chromium/components/sync_bookmarks/bookmark_model_type_processor_unittest.cc index 83920e00348..5f61bb13dcb 100644 --- a/chromium/components/sync_bookmarks/bookmark_model_type_processor_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_model_type_processor_unittest.cc @@ -142,6 +142,10 @@ sync_pb::BookmarkMetadata CreateNodeMetadata( .value()); *bookmark_metadata.mutable_metadata()->mutable_unique_position() = unique_position.ToProto(); + // Required by SyncedBookmarkTracker during validation of local metadata. + if (!node->is_folder()) { + bookmark_metadata.mutable_metadata()->set_bookmark_favicon_hash(123); + } return bookmark_metadata; } @@ -414,7 +418,7 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteUpdate) { const std::string kNewUrl = "http://www.new-url.com"; syncer::UpdateResponseDataList updates; updates.push_back(CreateUpdateResponseData( - {entity->metadata()->server_id(), kNewTitle, kNewUrl, kBookmarkBarId, + {entity->metadata().server_id(), kNewTitle, kNewUrl, kBookmarkBarId, /*server_tag=*/std::string()}, kRandomPosition, /*response_version=*/1, bookmark_node->guid())); @@ -456,7 +460,7 @@ TEST_F( // Process an update for the same bookmark with the same data. syncer::UpdateResponseDataList updates; updates.push_back(CreateUpdateResponseData( - {entity->metadata()->server_id(), kTitle, kUrl.spec(), kBookmarkBarId, + {entity->metadata().server_id(), kTitle, kUrl.spec(), kBookmarkBarId, /*server_tag=*/std::string()}, kRandomPosition, /*response_version=*/1, bookmark_node->guid())); updates[0].response_version++; @@ -812,7 +816,7 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldReuploadLegacyBookmarksOnStart) { ->GetTrackerForTest() ->GetEntityForBookmarkNode(node) ->metadata() - ->server_id(); + .server_id(); sync_pb::BookmarkModelMetadata model_metadata = processor()->GetTrackerForTest()->BuildBookmarkModelMetadata(); diff --git a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc index ab13ec97357..0754350a3de 100644 --- a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc +++ b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler.cc @@ -109,7 +109,7 @@ syncer::UniquePosition ComputeUniquePositionForTrackedBookmarkNode( // TODO(crbug.com/1113139): precompute UniquePosition to prevent its // calculation on each remote update. return syncer::UniquePosition::FromProto( - child_entity->metadata()->unique_position()); + child_entity->metadata().unique_position()); } size_t ComputeChildNodeIndex(const bookmarks::BookmarkNode* parent, @@ -216,7 +216,7 @@ void ApplyRemoteUpdate( UpdateBookmarkNodeFromSpecifics(update_entity.specifics.bookmark(), node, model, favicon_service); // Compute index information before updating the |tracker|. - const size_t old_index = static_cast<size_t>(old_parent->GetIndexOf(node)); + const size_t old_index = old_parent->GetIndexOf(node).value(); const size_t new_index = ComputeChildNodeIndex( new_parent, update_entity.specifics.bookmark().unique_position(), tracker); @@ -289,9 +289,9 @@ void BookmarkRemoteUpdatesHandler::Process( } // Ignore updates that have already been seen according to the version. - if (tracked_entity && tracked_entity->metadata()->server_version() >= + if (tracked_entity && tracked_entity->metadata().server_version() >= update->response_version) { - if (update_entity.id == tracked_entity->metadata()->server_id()) { + if (update_entity.id == tracked_entity->metadata().server_id()) { // Seen this update before. This update may be a reflection and may have // missing the GUID in specifics. Next reupload will populate GUID in // specifics and this codepath will not repeat indefinitely. This logic @@ -373,7 +373,7 @@ void BookmarkRemoteUpdatesHandler::Process( bookmark_tracker_->GetAllEntities(); for (const SyncedBookmarkTrackerEntity* entity : all_entities) { // No need to recommit tombstones and permanent nodes. - if (entity->metadata()->is_deleted()) { + if (entity->metadata().is_deleted()) { continue; } DCHECK(entity->bookmark_node()); @@ -381,7 +381,7 @@ void BookmarkRemoteUpdatesHandler::Process( continue; } if (entities_with_up_to_date_encryption.count( - entity->metadata()->server_id()) != 0) { + entity->metadata().server_id()) != 0) { continue; } bookmark_tracker_->IncrementSequenceNumber(entity); @@ -690,7 +690,7 @@ BookmarkRemoteUpdatesHandler::ProcessConflict( !tracked_entity->bookmark_node()->is_permanent_node()); DCHECK(!IsPermanentNodeUpdate(update_entity)); - if (tracked_entity->metadata()->is_deleted() && update_entity.is_deleted()) { + if (tracked_entity->metadata().is_deleted() && update_entity.is_deleted()) { // Both have been deleted, delete the corresponding entity from the tracker. bookmark_tracker_->Remove(tracked_entity); DLOG(WARNING) << "Conflict: CHANGES_MATCH"; @@ -708,7 +708,7 @@ BookmarkRemoteUpdatesHandler::ProcessConflict( DCHECK(IsValidBookmarkSpecifics(update_entity.specifics.bookmark())); - if (tracked_entity->metadata()->is_deleted()) { + if (tracked_entity->metadata().is_deleted()) { // Only local node has been deleted. It should be restored from the server // data as a remote creation. bookmark_tracker_->Remove(tracked_entity); @@ -803,7 +803,7 @@ void BookmarkRemoteUpdatesHandler::ReuploadEntityIfNeeded( const syncer::EntityData& entity_data, const SyncedBookmarkTrackerEntity* tracked_entity) { DCHECK(tracked_entity); - DCHECK_EQ(tracked_entity->metadata()->server_id(), entity_data.id); + DCHECK_EQ(tracked_entity->metadata().server_id(), entity_data.id); DCHECK(!tracked_entity->bookmark_node() || !tracked_entity->bookmark_node()->is_permanent_node()); diff --git a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc index 2605f2efd09..368455360e9 100644 --- a/chromium/components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc @@ -1166,7 +1166,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, // The sync id in the tracker should have been updated. EXPECT_THAT(tracker()->GetEntityForSyncId(kOriginatorClientItemId), IsNull()); EXPECT_THAT(tracker()->GetEntityForSyncId(kSyncId), Eq(entity)); - EXPECT_THAT(entity->metadata()->server_id(), Eq(kSyncId)); + EXPECT_THAT(entity->metadata().server_id(), Eq(kSyncId)); EXPECT_THAT(entity->bookmark_node(), Eq(node)); } @@ -1236,7 +1236,7 @@ TEST_F( EXPECT_THAT(tracker()->GetEntityForSyncId(kBookmarkGuid.AsLowercaseString()), IsNull()); EXPECT_THAT(tracker()->GetEntityForSyncId(kSyncId), Eq(entity)); - EXPECT_THAT(entity->metadata()->server_id(), Eq(kSyncId)); + EXPECT_THAT(entity->metadata().server_id(), Eq(kSyncId)); EXPECT_THAT(entity->bookmark_node(), Eq(node)); } @@ -1568,7 +1568,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest, entity = tracker()->GetEntityForGUID(kGuid); ASSERT_THAT(entity, NotNull()); EXPECT_THAT(entity->IsUnsynced(), Eq(false)); - EXPECT_THAT(entity->metadata()->is_deleted(), Eq(false)); + EXPECT_THAT(entity->metadata().is_deleted(), Eq(false)); // The bookmark should have been resurrected. EXPECT_THAT(bookmark_bar_node->children().size(), Eq(1u)); diff --git a/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc b/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc index 6a815bb5e14..5428ee31d8a 100644 --- a/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc +++ b/chromium/components/sync_bookmarks/bookmark_specifics_conversions.cc @@ -298,6 +298,10 @@ sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode( bm_specifics->set_creation_time_us( node->date_added().ToDeltaSinceWindowsEpoch().InMicroseconds()); *bm_specifics->mutable_unique_position() = unique_position; + if (!node->is_folder() && node->date_last_used() != base::Time()) { + bm_specifics->set_last_used_time_us( + node->date_last_used().ToDeltaSinceWindowsEpoch().InMicroseconds()); + } if (node->GetMetaInfoMap()) { UpdateBookmarkSpecificsMetaInfo(node->GetMetaInfoMap(), bm_specifics); @@ -372,6 +376,15 @@ const bookmarks::BookmarkNode* CreateBookmarkNodeFromSpecifics( const bookmarks::BookmarkNode* node = model->AddURL(parent, index, NodeTitleFromSpecifics(specifics), GURL(specifics.url()), &metainfo, creation_time, guid); + if (specifics.has_last_used_time_us()) { + const int64_t last_used_time_us = specifics.last_used_time_us(); + const base::Time last_used_time = + base::Time::FromDeltaSinceWindowsEpoch( + // Use FromDeltaSinceWindowsEpoch because last_used_time_us has + // always used the Windows epoch. + base::Microseconds(last_used_time_us)); + model->UpdateLastUsedTime(node, last_used_time); + } SetBookmarkFaviconFromSpecifics(specifics, node, favicon_service); return node; } @@ -404,6 +417,15 @@ void UpdateBookmarkNodeFromSpecifics( if (!node->is_folder()) { model->SetURL(node, GURL(specifics.url())); SetBookmarkFaviconFromSpecifics(specifics, node, favicon_service); + + if (specifics.has_last_used_time_us()) { + const int64_t last_used_time_us = specifics.last_used_time_us(); + const base::Time last_used_time = base::Time::FromDeltaSinceWindowsEpoch( + // Use FromDeltaSinceWindowsEpoch because last_used_time_us has + // always used the Windows epoch. + base::Microseconds(last_used_time_us)); + model->UpdateLastUsedTime(node, last_used_time); + } } } @@ -438,13 +460,14 @@ const bookmarks::BookmarkNode* ReplaceBookmarkNodeGUID( const bookmarks::BookmarkNode* new_node = nullptr; if (node->is_folder()) { new_node = model->AddFolder( - node->parent(), node->parent()->GetIndexOf(node), node->GetTitle(), - node->GetMetaInfoMap(), node->date_added(), guid); + node->parent(), node->parent()->GetIndexOf(node).value(), + node->GetTitle(), node->GetMetaInfoMap(), node->date_added(), guid); MoveAllChildren(model, node, new_node); } else { - new_node = model->AddURL(node->parent(), node->parent()->GetIndexOf(node), - node->GetTitle(), node->url(), - node->GetMetaInfoMap(), node->date_added(), guid); + new_node = + model->AddURL(node->parent(), node->parent()->GetIndexOf(node).value(), + node->GetTitle(), node->url(), node->GetMetaInfoMap(), + node->date_added(), guid); } model->Remove(node); diff --git a/chromium/components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc b/chromium/components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc index d816ab388ca..6b52a433380 100644 --- a/chromium/components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc +++ b/chromium/components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc @@ -94,6 +94,7 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) { kUrl); ASSERT_THAT(node, NotNull()); model->SetDateAdded(node, kTime); + model->UpdateLastUsedTime(node, kTime); model->SetNodeMetaInfo(node, kKey1, kValue1); model->SetNodeMetaInfo(node, kKey2, kValue2); @@ -110,6 +111,9 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) { EXPECT_THAT(base::Time::FromDeltaSinceWindowsEpoch( base::Microseconds(bm_specifics.creation_time_us())), Eq(kTime)); + EXPECT_THAT(base::Time::FromDeltaSinceWindowsEpoch( + base::Microseconds(bm_specifics.last_used_time_us())), + Eq(kTime)); EXPECT_TRUE(syncer::UniquePosition::FromProto(bm_specifics.unique_position()) .Equals(kUniquePosition)); for (const sync_pb::MetaInfo& meta_info : bm_specifics.meta_info()) { @@ -120,6 +124,32 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) { } TEST(BookmarkSpecificsConversionsTest, + ShouldCreateSpecificsFromBookmarkNodeNoDateLastUsed) { + const GURL kUrl("http://www.url.com"); + const std::string kTitle = "Title"; + const syncer::UniquePosition kUniquePosition = + syncer::UniquePosition::InitialPosition( + syncer::UniquePosition::RandomSuffix()); + + std::unique_ptr<bookmarks::BookmarkModel> model = + bookmarks::TestBookmarkClient::CreateModel(); + + const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node(); + const bookmarks::BookmarkNode* node = model->AddURL( + /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle), + kUrl); + + sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode( + node, model.get(), kUniquePosition.ToProto(), + /*force_favicon_load=*/false); + const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark(); + EXPECT_THAT(bm_specifics.guid(), Eq(node->guid().AsLowercaseString())); + EXPECT_THAT(bm_specifics.parent_guid(), + Eq(bookmarks::BookmarkNode::kBookmarkBarNodeGuid)); + EXPECT_FALSE(bm_specifics.has_last_used_time_us()); +} + +TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNodeWithIllegalTitle) { std::unique_ptr<bookmarks::BookmarkModel> model = bookmarks::TestBookmarkClient::CreateModel(); @@ -156,6 +186,7 @@ TEST(BookmarkSpecificsConversionsTest, const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark(); EXPECT_FALSE(bm_specifics.has_url()); EXPECT_THAT(bm_specifics.type(), Eq(sync_pb::BookmarkSpecifics::FOLDER)); + EXPECT_FALSE(bm_specifics.has_last_used_time_us()); } TEST(BookmarkSpecificsConversionsTest, @@ -307,6 +338,8 @@ TEST(BookmarkSpecificsConversionsTest, bm_specifics.set_legacy_canonicalized_title(kTitle); bm_specifics.set_creation_time_us( kTime.ToDeltaSinceWindowsEpoch().InMicroseconds()); + bm_specifics.set_last_used_time_us( + kTime.ToDeltaSinceWindowsEpoch().InMicroseconds()); bm_specifics.set_type(sync_pb::BookmarkSpecifics::URL); // Parent GUID and unique position are ignored by @@ -339,6 +372,7 @@ TEST(BookmarkSpecificsConversionsTest, EXPECT_FALSE(node->is_folder()); EXPECT_THAT(node->url(), Eq(kUrl)); EXPECT_THAT(node->date_added(), Eq(kTime)); + EXPECT_THAT(node->date_last_used(), Eq(kTime)); std::string value1; node->GetMetaInfo(kKey1, &value1); EXPECT_THAT(value1, Eq(kValue1)); @@ -569,6 +603,8 @@ TEST(BookmarkSpecificsConversionsTest, ShouldUpdateBookmarkNodeFromSpecifics) { bm_specifics.set_legacy_canonicalized_title(kNewTitle); bm_specifics.set_creation_time_us( kTime.ToDeltaSinceWindowsEpoch().InMicroseconds()); + bm_specifics.set_last_used_time_us( + kTime.ToDeltaSinceWindowsEpoch().InMicroseconds()); sync_pb::MetaInfo* meta_info1 = bm_specifics.add_meta_info(); meta_info1->set_key(kKey1); meta_info1->set_value(kNewValue1); @@ -932,7 +968,7 @@ TEST(BookmarkSpecificsConversionsTest, ReplaceUrlNodeWithUpdatedGUID) { EXPECT_EQ(kGuid, new_url->guid()); EXPECT_EQ(kTitle, new_url->GetTitle()); EXPECT_EQ(bookmark_bar_node, new_url->parent()); - EXPECT_EQ(0, bookmark_bar_node->GetIndexOf(new_url)); + EXPECT_EQ(0u, bookmark_bar_node->GetIndexOf(new_url)); EXPECT_EQ(kUrl, new_url->url()); EXPECT_EQ(kCreationTime, new_url->date_added()); std::string out_value_url; @@ -968,13 +1004,13 @@ TEST(BookmarkSpecificsConversionsTest, ReplaceFolderNodeWithUpdatedGUID) { EXPECT_EQ(kGuid, new_folder->guid()); EXPECT_EQ(kTitle, new_folder->GetTitle()); EXPECT_EQ(bookmark_bar_node, new_folder->parent()); - EXPECT_EQ(0, bookmark_bar_node->GetIndexOf(new_folder)); + EXPECT_EQ(0u, bookmark_bar_node->GetIndexOf(new_folder)); std::string out_value_folder; EXPECT_TRUE(new_folder->GetMetaInfo(kKey, &out_value_folder)); EXPECT_EQ(kValue, out_value_folder); EXPECT_EQ(2u, new_folder->children().size()); - EXPECT_EQ(0, new_folder->GetIndexOf(url1)); - EXPECT_EQ(1, new_folder->GetIndexOf(url2)); + EXPECT_EQ(0u, new_folder->GetIndexOf(url1)); + EXPECT_EQ(1u, new_folder->GetIndexOf(url2)); } TEST(BookmarkSpecificsConversionsTest, diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc b/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc index a129b0799df..4881c8fb1d3 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker.cc @@ -158,7 +158,7 @@ const SyncedBookmarkTrackerEntity* SyncedBookmarkTracker::GetEntityForGUID( SyncedBookmarkTrackerEntity* SyncedBookmarkTracker::AsMutableEntity( const SyncedBookmarkTrackerEntity* entity) { DCHECK(entity); - DCHECK_EQ(entity, GetEntityForSyncId(entity->metadata()->server_id())); + DCHECK_EQ(entity, GetEntityForSyncId(entity->metadata().server_id())); // As per DCHECK above, this tracker owns |*entity|, so it's legitimate to // return non-const pointer. @@ -188,21 +188,22 @@ const SyncedBookmarkTrackerEntity* SyncedBookmarkTracker::Add( syncer::ClientTagHash client_tag_hash = GetClientTagHashFromGUID(bookmark_node->guid()); - auto metadata = std::make_unique<sync_pb::EntityMetadata>(); - metadata->set_is_deleted(false); - metadata->set_server_id(sync_id); - metadata->set_server_version(server_version); - metadata->set_creation_time(syncer::TimeToProtoTime(creation_time)); - metadata->set_modification_time(syncer::TimeToProtoTime(creation_time)); - metadata->set_sequence_number(0); - metadata->set_acked_sequence_number(0); - *metadata->mutable_unique_position() = specifics.bookmark().unique_position(); - metadata->set_client_tag_hash(client_tag_hash.value()); - HashSpecifics(specifics, metadata->mutable_specifics_hash()); - metadata->set_bookmark_favicon_hash( + sync_pb::EntityMetadata metadata; + metadata.set_is_deleted(false); + metadata.set_server_id(sync_id); + metadata.set_server_version(server_version); + metadata.set_creation_time(syncer::TimeToProtoTime(creation_time)); + metadata.set_modification_time(syncer::TimeToProtoTime(creation_time)); + metadata.set_sequence_number(0); + metadata.set_acked_sequence_number(0); + *metadata.mutable_unique_position() = specifics.bookmark().unique_position(); + metadata.set_client_tag_hash(client_tag_hash.value()); + HashSpecifics(specifics, metadata.mutable_specifics_hash()); + metadata.set_bookmark_favicon_hash( base::PersistentHash(specifics.bookmark().favicon())); auto entity = std::make_unique<SyncedBookmarkTrackerEntity>( bookmark_node, std::move(metadata)); + DCHECK_EQ(0U, bookmark_node_to_entities_map_.count(bookmark_node)); bookmark_node_to_entities_map_[bookmark_node] = entity.get(); DCHECK_EQ(0U, client_tag_hash_to_entities_map_.count(client_tag_hash)); @@ -226,14 +227,14 @@ void SyncedBookmarkTracker::Update(const SyncedBookmarkTrackerEntity* entity, DCHECK(specifics.bookmark().has_unique_position()); SyncedBookmarkTrackerEntity* mutable_entity = AsMutableEntity(entity); - mutable_entity->metadata()->set_server_version(server_version); - mutable_entity->metadata()->set_modification_time( + mutable_entity->MutableMetadata()->set_server_version(server_version); + mutable_entity->MutableMetadata()->set_modification_time( syncer::TimeToProtoTime(modification_time)); - *mutable_entity->metadata()->mutable_unique_position() = + *mutable_entity->MutableMetadata()->mutable_unique_position() = specifics.bookmark().unique_position(); HashSpecifics(specifics, - mutable_entity->metadata()->mutable_specifics_hash()); - mutable_entity->metadata()->set_bookmark_favicon_hash( + mutable_entity->MutableMetadata()->mutable_specifics_hash()); + mutable_entity->MutableMetadata()->set_bookmark_favicon_hash( base::PersistentHash(specifics.bookmark().favicon())); } @@ -241,14 +242,8 @@ void SyncedBookmarkTracker::UpdateServerVersion( const SyncedBookmarkTrackerEntity* entity, int64_t server_version) { DCHECK(entity); - AsMutableEntity(entity)->metadata()->set_server_version(server_version); -} - -void SyncedBookmarkTracker::PopulateFaviconHashIfUnset( - const SyncedBookmarkTrackerEntity* entity, - const std::string& favicon_png_bytes) { - DCHECK(entity); - AsMutableEntity(entity)->PopulateFaviconHashIfUnset(favicon_png_bytes); + AsMutableEntity(entity)->MutableMetadata()->set_server_version( + server_version); } void SyncedBookmarkTracker::MarkCommitMayHaveStarted( @@ -260,13 +255,13 @@ void SyncedBookmarkTracker::MarkCommitMayHaveStarted( void SyncedBookmarkTracker::MarkDeleted( const SyncedBookmarkTrackerEntity* entity) { DCHECK(entity); - DCHECK(!entity->metadata()->is_deleted()); + DCHECK(!entity->metadata().is_deleted()); DCHECK(entity->bookmark_node()); DCHECK_EQ(1U, bookmark_node_to_entities_map_.count(entity->bookmark_node())); SyncedBookmarkTrackerEntity* mutable_entity = AsMutableEntity(entity); - mutable_entity->metadata()->set_is_deleted(true); - mutable_entity->metadata()->clear_bookmark_favicon_hash(); + mutable_entity->MutableMetadata()->set_is_deleted(true); + mutable_entity->MutableMetadata()->clear_bookmark_favicon_hash(); // Clear all references to the deleted bookmark node. bookmark_node_to_entities_map_.erase(mutable_entity->bookmark_node()); mutable_entity->clear_bookmark_node(); @@ -276,23 +271,23 @@ void SyncedBookmarkTracker::MarkDeleted( void SyncedBookmarkTracker::Remove(const SyncedBookmarkTrackerEntity* entity) { DCHECK(entity); - DCHECK_EQ(entity, GetEntityForSyncId(entity->metadata()->server_id())); + DCHECK_EQ(entity, GetEntityForSyncId(entity->metadata().server_id())); DCHECK_EQ(entity, GetEntityForClientTagHash(entity->GetClientTagHash())); DCHECK_EQ(sync_id_to_entities_map_.size(), client_tag_hash_to_entities_map_.size()); if (entity->bookmark_node()) { - DCHECK(!entity->metadata()->is_deleted()); + DCHECK(!entity->metadata().is_deleted()); DCHECK_EQ(0, base::ranges::count(ordered_local_tombstones_, entity)); bookmark_node_to_entities_map_.erase(entity->bookmark_node()); } else { - DCHECK(entity->metadata()->is_deleted()); + DCHECK(entity->metadata().is_deleted()); } client_tag_hash_to_entities_map_.erase(entity->GetClientTagHash()); base::Erase(ordered_local_tombstones_, entity); - sync_id_to_entities_map_.erase(entity->metadata()->server_id()); + sync_id_to_entities_map_.erase(entity->metadata().server_id()); DCHECK_EQ(sync_id_to_entities_map_.size(), client_tag_hash_to_entities_map_.size()); } @@ -303,8 +298,8 @@ void SyncedBookmarkTracker::IncrementSequenceNumber( DCHECK(!entity->bookmark_node() || !entity->bookmark_node()->is_permanent_node()); - AsMutableEntity(entity)->metadata()->set_sequence_number( - entity->metadata()->sequence_number() + 1); + AsMutableEntity(entity)->MutableMetadata()->set_sequence_number( + entity->metadata().sequence_number() + 1); } sync_pb::BookmarkModelMetadata @@ -325,8 +320,7 @@ SyncedBookmarkTracker::BuildBookmarkModelMetadata() const { for (const auto& [sync_id, entity] : sync_id_to_entities_map_) { DCHECK(entity) << " for ID " << sync_id; - DCHECK(entity->metadata()) << " for ID " << sync_id; - if (entity->metadata()->is_deleted()) { + if (entity->metadata().is_deleted()) { // Deletions will be added later because they need to maintain the same // order as in |ordered_local_tombstones_|. continue; @@ -335,17 +329,16 @@ SyncedBookmarkTracker::BuildBookmarkModelMetadata() const { sync_pb::BookmarkMetadata* bookmark_metadata = model_metadata.add_bookmarks_metadata(); bookmark_metadata->set_id(entity->bookmark_node()->id()); - *bookmark_metadata->mutable_metadata() = *entity->metadata(); + *bookmark_metadata->mutable_metadata() = entity->metadata(); } // Add pending deletions. for (const SyncedBookmarkTrackerEntity* tombstone_entity : ordered_local_tombstones_) { DCHECK(tombstone_entity); - DCHECK(tombstone_entity->metadata()); - DCHECK(tombstone_entity->metadata()->is_deleted()); + DCHECK(tombstone_entity->metadata().is_deleted()); sync_pb::BookmarkMetadata* bookmark_metadata = model_metadata.add_bookmarks_metadata(); - *bookmark_metadata->mutable_metadata() = *tombstone_entity->metadata(); + *bookmark_metadata->mutable_metadata() = tombstone_entity->metadata(); } *model_metadata.mutable_model_type_state() = model_type_state_; return model_metadata; @@ -375,7 +368,7 @@ SyncedBookmarkTracker::GetEntitiesWithLocalChanges() const { // Entities with local non deletions should be sorted such that parent // creation/update comes before child creation/update. for (const auto& [sync_id, entity] : sync_id_to_entities_map_) { - if (entity->metadata()->is_deleted()) { + if (entity->metadata().is_deleted()) { // Deletions are stored sorted in |ordered_local_tombstones_| and will be // added later. continue; @@ -450,8 +443,7 @@ SyncedBookmarkTracker::InitEntitiesFromModelAndMetadata( bookmark_metadata.metadata().client_tag_hash()); auto tombstone_entity = std::make_unique<SyncedBookmarkTrackerEntity>( - /*node=*/nullptr, std::make_unique<sync_pb::EntityMetadata>(std::move( - *bookmark_metadata.mutable_metadata()))); + /*node=*/nullptr, std::move(*bookmark_metadata.mutable_metadata())); if (!client_tag_hash_to_entities_map_ .emplace(client_tag_hash, tombstone_entity.get()) @@ -518,9 +510,18 @@ SyncedBookmarkTracker::InitEntitiesFromModelAndMetadata( } } + // The code populates |bookmark_favicon_hash| for all new nodes, including + // folders, but it is possible that folders are tracked that predate the + // introduction of |bookmark_favicon_hash|, which never got it populated + // because for some time it got populated opportunistically upon favicon + // load, which never triggers for folders. + if (!node->is_folder() && + !bookmark_metadata.metadata().has_bookmark_favicon_hash()) { + return CorruptionReason::MISSING_FAVICON_HASH; + } + auto entity = std::make_unique<SyncedBookmarkTrackerEntity>( - node, std::make_unique<sync_pb::EntityMetadata>( - std::move(*bookmark_metadata.mutable_metadata()))); + node, std::move(*bookmark_metadata.mutable_metadata())); if (!client_tag_hash_to_entities_map_.emplace(client_tag_hash, entity.get()) .second) { @@ -576,7 +577,7 @@ SyncedBookmarkTracker::ReorderUnsyncedEntitiesExceptDeletions( // Collect nodes with updates for (const SyncedBookmarkTrackerEntity* entity : entities) { DCHECK(entity->IsUnsynced()); - DCHECK(!entity->metadata()->is_deleted()); + DCHECK(!entity->metadata().is_deleted()); DCHECK(entity->bookmark_node()); nodes.insert(entity->bookmark_node()); } @@ -602,7 +603,7 @@ bool SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() { return false; } for (const auto& [sync_id, entity] : sync_id_to_entities_map_) { - if (entity->IsUnsynced() || entity->metadata()->is_deleted()) { + if (entity->IsUnsynced() || entity->metadata().is_deleted()) { continue; } if (entity->bookmark_node()->is_permanent_node()) { @@ -645,7 +646,7 @@ void SyncedBookmarkTracker::TraverseAndAppend( const SyncedBookmarkTrackerEntity* entity = GetEntityForBookmarkNode(node); DCHECK(entity); DCHECK(entity->IsUnsynced()); - DCHECK(!entity->metadata()->is_deleted()); + DCHECK(!entity->metadata().is_deleted()); ordered_entities->push_back(entity); // Recurse for all children. for (const auto& child : node->children()) { @@ -660,7 +661,7 @@ void SyncedBookmarkTracker::TraverseAndAppend( // call to TraverseAndAppend(). continue; } - if (child_entity->metadata()->is_deleted()) { + if (child_entity->metadata().is_deleted()) { // Deletion are stored sorted in |ordered_local_tombstones_| and will be // added later. continue; @@ -677,11 +678,12 @@ void SyncedBookmarkTracker::UpdateUponCommitResponse( DCHECK(entity); SyncedBookmarkTrackerEntity* mutable_entity = AsMutableEntity(entity); - mutable_entity->metadata()->set_acked_sequence_number(acked_sequence_number); - mutable_entity->metadata()->set_server_version(server_version); + mutable_entity->MutableMetadata()->set_acked_sequence_number( + acked_sequence_number); + mutable_entity->MutableMetadata()->set_server_version(server_version); // If there are no pending commits, remove tombstones. if (!mutable_entity->IsUnsynced() && - mutable_entity->metadata()->is_deleted()) { + mutable_entity->metadata().is_deleted()) { Remove(mutable_entity); return; } @@ -694,7 +696,7 @@ void SyncedBookmarkTracker::UpdateSyncIdIfNeeded( const std::string& sync_id) { DCHECK(entity); - const std::string old_id = entity->metadata()->server_id(); + const std::string old_id = entity->metadata().server_id(); if (old_id == sync_id) { return; } @@ -704,7 +706,7 @@ void SyncedBookmarkTracker::UpdateSyncIdIfNeeded( std::unique_ptr<SyncedBookmarkTrackerEntity> owned_entity = std::move(sync_id_to_entities_map_.at(old_id)); DCHECK_EQ(entity, owned_entity.get()); - owned_entity->metadata()->set_server_id(sync_id); + owned_entity->MutableMetadata()->set_server_id(sync_id); sync_id_to_entities_map_[sync_id] = std::move(owned_entity); sync_id_to_entities_map_.erase(old_id); } @@ -714,18 +716,18 @@ void SyncedBookmarkTracker::UndeleteTombstoneForBookmarkNode( const bookmarks::BookmarkNode* node) { DCHECK(entity); DCHECK(node); - DCHECK(entity->metadata()->is_deleted()); + DCHECK(entity->metadata().is_deleted()); const syncer::ClientTagHash client_tag_hash = GetClientTagHashFromGUID(node->guid()); // The same entity must be used only for the same bookmark node. - DCHECK_EQ(entity->metadata()->client_tag_hash(), client_tag_hash.value()); + DCHECK_EQ(entity->metadata().client_tag_hash(), client_tag_hash.value()); DCHECK(bookmark_node_to_entities_map_.find(node) == bookmark_node_to_entities_map_.end()); - DCHECK_EQ(GetEntityForSyncId(entity->metadata()->server_id()), entity); + DCHECK_EQ(GetEntityForSyncId(entity->metadata().server_id()), entity); base::Erase(ordered_local_tombstones_, entity); SyncedBookmarkTrackerEntity* mutable_entity = AsMutableEntity(entity); - mutable_entity->metadata()->set_is_deleted(false); + mutable_entity->MutableMetadata()->set_is_deleted(false); mutable_entity->set_bookmark_node(node); bookmark_node_to_entities_map_[node] = mutable_entity; } @@ -733,8 +735,8 @@ void SyncedBookmarkTracker::UndeleteTombstoneForBookmarkNode( void SyncedBookmarkTracker::AckSequenceNumber( const SyncedBookmarkTrackerEntity* entity) { DCHECK(entity); - AsMutableEntity(entity)->metadata()->set_acked_sequence_number( - entity->metadata()->sequence_number()); + AsMutableEntity(entity)->MutableMetadata()->set_acked_sequence_number( + entity->metadata().sequence_number()); } bool SyncedBookmarkTracker::IsEmpty() const { @@ -765,7 +767,7 @@ size_t SyncedBookmarkTracker::TrackedEntitiesCountForTest() const { void SyncedBookmarkTracker::ClearSpecificsHashForTest( const SyncedBookmarkTrackerEntity* entity) { - AsMutableEntity(entity)->metadata()->clear_specifics_hash(); + AsMutableEntity(entity)->MutableMetadata()->clear_specifics_hash(); } void SyncedBookmarkTracker::CheckAllNodesTracked( diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker.h b/chromium/components/sync_bookmarks/synced_bookmark_tracker.h index 96efb7bbe07..49a6d951116 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker.h +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker.h @@ -104,11 +104,6 @@ class SyncedBookmarkTracker { void UpdateServerVersion(const SyncedBookmarkTrackerEntity* entity, int64_t server_version); - // Populates the metadata field representing the hashed favicon. This method - // is effectively used to backfill the proto field, which was introduced late. - void PopulateFaviconHashIfUnset(const SyncedBookmarkTrackerEntity* entity, - const std::string& favicon_png_bytes); - // Marks an existing entry that a commit request might have been sent to the // server. |entity| must be owned by this tracker. void MarkCommitMayHaveStarted(const SyncedBookmarkTrackerEntity* entity); @@ -236,8 +231,9 @@ class SyncedBookmarkTracker { DUPLICATED_CLIENT_TAG_HASH = 10, TRACKED_MANAGED_NODE = 11, MISSING_CLIENT_TAG_HASH = 12, + MISSING_FAVICON_HASH = 13, - kMaxValue = MISSING_CLIENT_TAG_HASH + kMaxValue = MISSING_FAVICON_HASH }; SyncedBookmarkTracker( diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.cc b/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.cc index 2a384ea9562..8e05503500c 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.cc +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.cc @@ -34,58 +34,48 @@ void HashSpecifics(const sync_pb::EntitySpecifics& specifics, SyncedBookmarkTrackerEntity::SyncedBookmarkTrackerEntity( const bookmarks::BookmarkNode* bookmark_node, - std::unique_ptr<sync_pb::EntityMetadata> metadata) + sync_pb::EntityMetadata metadata) : bookmark_node_(bookmark_node), metadata_(std::move(metadata)) { - DCHECK(metadata_); if (bookmark_node) { - DCHECK(!metadata_->is_deleted()); + DCHECK(!metadata_.is_deleted()); } else { - DCHECK(metadata_->is_deleted()); + DCHECK(metadata_.is_deleted()); } } SyncedBookmarkTrackerEntity::~SyncedBookmarkTrackerEntity() = default; bool SyncedBookmarkTrackerEntity::IsUnsynced() const { - return metadata_->sequence_number() > metadata_->acked_sequence_number(); + return metadata_.sequence_number() > metadata_.acked_sequence_number(); } bool SyncedBookmarkTrackerEntity::MatchesData( const syncer::EntityData& data) const { - if (metadata_->is_deleted() || data.is_deleted()) { + if (metadata_.is_deleted() || data.is_deleted()) { // In case of deletion, no need to check the specifics. - return metadata_->is_deleted() == data.is_deleted(); + return metadata_.is_deleted() == data.is_deleted(); } return MatchesSpecificsHash(data.specifics); } bool SyncedBookmarkTrackerEntity::MatchesSpecificsHash( const sync_pb::EntitySpecifics& specifics) const { - DCHECK(!metadata_->is_deleted()); + DCHECK(!metadata_.is_deleted()); DCHECK_GT(specifics.ByteSize(), 0); std::string hash; HashSpecifics(specifics, &hash); - return hash == metadata_->specifics_hash(); + return hash == metadata_.specifics_hash(); } bool SyncedBookmarkTrackerEntity::MatchesFaviconHash( const std::string& favicon_png_bytes) const { - DCHECK(!metadata_->is_deleted()); - return metadata_->bookmark_favicon_hash() == + DCHECK(!metadata_.is_deleted()); + return metadata_.bookmark_favicon_hash() == base::PersistentHash(favicon_png_bytes); } -void SyncedBookmarkTrackerEntity::PopulateFaviconHashIfUnset( - const std::string& favicon_png_bytes) { - if (metadata_->has_bookmark_favicon_hash()) { - return; - } - - metadata_->set_bookmark_favicon_hash(base::PersistentHash(favicon_png_bytes)); -} - syncer::ClientTagHash SyncedBookmarkTrackerEntity::GetClientTagHash() const { - return syncer::ClientTagHash::FromHashed(metadata_->client_tag_hash()); + return syncer::ClientTagHash::FromHashed(metadata_.client_tag_hash()); } size_t SyncedBookmarkTrackerEntity::EstimateMemoryUsage() const { diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.h b/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.h index 9400f4f4603..9a370deabb6 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.h +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker_entity.h @@ -5,14 +5,13 @@ #ifndef COMPONENTS_SYNC_BOOKMARKS_SYNCED_BOOKMARK_TRACKER_ENTITY_H_ #define COMPONENTS_SYNC_BOOKMARKS_SYNCED_BOOKMARK_TRACKER_ENTITY_H_ -#include <memory> #include <string> #include "base/memory/raw_ptr.h" #include "components/sync/base/client_tag_hash.h" +#include "components/sync/protocol/entity_metadata.pb.h" namespace sync_pb { -class EntityMetadata; class EntitySpecifics; } // namespace sync_pb @@ -31,10 +30,9 @@ namespace sync_bookmarks { // is not reused for bookmarks for historic reasons. class SyncedBookmarkTrackerEntity { public: - // |bookmark_node| can be null for tombstones. |metadata| must not be null. - SyncedBookmarkTrackerEntity( - const bookmarks::BookmarkNode* bookmark_node, - std::unique_ptr<sync_pb::EntityMetadata> metadata); + // |bookmark_node| can be null for tombstones. + SyncedBookmarkTrackerEntity(const bookmarks::BookmarkNode* bookmark_node, + sync_pb::EntityMetadata metadata); SyncedBookmarkTrackerEntity(const SyncedBookmarkTrackerEntity&) = delete; SyncedBookmarkTrackerEntity(SyncedBookmarkTrackerEntity&&) = delete; ~SyncedBookmarkTrackerEntity(); @@ -73,17 +71,15 @@ class SyncedBookmarkTrackerEntity { bookmark_node_ = bookmark_node; } - const sync_pb::EntityMetadata* metadata() const { return metadata_.get(); } + const sync_pb::EntityMetadata& metadata() const { return metadata_; } - sync_pb::EntityMetadata* metadata() { return metadata_.get(); } + sync_pb::EntityMetadata* MutableMetadata() { return &metadata_; } bool commit_may_have_started() const { return commit_may_have_started_; } void set_commit_may_have_started(bool value) { commit_may_have_started_ = value; } - void PopulateFaviconHashIfUnset(const std::string& favicon_png_bytes); - syncer::ClientTagHash GetClientTagHash() const; // Returns the estimate of dynamically allocated memory in bytes. @@ -94,7 +90,7 @@ class SyncedBookmarkTrackerEntity { raw_ptr<const bookmarks::BookmarkNode> bookmark_node_; // Serializable Sync metadata. - const std::unique_ptr<sync_pb::EntityMetadata> metadata_; + sync_pb::EntityMetadata metadata_; // Whether there could be a commit sent to the server for this entity. It's // used to protect against sending tombstones for entities that have never diff --git a/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc b/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc index 489920930f8..d1e04c5abb4 100644 --- a/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc +++ b/chromium/components/sync_bookmarks/synced_bookmark_tracker_unittest.cc @@ -55,8 +55,9 @@ enum class ExpectedCorruptionReason { DUPLICATED_CLIENT_TAG_HASH = 10, TRACKED_MANAGED_NODE = 11, MISSING_CLIENT_TAG_HASH = 12, + MISSING_FAVICON_HASH = 13, - kMaxValue = MISSING_CLIENT_TAG_HASH + kMaxValue = MISSING_FAVICON_HASH }; sync_pb::EntitySpecifics GenerateSpecifics(const std::string& title, @@ -82,6 +83,10 @@ sync_pb::BookmarkMetadata CreateNodeMetadata( syncer::ClientTagHash::FromUnhashed(syncer::BOOKMARKS, node->guid().AsLowercaseString()) .value()); + // Required by the validation logic. + if (!node->is_folder()) { + bookmark_metadata.mutable_metadata()->set_bookmark_favicon_hash(123); + } return bookmark_metadata; } @@ -138,12 +143,12 @@ TEST(SyncedBookmarkTrackerTest, ShouldAddEntity) { EXPECT_THAT(entity->GetClientTagHash(), Eq(syncer::ClientTagHash::FromUnhashed( syncer::BOOKMARKS, kGuid.AsLowercaseString()))); - EXPECT_THAT(entity->metadata()->server_id(), Eq(kSyncId)); - EXPECT_THAT(entity->metadata()->server_version(), Eq(kServerVersion)); - EXPECT_THAT(entity->metadata()->creation_time(), + EXPECT_THAT(entity->metadata().server_id(), Eq(kSyncId)); + EXPECT_THAT(entity->metadata().server_version(), Eq(kServerVersion)); + EXPECT_THAT(entity->metadata().creation_time(), Eq(syncer::TimeToProtoTime(kCreationTime))); EXPECT_TRUE( - syncer::UniquePosition::FromProto(entity->metadata()->unique_position()) + syncer::UniquePosition::FromProto(entity->metadata().unique_position()) .Equals(syncer::UniquePosition::FromProto( specifics.bookmark().unique_position()))); EXPECT_THAT(tracker->GetEntityForSyncId(kSyncId), Eq(entity)); @@ -297,9 +302,9 @@ TEST(SyncedBookmarkTrackerTest, ShouldUpdateUponCommitResponseWithNewId) { EXPECT_THAT(tracker->GetEntityForSyncId(kSyncId), IsNull()); EXPECT_THAT(tracker->GetEntityForSyncId(kNewSyncId), Eq(entity)); - EXPECT_THAT(entity->metadata()->server_id(), Eq(kNewSyncId)); + EXPECT_THAT(entity->metadata().server_id(), Eq(kNewSyncId)); EXPECT_THAT(entity->bookmark_node(), Eq(&node)); - EXPECT_THAT(entity->metadata()->server_version(), Eq(kNewServerVersion)); + EXPECT_THAT(entity->metadata().server_version(), Eq(kNewServerVersion)); } TEST(SyncedBookmarkTrackerTest, ShouldUpdateId) { @@ -326,9 +331,9 @@ TEST(SyncedBookmarkTrackerTest, ShouldUpdateId) { EXPECT_THAT(tracker->GetEntityForSyncId(kSyncId), IsNull()); EXPECT_THAT(tracker->GetEntityForSyncId(kNewSyncId), Eq(entity)); - EXPECT_THAT(entity->metadata()->server_id(), Eq(kNewSyncId)); + EXPECT_THAT(entity->metadata().server_id(), Eq(kNewSyncId)); EXPECT_THAT(entity->bookmark_node(), Eq(&node)); - EXPECT_THAT(entity->metadata()->server_version(), Eq(kServerVersion)); + EXPECT_THAT(entity->metadata().server_version(), Eq(kServerVersion)); } TEST(SyncedBookmarkTrackerTest, @@ -483,7 +488,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldMarkDeleted) { tracker->GetEntityForClientTagHash(syncer::ClientTagHash::FromUnhashed( syncer::BOOKMARKS, kGuid.AsLowercaseString())), Eq(entity)); - ASSERT_FALSE(entity->metadata()->is_deleted()); + ASSERT_FALSE(entity->metadata().is_deleted()); ASSERT_THAT(entity->bookmark_node(), Eq(&node)); // Delete the bookmark, leading to a pending deletion (local tombstone). @@ -496,7 +501,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldMarkDeleted) { tracker->GetEntityForClientTagHash(syncer::ClientTagHash::FromUnhashed( syncer::BOOKMARKS, kGuid.AsLowercaseString())), Eq(entity)); - EXPECT_TRUE(entity->metadata()->is_deleted()); + EXPECT_TRUE(entity->metadata().is_deleted()); EXPECT_THAT(entity->bookmark_node(), IsNull()); } @@ -521,7 +526,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldUndeleteTombstone) { // Delete the bookmark, leading to a pending deletion (local tombstone). tracker->MarkDeleted(entity); ASSERT_THAT(entity->bookmark_node(), IsNull()); - ASSERT_TRUE(entity->metadata()->is_deleted()); + ASSERT_TRUE(entity->metadata().is_deleted()); ASSERT_THAT(tracker->TrackedUncommittedTombstonesCount(), Eq(1U)); ASSERT_THAT(tracker->GetEntityForBookmarkNode(&node), IsNull()); ASSERT_THAT( @@ -533,7 +538,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldUndeleteTombstone) { tracker->UndeleteTombstoneForBookmarkNode(entity, &node); EXPECT_THAT(entity->bookmark_node(), NotNull()); - EXPECT_FALSE(entity->metadata()->is_deleted()); + EXPECT_FALSE(entity->metadata().is_deleted()); EXPECT_THAT(tracker->TrackedUncommittedTombstonesCount(), Eq(0U)); ASSERT_THAT(tracker->GetEntityForBookmarkNode(&node), Eq(entity)); EXPECT_THAT( @@ -599,11 +604,11 @@ TEST(SyncedBookmarkTrackerTest, ASSERT_THAT(entities_with_local_change.size(), Eq(4U)); // Verify updates are in parent before child order node0 --> node1 --> node2. - EXPECT_THAT(entities_with_local_change[0]->metadata()->server_id(), Eq(kId0)); - EXPECT_THAT(entities_with_local_change[1]->metadata()->server_id(), Eq(kId1)); - EXPECT_THAT(entities_with_local_change[2]->metadata()->server_id(), Eq(kId2)); + EXPECT_THAT(entities_with_local_change[0]->metadata().server_id(), Eq(kId0)); + EXPECT_THAT(entities_with_local_change[1]->metadata().server_id(), Eq(kId1)); + EXPECT_THAT(entities_with_local_change[2]->metadata().server_id(), Eq(kId2)); // Verify that deletion is the last entry. - EXPECT_THAT(entities_with_local_change[3]->metadata()->server_id(), Eq(kId3)); + EXPECT_THAT(entities_with_local_change[3]->metadata().server_id(), Eq(kId3)); } TEST(SyncedBookmarkTrackerTest, ShouldNotInvalidateMetadata) { @@ -927,6 +932,34 @@ TEST(SyncedBookmarkTrackerTest, /*expected_bucket_count=*/1); } +TEST(SyncedBookmarkTrackerTest, ShouldInvalidateMetadataIfMissingFaviconHash) { + std::unique_ptr<bookmarks::BookmarkModel> model = + bookmarks::TestBookmarkClient::CreateModel(); + + const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node(); + const bookmarks::BookmarkNode* node0 = + model->AddURL(/*parent=*/bookmark_bar_node, /*index=*/0, u"Title", + GURL("http://www.url.com")); + + sync_pb::BookmarkModelMetadata model_metadata = + CreateMetadataForPermanentNodes(model.get()); + sync_pb::BookmarkMetadata* node0_metadata = + model_metadata.add_bookmarks_metadata(); + *node0_metadata = CreateNodeMetadata(node0, /*server_id=*/"id0"); + + node0_metadata->mutable_metadata()->clear_bookmark_favicon_hash(); + + base::HistogramTester histogram_tester; + EXPECT_THAT(SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( + model.get(), std::move(model_metadata)), + IsNull()); + + histogram_tester.ExpectUniqueSample( + "Sync.BookmarksModelMetadataCorruptionReason", + /*sample=*/ExpectedCorruptionReason::MISSING_FAVICON_HASH, + /*expected_bucket_count=*/1); +} + TEST(SyncedBookmarkTrackerTest, ShouldMatchModelWithUnsyncableNodesAndMetadata) { auto client = std::make_unique<bookmarks::TestBookmarkClient>(); @@ -974,7 +1007,7 @@ TEST(SyncedBookmarkTrackerTest, const SyncedBookmarkTrackerEntity* entity = tracker->Add(&node, kSyncId, kServerVersion, kCreationTime, specifics); - EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); + EXPECT_TRUE(entity->metadata().has_bookmark_favicon_hash()); EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes)); EXPECT_FALSE(entity->MatchesFaviconHash("otherhash")); } @@ -1009,7 +1042,6 @@ TEST(SyncedBookmarkTrackerTest, ShouldPopulateFaviconHashUponUpdate) { const SyncedBookmarkTrackerEntity* entity = tracker->GetEntityForSyncId(kSyncId); ASSERT_THAT(entity, NotNull()); - ASSERT_FALSE(entity->metadata()->has_bookmark_favicon_hash()); ASSERT_FALSE(entity->MatchesFaviconHash(kFaviconPngBytes)); sync_pb::EntitySpecifics specifics = GenerateSpecifics(kTitle, kUrl.spec()); @@ -1017,48 +1049,9 @@ TEST(SyncedBookmarkTrackerTest, ShouldPopulateFaviconHashUponUpdate) { tracker->Update(entity, kServerVersion, kModificationTime, specifics); - EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); - EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes)); - EXPECT_FALSE(entity->MatchesFaviconHash("otherhash")); -} - -TEST(SyncedBookmarkTrackerTest, ShouldPopulateFaviconHashExplicitly) { - const std::string kSyncId = "SYNC_ID"; - const std::string kFaviconPngBytes = "fakefaviconbytes"; - - std::unique_ptr<bookmarks::BookmarkModel> model = - bookmarks::TestBookmarkClient::CreateModel(); - - const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node(); - const bookmarks::BookmarkNode* node = - model->AddURL(/*parent=*/bookmark_bar_node, /*index=*/0, u"Title", - GURL("http://www.url.com")); - - sync_pb::BookmarkModelMetadata model_metadata = - CreateMetadataForPermanentNodes(model.get()); - - // Add entry for the URL node. - *model_metadata.add_bookmarks_metadata() = CreateNodeMetadata(node, kSyncId); - - std::unique_ptr<SyncedBookmarkTracker> tracker = - SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata( - model.get(), std::move(model_metadata)); - ASSERT_THAT(tracker, NotNull()); - - const SyncedBookmarkTrackerEntity* entity = - tracker->GetEntityForSyncId(kSyncId); - ASSERT_THAT(entity, NotNull()); - ASSERT_FALSE(entity->metadata()->has_bookmark_favicon_hash()); - ASSERT_FALSE(entity->MatchesFaviconHash(kFaviconPngBytes)); - - tracker->PopulateFaviconHashIfUnset(entity, kFaviconPngBytes); - EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash()); + EXPECT_TRUE(entity->metadata().has_bookmark_favicon_hash()); EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes)); EXPECT_FALSE(entity->MatchesFaviconHash("otherhash")); - - // Further calls should be ignored. - tracker->PopulateFaviconHashIfUnset(entity, "otherpngbytes"); - EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes)); } TEST(SyncedBookmarkTrackerTest, ShouldNotReuploadEntitiesAfterMergeAndRestart) { |